Adds logic to prevent symlink cycles from causing deadlock

Author: ForestEckhardt

vacation/tar_archive.go

@@ -130,11 +130,11 @@ func (ta TarArchive) Decompress(destination string) error {
 		symlinkMap[filepath.Clean(h.path)] = h.linkname
 	}
 
-	// Loop over map until it is empty this is potentially O(infinity) if there
-	// is a cyclical link but I like to live on the edge
-	for len(symlinkMap) > 0 {
-		// Name the outer loop as an escape hatch
-	Builder:
+	// Iterate over the symlink map for every link that is found this ensures
+	// that all symlinks that can be created will be created and any that are
+	// left over are cyclically dependent
+	maxIterations := len(symlinkMap)
+	for i := 0; i < maxIterations; i++ {
 		for path, linkname := range symlinkMap {
 			// Check to see if the linkname lies on the path of another symlink in
 			// the table or if it is another symlink in the table
@@ -147,11 +147,18 @@ func (ta TarArchive) Decompress(destination string) error {
 			//
 			// If there is a match either of the symlink or it is on the path then
 			// skip the creation of this symlink for now
-			sln := strings.Split(linkname, "/")
-			for i := 0; i < len(sln); i++ {
-				if _, ok := symlinkMap[linknameFullPath(path, filepath.Join(sln[:i+1]...))]; ok {
-					continue Builder
+			shouldSkipLink := func() bool {
+				sln := strings.Split(linkname, "/")
+				for j := 0; j < len(sln); j++ {
+					if _, ok := symlinkMap[linknameFullPath(path, filepath.Join(sln[:j+1]...))]; ok {
+						return true
+					}
 				}
+				return false
+			}
+
+			if shouldSkipLink() {
+				continue
 			}
 
 			// If the linkname is not an existing link in the symlink table then we
@@ -175,6 +182,12 @@ func (ta TarArchive) Decompress(destination string) error {
 		}
 	}
 
+	// Check to see if there are any symlinks left in the map which would
+	// indicate a cyclical dependency
+	if len(symlinkMap) > 0 {
+		return fmt.Errorf("failed: max iterations reached: this symlink graph contains a cycle")
+	}
+
 	return nil
 }
 

vacation/tar_archive_test.go

@@ -289,5 +289,33 @@ func testTarArchive(t *testing.T, context spec.G, it spec.S) {
 				})
 			})
 		})
+
+		context("when there is a symlink cycle", func() {
+			var cyclicalSymlinkTar vacation.TarArchive
+
+			it.Before(func() {
+				var err error
+
+				buffer := bytes.NewBuffer(nil)
+				tw := tar.NewWriter(buffer)
+
+				Expect(tw.WriteHeader(&tar.Header{Name: "a-symlink", Mode: 0755, Size: int64(0), Typeflag: tar.TypeSymlink, Linkname: "b-symlink"})).To(Succeed())
+				_, err = tw.Write([]byte{})
+				Expect(err).NotTo(HaveOccurred())
+
+				Expect(tw.WriteHeader(&tar.Header{Name: "b-symlink", Mode: 0755, Size: int64(0), Typeflag: tar.TypeSymlink, Linkname: "a-symlink"})).To(Succeed())
+				_, err = tw.Write([]byte{})
+				Expect(err).NotTo(HaveOccurred())
+
+				Expect(tw.Close()).To(Succeed())
+
+				cyclicalSymlinkTar = vacation.NewTarArchive(bytes.NewReader(buffer.Bytes()))
+			})
+
+			it("returns an error", func() {
+				err := cyclicalSymlinkTar.Decompress(tempDir)
+				Expect(err).To(MatchError(ContainSubstring("failed: max iterations reached: this symlink graph contains a cycle")))
+			})
+		})
 	})
 }

vacation/zip_archive.go

@@ -137,11 +137,11 @@ func (z ZipArchive) Decompress(destination string) error {
 		symlinkMap[filepath.Clean(h.path)] = h.linkname
 	}
 
