diff options
author | Nick Thomas <nick@gitlab.com> | 2018-04-23 20:08:02 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2018-04-27 19:23:34 +0300 |
commit | fef9c6546b3ce00234db349dac2eabce1ffd2b40 (patch) | |
tree | 43d7fa5d04cb95a9fa11f8afcddf92b24c88827c | |
parent | 54058312eef037d76d9ae9217cec12cd67cf9f92 (diff) |
Clean up the created jail directory if building the jail doesn't work
-rw-r--r-- | internal/jail/jail.go | 36 | ||||
-rw-r--r-- | internal/jail/jail_test.go | 27 |
2 files changed, 57 insertions, 6 deletions
diff --git a/internal/jail/jail.go b/internal/jail/jail.go index 745ccb2a..16690235 100644 --- a/internal/jail/jail.go +++ b/internal/jail/jail.go @@ -41,32 +41,56 @@ func (j *Jail) Path() string { return j.directories[0].path } -// Build creates the jail, making directories and copying files +// Build creates the jail, making directories and copying files. If an error +// setting up is encountered, a best-effort attempt will be made to remove any +// partial state before returning the error func (j *Jail) Build() error { + // Simplify error-handling in this method. It's unsafe to run os.RemoveAll() + // across a bind mount. Only one is needed at present, and this restriction + // means there's no need to handle the case where one of several mounts + // failed in j.mount() + // + // Make j.mount() robust before removing this restriction, at the risk of + // extreme data loss + if len(j.bindMounts) > 1 { + return fmt.Errorf("BUG: jail does not currently support multiple bind mounts") + } + for _, dir := range j.directories { if err := os.Mkdir(dir.path, dir.mode); err != nil { + j.removeAll() return fmt.Errorf("Can't create directory %q. %s", dir.path, err) } } for dest, src := range j.files { if err := copyFile(dest, src.path, src.mode); err != nil { + j.removeAll() return fmt.Errorf("Can't copy %q -> %q. %s", src.path, dest, err) } } - return j.mount() + if err := j.mount(); err != nil { + // Only one bind mount is supported. If it failed to mount, there is + // nothing to unmount, so it is safe to run removeAll() here. + j.removeAll() + return err + } + + return nil +} + +func (j *Jail) removeAll() error { + return os.RemoveAll(j.Path()) } // Dispose erases everything inside the jail func (j *Jail) Dispose() error { - err := j.unmount() - if err != nil { + if err := j.unmount(); err != nil { return err } - err = os.RemoveAll(j.Path()) - if err != nil { + if err := j.removeAll(); err != nil { return fmt.Errorf("Can't delete jail %q. %s", j.Path(), err) } diff --git a/internal/jail/jail_test.go b/internal/jail/jail_test.go index 2095000e..6c96ceeb 100644 --- a/internal/jail/jail_test.go +++ b/internal/jail/jail_test.go @@ -57,6 +57,33 @@ func TestJailBuild(t *testing.T) { assert.NoError(err, "Jail path should exist after Jail.Build()") } +func TestJailOnlySupportsOneBindMount(t *testing.T) { + jailPath := tmpJailPath() + cage := jail.New(jailPath, 0755) + + cage.Bind("/bin", "/bin") + cage.Bind("/lib", "/lib") + cage.Bind("/usr", "/usr") + + err := cage.Build() + require.Error(t, err, "Build() is expected to fail in this test") + + _, statErr := os.Stat(cage.Path()) + require.True(t, os.IsNotExist(statErr), "Jail path should not exist") +} + +func TestJailBuildCleansUpWhenMountFails(t *testing.T) { + jailPath := tmpJailPath() + cage := jail.New(jailPath, 0755) + cage.Bind("/foo", "/this/path/does/not/exist/so/mount/will/fail") + + err := cage.Build() + require.Error(t, err, "Build() is expected to fail in this test") + + _, statErr := os.Stat(cage.Path()) + require.True(t, os.IsNotExist(statErr), "Jail path should have been cleaned up") +} + func TestJailDispose(t *testing.T) { assert := assert.New(t) |