Clean up the created jail directory if building the jail doesn't work

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)