From: Peter Amstutz <peter.amstutz@curoverse.com>
Date: Mon, 2 Oct 2017 19:34:38 +0000 (-0400)
Subject: 12183: Improve error handling if unable to delete symlinks marked for deletion.
X-Git-Tag: 1.1.1~24^2~10
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/f93e925c4a4322ee4ef2244a0eb99552a01fce9a?hp=174d343cf86e02e083293746f86366e1d0a95786

12183: Improve error handling if unable to delete symlinks marked for deletion.

Add unit test for EvalSymlinks.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>
---

diff --git a/services/crunch-run/crunchrun.go b/services/crunch-run/crunchrun.go
index c50f799e8c..83c4ca3976 100644
--- a/services/crunch-run/crunchrun.go
+++ b/services/crunch-run/crunchrun.go
@@ -942,7 +942,6 @@ func (runner *ContainerRunner) EvalSymlinks(path string, binds []string) (manife
 			return
 		}
 
-		var tgt string
 		if info.Mode()&os.ModeSymlink == 0 {
 			// Not a symlink, nothing to do.
 			return
@@ -951,7 +950,9 @@ func (runner *ContainerRunner) EvalSymlinks(path string, binds []string) (manife
 		// Remember symlink for cleanup later
 		links = append(links, path)
 
-		tgt, err = os.Readlink(path)
+		var readlinktgt string
+		readlinktgt, err = os.Readlink(path)
+		tgt := readlinktgt
 		if err != nil {
 			return
 		}
@@ -999,13 +1000,22 @@ func (runner *ContainerRunner) EvalSymlinks(path string, binds []string) (manife
 		// directory, otherwise it is an error.
 		if !strings.HasPrefix(tgt, runner.HostOutputDir+"/") {
 			err = fmt.Errorf("Output directory symlink %q points to invalid location %q, must point to mount or output directory.",
-				path[len(runner.HostOutputDir):], tgt)
+				path[len(runner.HostOutputDir):], readlinktgt)
 			return
 		}
 
 		// Update symlink to host FS
-		os.Remove(path)
-		os.Symlink(tgt, path)
+		err = os.Remove(path)
+		if err != nil {
+			err = fmt.Errorf("Error removing symlink %q: %v", path, err)
+			return
+		}
+
+		err = os.Symlink(tgt, path)
+		if err != nil {
+			err = fmt.Errorf("Error updating symlink %q: %v", path, err)
+			return
+		}
 
 		// Target is within the output directory, so loop and check if
 		// it is also a symlink.
@@ -1062,7 +1072,7 @@ func (runner *ContainerRunner) CaptureOutput() error {
 	if err != nil {
 		// Regular directory
 
-		var symlinksToRemove []string
+		symlinksToRemove := make(map[string]bool)
 		var m string
 		var srm []string
 		// Find symlinks to arv-mounted files & dirs.
@@ -1071,14 +1081,24 @@ func (runner *ContainerRunner) CaptureOutput() error {
 				return err
 			}
 			m, srm, err = runner.EvalSymlinks(path, binds)
-			symlinksToRemove = append(symlinksToRemove, srm...)
+			for _, r := range srm {
+				symlinksToRemove[r] = true
+			}
 			if err == nil {
 				manifestText = manifestText + m
 			}
 			return err
 		})
-		for _, l := range symlinksToRemove {
-			os.Remove(l)
+		for l, _ := range symlinksToRemove {
+			err2 := os.Remove(l)
+			if err2 != nil {
+				if err == nil {
+					err = fmt.Errorf("Error removing symlink %q: %v", err2)
+				} else {
+					err = fmt.Errorf("%v\nError removing symlink %q: %v",
+						err, err2)
+				}
+			}
 		}
 		if err != nil {
 			return fmt.Errorf("While checking output symlinks: %v", err)
diff --git a/services/crunch-run/crunchrun_test.go b/services/crunch-run/crunchrun_test.go
index 9081783bdd..d3606ec6cb 100644
--- a/services/crunch-run/crunchrun_test.go
+++ b/services/crunch-run/crunchrun_test.go
@@ -1685,3 +1685,41 @@ func (s *TestSuite) TestNumberRoundTrip(c *C) {
 	c.Check(err, IsNil)
 	c.Check(string(jsondata), Equals, `{"number":123456789123456789}`)
 }
+
+func (s *TestSuite) TestEvalSymlinks(c *C) {
+	cr := NewContainerRunner(&ArvTestClient{callraw: true}, &KeepTestClient{}, nil, "zzzzz-zzzzz-zzzzzzzzzzzzzzz")
+
+	realTemp, err := ioutil.TempDir("", "crunchrun_test-")
+	c.Assert(err, IsNil)
+	defer os.RemoveAll(realTemp)
+
+	cr.HostOutputDir = realTemp
+
+	// Absolute path outside output dir
+	os.Symlink("/etc/passwd", realTemp+"/p1")
+
+	// Relative outside output dir
+	os.Symlink("../..", realTemp+"/p2")
+
+	// Circular references
+	os.Symlink("p4", realTemp+"/p3")
+	os.Symlink("p5", realTemp+"/p4")
+	os.Symlink("p3", realTemp+"/p5")
+
+	symlinksToRemove := make(map[string]bool)
+	for _, v := range []string{"p1", "p2", "p3", "p4", "p5"} {
+		var srm []string
+		_, srm, err = cr.EvalSymlinks(realTemp+"/"+v, []string{})
+		c.Assert(err, NotNil)
+		for _, r := range srm {
+			symlinksToRemove[r] = true
+		}
+	}
+	c.Assert(len(symlinksToRemove), Equals, 5)
+
+	c.Assert(map[string]bool{realTemp + "/" + "p1": true,
+		realTemp + "/" + "p2": true,
+		realTemp + "/" + "p3": true,
+		realTemp + "/" + "p4": true,
+		realTemp + "/" + "p5": true}, DeepEquals, symlinksToRemove)
+}
diff --git a/services/crunch-run/upload.go b/services/crunch-run/upload.go
index faf20b4f8b..7b13109533 100644
--- a/services/crunch-run/upload.go
+++ b/services/crunch-run/upload.go
@@ -283,7 +283,7 @@ func (m *WalkUpload) WalkFunc(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return fmt.Errorf("stat symlink %q target %q: %s", path, targetPath, err)
 		}
-		if targetInfo.Mode()&os.ModeDir != 0 {
+		if targetInfo.IsDir() {
 			// Symlinks to directories don't get walked, so do it
 			// here.  We've previously checked that they stay in
 			// the output directory and don't result in an endless