-	// Loop over map until it is empty this is potentially O(infinity) if there
-	// is a cyclical link but I like to live on the edge
-	for len(symlinkMap) > 0 {
-		// Name the outer loop as an escape hatch
-	Builder:
+	// Iterate over the symlink map for every link that is found this ensures
+	// that all symlinks that can be created will be created and any that are
+	// left over are cyclically dependent
+	maxIterations := len(symlinkMap)
+	for i := 0; i < maxIterations; i++ {
 		for path, linkname := range symlinkMap {
 			// Check to see if the linkname lies on the path of another symlink in
 			// the table or if it is another symlink in the table
@@ -154,11 +154,18 @@ func (z ZipArchive) Decompress(destination string) error {
 			//
 			// If there is a match either of the symlink or it is on the path then
 			// skip the creation of this symlink for now
-			sln := strings.Split(linkname, "/")
-			for i := 0; i < len(sln); i++ {
-				if _, ok := symlinkMap[linknameFullPath(path, filepath.Join(sln[:i+1]...))]; ok {
-					continue Builder
+			shouldSkipLink := func() bool {
+				sln := strings.Split(linkname, "/")
+				for j := 0; j < len(sln); j++ {
+					if _, ok := symlinkMap[linknameFullPath(path, filepath.Join(sln[:j+1]...))]; ok {
+						return true
+					}
 				}
+				return false
+			}
+
+			if shouldSkipLink() {
+				continue
 			}
 
 			// If the linkname is not an existing link in the symlink table then we
@@ -182,6 +189,12 @@ func (z ZipArchive) Decompress(destination string) error {
 		}
 	}
 
+	// Check to see if there are any symlinks left in the map which would
+	// indicate a cyclical dependency
+	if len(symlinkMap) > 0 {
+		return fmt.Errorf("failed: max iterations reached: this symlink graph contains a cycle")
+	}
+
 	return nil
 }
 

vacation/zip_archive_test.go

@@ -244,6 +244,33 @@ func testZipArchive(t *testing.T, context spec.G, it spec.S) {
 				})
 			})
 
+			context("when it fails to unzip a file", func() {
+				var buffer *bytes.Buffer
+				it.Before(func() {
+					var err error
+					buffer = bytes.NewBuffer(nil)
+					zw := zip.NewWriter(buffer)
+
+					_, err = zw.Create("some-file")
+					Expect(err).NotTo(HaveOccurred())
+
+					Expect(zw.Close()).To(Succeed())
+
+					Expect(os.Chmod(tempDir, 0000)).To(Succeed())
+				})
+
+				it.After(func() {
+					Expect(os.Chmod(tempDir, os.ModePerm)).To(Succeed())
+				})
+
+				it("returns an error", func() {
+					readyArchive := vacation.NewZipArchive(buffer)
+
+					err := readyArchive.Decompress(tempDir)
+					Expect(err).To(MatchError(ContainSubstring("failed to unzip file")))
+				})
+			})
+
 			context("when it tries to symlink to a file that does not exist", func() {
 				var buffer *bytes.Buffer
 				it.Before(func() {
@@ -305,30 +332,39 @@ func testZipArchive(t *testing.T, context spec.G, it spec.S) {
 				})
 			})
 
-			context("when it fails to unzip a file", func() {
+			context("when there is a symlink cycle", func() {
 				var buffer *bytes.Buffer
 				it.Before(func() {
 					var err error
 					buffer = bytes.NewBuffer(nil)
 					zw := zip.NewWriter(buffer)
 
-					_, err = zw.Create("some-file")
+					header := &zip.FileHeader{Name: "a-symlink"}
+					header.SetMode(0755 | os.ModeSymlink)
+
+					aSymlink, err := zw.CreateHeader(header)
 					Expect(err).NotTo(HaveOccurred())
 
-					Expect(zw.Close()).To(Succeed())
+					_, err = aSymlink.Write([]byte(filepath.Join("b-symlink")))
+					Expect(err).NotTo(HaveOccurred())
 
-					Expect(os.Chmod(tempDir, 0000)).To(Succeed())
-				})
+					header = &zip.FileHeader{Name: "b-symlink"}
+					header.SetMode(0755 | os.ModeSymlink)
 
-				it.After(func() {
-					Expect(os.Chmod(tempDir, os.ModePerm)).To(Succeed())
+					bSymlink, err := zw.CreateHeader(header)
+					Expect(err).NotTo(HaveOccurred())
+
+					_, err = bSymlink.Write([]byte(filepath.Join("a-symlink")))
+					Expect(err).NotTo(HaveOccurred())
+
+					Expect(zw.Close()).To(Succeed())
 				})
 
 				it("returns an error", func() {
 					readyArchive := vacation.NewZipArchive(buffer)
 
 					err := readyArchive.Decompress(tempDir)
-					Expect(err).To(MatchError(ContainSubstring("failed to unzip file")))
+					Expect(err).To(MatchError("failed: max iterations reached: this symlink graph contains a cycle"))
 				})
 			})
 		})