[ARVADOS] updated: 1.1.0-38-gca3d900

[Git user]

Summary of changes:
 services/crunch-run/crunchrun.go      | 30 +++++++++++++++++++++---------
 services/crunch-run/crunchrun_test.go | 18 ++++++++++++++++++
 2 files changed, 39 insertions(+), 9 deletions(-)

       via  ca3d9008da4a57e71cb7e6dfd1e25b5eb0c0de0b (commit)
      from  e5f33b99e1c363e4d14f429075fdce15b9643000 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit ca3d9008da4a57e71cb7e6dfd1e25b5eb0c0de0b
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Mon Oct 16 16:39:36 2017 -0400

    12183: Handle circular directory symlinks
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/crunch-run/crunchrun.go b/services/crunch-run/crunchrun.go
index f900d19..cb80436 100644
--- a/services/crunch-run/crunchrun.go
+++ b/services/crunch-run/crunchrun.go
@@ -914,6 +914,8 @@ func (runner *ContainerRunner) WaitFinish() (err error) {
 	return nil
 }
 
+var NotInOutputDirError = fmt.Errorf("Must point to path within the output directory")
+
 func (runner *ContainerRunner) derefOutputSymlink(path string, startinfo os.FileInfo) (tgt string, readlinktgt string, info os.FileInfo, err error) {
 	// Follow symlinks if necessary
 	info = startinfo
@@ -921,7 +923,7 @@ func (runner *ContainerRunner) derefOutputSymlink(path string, startinfo os.File
 	readlinktgt = ""
 	nextlink := path
 	for followed := 0; info.Mode()&os.ModeSymlink != 0; followed++ {
-		if followed >= 32 {
+		if followed >= 16 {
 			// Got stuck in a loop or just a pathological number of links, give up.
 			err = fmt.Errorf("Followed too many symlinks from path %q", path)
 			return
@@ -945,14 +947,15 @@ func (runner *ContainerRunner) derefOutputSymlink(path string, startinfo os.File
 			// After dereferencing, symlink target must either be
 			// within output directory, or must point to a
 			// collection mount.
-			break
+			err = NotInOutputDirError
+			return
 		}
 
 		info, err = os.Lstat(tgt)
 		if err != nil {
 			// tgt
-			err = fmt.Errorf("Symlink in output %q points to invalid location %q, must exist, be readable, and point to path within the output directory.",
-				path[len(runner.HostOutputDir):], readlinktgt)
+			err = fmt.Errorf("Symlink in output %q points to invalid location %q: %v",
+				path[len(runner.HostOutputDir):], readlinktgt, err)
 			return
 		}
 
@@ -976,7 +979,8 @@ func (runner *ContainerRunner) UploadOutputFile(
 	binds []string,
 	walkUpload *WalkUpload,
 	relocateFrom string,
-	relocateTo string) (manifestText string, err error) {
+	relocateTo string,
+	followed int) (manifestText string, err error) {
 
 	if info.Mode().IsDir() {
 		return
@@ -986,13 +990,20 @@ func (runner *ContainerRunner) UploadOutputFile(
 		return "", infoerr
 	}
 
+	if followed >= 8 {
+		// Got stuck in a loop or just a pathological number of
+		// directory links, give up.
+		err = fmt.Errorf("Followed too many symlinks from path %q", path)
+		return
+	}
+
 	// When following symlinks, the source path may need to be logically
 	// relocated to some other path within the output collection.  Remove
 	// the relocateFrom prefix and replace it with relocateTo.
 	relocated := relocateTo + path[len(relocateFrom):]
 
 	tgt, readlinktgt, info, derefErr := runner.derefOutputSymlink(path, info)
-	if derefErr != nil {
+	if derefErr != nil && derefErr != NotInOutputDirError {
 		return "", derefErr
 	}
 
@@ -1017,7 +1028,7 @@ func (runner *ContainerRunner) UploadOutputFile(
 
 	// If target is not a collection mount, it must be located within the
 	// output directory, otherwise it is an error.
-	if !strings.HasPrefix(tgt, runner.HostOutputDir+"/") {
+	if derefErr == NotInOutputDirError {
 		err = fmt.Errorf("Symlink in output %q points to invalid location %q, must point to path within the output directory.",
 			path[len(runner.HostOutputDir):], readlinktgt)
 		return
@@ -1034,7 +1045,8 @@ func (runner *ContainerRunner) UploadOutputFile(
 		// so they appear under the original symlink path.
 		err = filepath.Walk(tgt, func(walkpath string, walkinfo os.FileInfo, walkerr error) error {
 			var m string
-			m, walkerr = runner.UploadOutputFile(walkpath, walkinfo, walkerr, binds, walkUpload, tgt, relocated)
+			m, walkerr = runner.UploadOutputFile(walkpath, walkinfo, walkerr,
+				binds, walkUpload, tgt, relocated, followed+1)
 			if walkerr == nil {
 				manifestText = manifestText + m
 			}
@@ -1097,7 +1109,7 @@ func (runner *ContainerRunner) CaptureOutput() error {
 
 		var m string
 		err = filepath.Walk(runner.HostOutputDir, func(path string, info os.FileInfo, err error) error {
-			m, err = runner.UploadOutputFile(path, info, err, binds, walkUpload, "", "")
+			m, err = runner.UploadOutputFile(path, info, err, binds, walkUpload, "", "", 0)
 			if err == nil {
 				manifestText = manifestText + m
 			}
diff --git a/services/crunch-run/crunchrun_test.go b/services/crunch-run/crunchrun_test.go
index 098a642..d3c9990 100644
--- a/services/crunch-run/crunchrun_test.go
+++ b/services/crunch-run/crunchrun_test.go
@@ -1715,3 +1715,21 @@ func (s *TestSuite) TestEvalSymlinks(c *C) {
 		c.Assert(err, NotNil)
 	}
 }
+
+func (s *TestSuite) TestEvalSymlinkDir(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(".", realTemp+"/loop")
+
+	v := "loop"
+	info, err := os.Lstat(realTemp + "/" + v)
+	_, err = cr.UploadOutputFile(realTemp+"/"+v, info, err, []string{}, nil, "", "", 0)
+	c.Assert(err, NotNil)
+}

-----------------------------------------------------------------------


hooks/post-receive
--