From ecf520ba9f1bfca4853bd4fbc5a756d0505cc4d6 Mon Sep 17 00:00:00 2001
From: Alex Suraci <suraci.alex@gmail.com>
Date: Tue, 7 Aug 2018 04:24:37 +0000
Subject: [PATCH] fix hardlink behavior when extracting layers

extracting in reverse order is more optimal, but it makes supporting
hardlinks very difficult. the 'concourse/git-resource' image didn't work
because a hardlinked file was removed in a later layer, making it
impossible to link to in the earlier layer.

it may be worth investigating another solution; it's really nice to not
have to write files that are later removed.
---
 cmd/in/unpack.go             | 61 ++++++++++++++----------------------
 in_test.go                   | 17 ++++++++++
 testdata/symlinks/Dockerfile |  3 ++
 3 files changed, 44 insertions(+), 37 deletions(-)
 create mode 100644 testdata/symlinks/Dockerfile

diff --git a/cmd/in/unpack.go b/cmd/in/unpack.go
index 50c311e..9df86b7 100644
--- a/cmd/in/unpack.go
+++ b/cmd/in/unpack.go
@@ -25,9 +25,6 @@ func unpackImage(dest string, img v1.Image, debug bool) error {
 		return err
 	}
 
-	written := map[string]struct{}{}
-	removed := map[string]struct{}{}
-
 	chown := os.Getuid() == 0
 
 	var out io.Writer
@@ -61,10 +58,10 @@ func unpackImage(dest string, img v1.Image, debug bool) error {
 
 	// iterate over layers in reverse order; no need to write things files that
 	// are modified by later layers anyway
-	for i := len(layers) - 1; i >= 0; i-- {
+	for i, layer := range layers {
 		logrus.Debugf("extracting layer %d of %d", i+1, len(layers))
 
-		err := extractLayer(dest, layers[i], bars[i], written, removed, chown)
+		err := extractLayer(dest, layer, bars[i], chown)
 		if err != nil {
 			return err
 		}
@@ -75,7 +72,7 @@ func unpackImage(dest string, img v1.Image, debug bool) error {
 	return nil
 }
 
-func extractLayer(dest string, layer v1.Layer, bar *mpb.Bar, written, removed map[string]struct{}, chown bool) error {
+func extractLayer(dest string, layer v1.Layer, bar *mpb.Bar, chown bool) error {
 	r, err := layer.Compressed()
 	if err != nil {
 		return err
@@ -102,38 +99,43 @@ func extractLayer(dest string, layer v1.Layer, bar *mpb.Bar, written, removed ma
 		base := filepath.Base(path)
 		dir := filepath.Dir(path)
 
-		logrus.Debugf("unpacking %s", hdr.Name)
+		log := logrus.WithFields(logrus.Fields{
+			"Name": hdr.Name,
+		})
+
+		log.Debug("unpacking")
 
 		if strings.HasPrefix(base, whiteoutPrefix) {
 			// layer has marked a file as deleted
 			name := strings.TrimPrefix(base, whiteoutPrefix)
 			removedPath := filepath.Join(dir, name)
-			removed[removedPath] = struct{}{}
-			logrus.Debugf("whiting out %s", removedPath)
-			continue
-		}
 
-		if pathIsRemoved(path, removed) {
-			// path has been removed by lower layer
-			logrus.Debugf("skipping removed path %s", path)
-			continue
-		}
+			log.Debugf("removing %s", removedPath)
+
+			err := os.RemoveAll(removedPath)
+			if err != nil {
+				return nil
+			}
 
-		if _, ok := written[path]; ok {
-			// path has already been written by lower layer
-			logrus.Debugf("skipping already-written file %s", path)
 			continue
 		}
 
-		written[path] = struct{}{}
-
 		if hdr.Typeflag == tar.TypeBlock || hdr.Typeflag == tar.TypeChar {
 			// devices can't be created in a user namespace
-			logrus.Debugf("skipping device %s", hdr.Name)
+			log.Debugf("skipping device %s", hdr.Name)
 			continue
 		}
 
+		if hdr.Typeflag == tar.TypeSymlink {
+			log.Warnf("symlinking to %s", hdr.Linkname)
+		}
+
+		if hdr.Typeflag == tar.TypeLink {
+			log.Warnf("hardlinking to %s", hdr.Linkname)
+		}
+
 		if err := tarfs.ExtractEntry(hdr, dest, tr, chown); err != nil {
+			log.Infof("extracting")
 			return err
 		}
 	}
@@ -150,18 +152,3 @@ func extractLayer(dest string, layer v1.Layer, bar *mpb.Bar, written, removed ma
 
 	return nil
 }
-
-func pathIsRemoved(path string, removed map[string]struct{}) bool {
-	if _, ok := removed[path]; ok {
-		return true
-	}
-
-	// check if parent dir has been removed
-	for wd := range removed {
-		if strings.HasPrefix(path, wd) {
-			return true
-		}
-	}
-
-	return false
-}
diff --git a/in_test.go b/in_test.go
index ddb200b..86f2952 100644
--- a/in_test.go
+++ b/in_test.go
@@ -150,4 +150,21 @@ var _ = Describe("In", func() {
 			Expect(err).To(HaveOccurred())
 		})
 	})
+
+	Describe("a hardlink that is later removed", func() {
+		BeforeEach(func() {
+			req.Source.Repository = "concourse/test-image-symlinks"
+			req.Version.Digest = latestDigest(req.Source.Repository)
+		})
+
+		It("works", func() {
+			lstat, err := os.Lstat(rootfsPath("usr", "libexec", "git-core", "git"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(lstat.Mode() & os.ModeSymlink).To(BeZero())
+
+			stat, err := os.Stat(rootfsPath("usr", "libexec", "git-core", "git"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(stat.Mode() & os.ModeSymlink).To(BeZero())
+		})
+	})
 })
diff --git a/testdata/symlinks/Dockerfile b/testdata/symlinks/Dockerfile
new file mode 100644
index 0000000..aa44904
--- /dev/null
+++ b/testdata/symlinks/Dockerfile
@@ -0,0 +1,3 @@
+FROM alpine
+RUN apk --no-cache add git
+RUN rm /usr/bin/git