diff options
author | Nick Thomas <nick@gitlab.com> | 2018-04-25 16:55:04 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2018-04-25 16:55:04 +0300 |
commit | 67e2fe7eebe60809c6b26e24653ea1a8409abf82 (patch) | |
tree | 2f7acedf0db2507328b514539d87126eef2ad19e /internal/jail | |
parent | ce40ae2b0ec9cb3060a7a0e3b1c04642d10be224 (diff) | |
parent | 17e77e203eb27c58ab907aa61aec48dc6b2fc500 (diff) |
Merge branch '129-fix-chroot-with-bind-mount' into 'master'
Clean up the created jail directory if building the jail doesn't work
Closes #131
See merge request gitlab-org/gitlab-pages!90
Diffstat (limited to 'internal/jail')
-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 2295b730..2acbcd6d 100644 --- a/internal/jail/jail_test.go +++ b/internal/jail/jail_test.go @@ -57,6 +57,33 @@ func TestJailBuild(t *testing.T) { require.NoError(t, 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) |