Merge pull request #17710 from tonistiigi/fix-builder-symlinks

Fix symlink handling in builder ADD/COPY commands

builder/builder.go

@@ -33,7 +33,8 @@ type Context interface {
 	Close() error
 	// Stat returns an entry corresponding to path if any.
 	// It is recommended to return an error if path was not found.
-	Stat(path string) (FileInfo, error)
+	// If path is a symlink it also returns the path to the target file.
+	Stat(path string) (string, FileInfo, error)
 	// Open opens path from the context and returns a readable stream of it.
 	Open(path string) (io.ReadCloser, error)
 	// Walk walks the tree of the context with the function passed to it.
@@ -64,6 +65,8 @@ type PathFileInfo struct {
 	os.FileInfo
 	// FilePath holds the absolute path to the file.
 	FilePath string
+	// Name holds the basename for the file.
+	FileName string
 }
 
 // Path returns the absolute path to the file.
@@ -71,6 +74,14 @@ func (fi PathFileInfo) Path() string {
 	return fi.FilePath
 }
 
+// Name returns the basename of the file.
+func (fi PathFileInfo) Name() string {
+	if fi.FileName != "" {
+		return fi.FileName
+	}
+	return fi.FileInfo.Name()
+}
+
 // Hashed defines an extra method intended for implementations of os.FileInfo.
 type Hashed interface {
 	// Hash returns the hash of a file.

builder/dockerfile/internals.go

@@ -366,7 +366,7 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression
 
 	// Must be a dir or a file
 
-	fi, err := b.context.Stat(origPath)
+	statPath, fi, err := b.context.Stat(origPath)
 	if err != nil {
 		return nil, err
 	}
@@ -383,11 +383,9 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression
 		hfi.SetHash("file:" + hfi.Hash())
 		return copyInfos, nil
 	}
-
 	// Must be a dir
-
 	var subfiles []string
-	b.context.Walk(origPath, func(path string, info builder.FileInfo, err error) error {
+	err = b.context.Walk(statPath, func(path string, info builder.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
@@ -395,6 +393,9 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression
 		subfiles = append(subfiles, info.(builder.Hashed).Hash())
 		return nil
 	})
+	if err != nil {
+		return nil, err
+	}
 
 	sort.Strings(subfiles)
 	hasher := sha256.New()
@@ -613,9 +614,9 @@ func (b *Builder) readDockerfile() error {
 	// back to 'Dockerfile' and use that in the error message.
 	if b.DockerfileName == "" {
 		b.DockerfileName = api.DefaultDockerfileName
-		if _, err := b.context.Stat(b.DockerfileName); os.IsNotExist(err) {
+		if _, _, err := b.context.Stat(b.DockerfileName); os.IsNotExist(err) {
 			lowercase := strings.ToLower(b.DockerfileName)
-			if _, err := b.context.Stat(lowercase); err == nil {
+			if _, _, err := b.context.Stat(lowercase); err == nil {
 				b.DockerfileName = lowercase
 			}
 		}

builder/tarsum.go

@@ -5,7 +5,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"strings"
 
 	"github.com/docker/docker/pkg/archive"
 	"github.com/docker/docker/pkg/chrootarchive"
@@ -43,26 +42,32 @@ func (c *tarSumContext) Open(path string) (io.ReadCloser, error) {
 	return r, nil
 }
 
-func (c *tarSumContext) Stat(path string) (fi FileInfo, err error) {
+func (c *tarSumContext) Stat(path string) (string, FileInfo, error) {
 	cleanpath, fullpath, err := c.normalize(path)
 	if err != nil {
-		return nil, err
+		return "", nil, err
 	}
 
 	st, err := os.Lstat(fullpath)
 	if err != nil {
-		return nil, convertPathError(err, cleanpath)
+		return "", nil, convertPathError(err, cleanpath)
 	}
 
-	fi = PathFileInfo{st, fullpath}
-	// we set sum to path by default for the case where GetFile returns nil.
-	// The usual case is if cleanpath is empty.
+	rel, err := filepath.Rel(c.root, fullpath)
+	if err != nil {
+		return "", nil, convertPathError(err, cleanpath)
+	}
+
+	// We set sum to path by default for the case where GetFile returns nil.
+	// The usual case is if relative path is empty.
 	sum := path
-	if tsInfo := c.sums.GetFile(cleanpath); tsInfo != nil {
+	// Use the checksum of the followed path(not the possible symlink) because
+	// this is the file that is actually copied.
+	if tsInfo := c.sums.GetFile(rel); tsInfo != nil {
 		sum = tsInfo.Sum()
 	}
-	fi = &HashedFileInfo{fi, sum}
-	return fi, nil
+	fi := &HashedFileInfo{PathFileInfo{st, fullpath, filepath.Base(cleanpath)}, sum}
+	return rel, fi, nil
 }
 
 // MakeTarSumContext returns a build Context from a tar stream.
@@ -114,7 +119,7 @@ func (c *tarSumContext) normalize(path string) (cleanpath, fullpath string, err
 	if err != nil {
 		return "", "", fmt.Errorf("Forbidden path outside the build context: %s (%s)", path, fullpath)
 	}
-	_, err = os.Stat(fullpath)
+	_, err = os.Lstat(fullpath)
 	if err != nil {
 		return "", "", convertPathError(err, path)
 	}
@@ -122,38 +127,26 @@ func (c *tarSumContext) normalize(path string) (cleanpath, fullpath string, err
 }
 
 func (c *tarSumContext) Walk(root string, walkFn WalkFunc) error {
-	for _, tsInfo := range c.sums {
-		path := tsInfo.Name()
-		path, fullpath, err := c.normalize(path)
+	root = filepath.Join(c.root, filepath.Join(string(filepath.Separator), root))
+	return filepath.Walk(root, func(fullpath string, info os.FileInfo, err error) error {
+		rel, err := filepath.Rel(c.root, fullpath)
 		if err != nil {
 			return err
 		}
-
-		// Any file in the context that starts with the given path will be
-		// picked up and its hashcode used.  However, we'll exclude the
-		// root dir itself.  We do this for a coupel of reasons:
-		// 1 - ADD/COPY will not copy the dir itself, just its children
-		//     so there's no reason to include it in the hash calc
-		// 2 - the metadata on the dir will change when any child file
-		//     changes.  This will lead to a miss in the cache check if that
-		//     child file is in the .dockerignore list.
-		if rel, err := filepath.Rel(root, path); err != nil {
-			return err
-		} else if rel == "." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) {
-			continue
+		if rel == "." {
+			return nil
 		}
 
-		info, err := os.Lstat(fullpath)
-		if err != nil {
-			return convertPathError(err, path)
+		sum := rel
+		if tsInfo := c.sums.GetFile(rel); tsInfo != nil {
+			sum = tsInfo.Sum()
 		}
-		// TODO check context breakout?
-		fi := &HashedFileInfo{PathFileInfo{info, fullpath}, tsInfo.Sum()}
-		if err := walkFn(path, fi, nil); err != nil {
+		fi := &HashedFileInfo{PathFileInfo{FileInfo: info, FilePath: fullpath}, sum}
+		if err := walkFn(rel, fi, nil); err != nil {
 			return err
 		}
-	}
-	return nil
+		return nil
+	})
 }
 
 func (c *tarSumContext) Remove(path string) error {

daemon/daemonbuilder/builder.go

@@ -183,7 +183,7 @@ func (d Docker) Copy(c *daemon.Container, destPath string, src builder.FileInfo,
 
 	// only needed for fixPermissions, but might as well put it before CopyFileWithTar
 	if destExists && destStat.IsDir() {
-		destPath = filepath.Join(destPath, filepath.Base(srcPath))
+		destPath = filepath.Join(destPath, src.Name())
 	}
 
 	if err := idtools.MkdirAllNewAs(filepath.Dir(destPath), 0755, rootUID, rootGID); err != nil {

integration-cli/docker_cli_build_test.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/docker/docker/builder/dockerfile/command"
 	"github.com/docker/docker/pkg/archive"
+	"github.com/docker/docker/pkg/integration/checker"
 	"github.com/docker/docker/pkg/stringutils"
 	"github.com/go-check/check"
 )
@@ -6276,3 +6277,127 @@ func (s *DockerSuite) TestBuildMultipleTags(c *check.C) {
 	c.Assert(err, check.IsNil)
 	c.Assert(id1, check.Equals, id2)
 }
+
+// #17290
+func (s *DockerSuite) TestBuildCacheBrokenSymlink(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+	name := "testbuildbrokensymlink"
+	ctx, err := fakeContext(`
+	FROM busybox
+	COPY . ./`,
+		map[string]string{
+			"foo": "bar",
+		})
+	c.Assert(err, checker.IsNil)
+	defer ctx.Close()
+
+	err = os.Symlink(filepath.Join(ctx.Dir, "nosuchfile"), filepath.Join(ctx.Dir, "asymlink"))
+	c.Assert(err, checker.IsNil)
+
+	// warm up cache
+	_, err = buildImageFromContext(name, ctx, true)
+	c.Assert(err, checker.IsNil)
+
+	// add new file to context, should invalidate cache
+	err = ioutil.WriteFile(filepath.Join(ctx.Dir, "newfile"), []byte("foo"), 0644)
+	c.Assert(err, checker.IsNil)
+
+	_, out, err := buildImageFromContextWithOut(name, ctx, true)
+	c.Assert(err, checker.IsNil)
+
+	c.Assert(out, checker.Not(checker.Contains), "Using cache")
+
+}
+
+func (s *DockerSuite) TestBuildFollowSymlinkToFile(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+	name := "testbuildbrokensymlink"
+	ctx, err := fakeContext(`
+	FROM busybox
+	COPY asymlink target`,
+		map[string]string{
+			"foo": "bar",
+		})
+	c.Assert(err, checker.IsNil)
+	defer ctx.Close()
+
+	err = os.Symlink("foo", filepath.Join(ctx.Dir, "asymlink"))
+	c.Assert(err, checker.IsNil)
+
+	id, err := buildImageFromContext(name, ctx, true)
+	c.Assert(err, checker.IsNil)
+
+	out, _ := dockerCmd(c, "run", "--rm", id, "cat", "target")
+	c.Assert(out, checker.Matches, "bar")
+
+	// change target file should invalidate cache
+	err = ioutil.WriteFile(filepath.Join(ctx.Dir, "foo"), []byte("baz"), 0644)
+	c.Assert(err, checker.IsNil)
+
+	id, out, err = buildImageFromContextWithOut(name, ctx, true)
+	c.Assert(err, checker.IsNil)
+	c.Assert(out, checker.Not(checker.Contains), "Using cache")
+
+	out, _ = dockerCmd(c, "run", "--rm", id, "cat", "target")
+	c.Assert(out, checker.Matches, "baz")
+}
+
+func (s *DockerSuite) TestBuildFollowSymlinkToDir(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+	name := "testbuildbrokensymlink"
+	ctx, err := fakeContext(`
+	FROM busybox
+	COPY asymlink /`,
+		map[string]string{
+			"foo/abc": "bar",
+			"foo/def": "baz",
+		})
+	c.Assert(err, checker.IsNil)
+	defer ctx.Close()
+
+	err = os.Symlink("foo", filepath.Join(ctx.Dir, "asymlink"))
+	c.Assert(err, checker.IsNil)
+
+	id, err := buildImageFromContext(name, ctx, true)
+	c.Assert(err, checker.IsNil)
+
+	out, _ := dockerCmd(c, "run", "--rm", id, "cat", "abc", "def")
+	c.Assert(out, checker.Matches, "barbaz")
+
+	// change target file should invalidate cache
+	err = ioutil.WriteFile(filepath.Join(ctx.Dir, "foo/def"), []byte("bax"), 0644)
+	c.Assert(err, checker.IsNil)
+
+	id, out, err = buildImageFromContextWithOut(name, ctx, true)
+	c.Assert(err, checker.IsNil)
+	c.Assert(out, checker.Not(checker.Contains), "Using cache")
+
+	out, _ = dockerCmd(c, "run", "--rm", id, "cat", "abc", "def")
+	c.Assert(out, checker.Matches, "barbax")
+
+}
+
+// TestBuildSymlinkBasename tests that target file gets basename from symlink,
+// not from the target file.
+func (s *DockerSuite) TestBuildSymlinkBasename(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+	name := "testbuildbrokensymlink"
+	ctx, err := fakeContext(`
+	FROM busybox
+	COPY asymlink /`,
+		map[string]string{
+			"foo": "bar",
+		})
+	c.Assert(err, checker.IsNil)
+	defer ctx.Close()
+
+	err = os.Symlink("foo", filepath.Join(ctx.Dir, "asymlink"))
+	c.Assert(err, checker.IsNil)
+
+	id, err := buildImageFromContext(name, ctx, true)
+	c.Assert(err, checker.IsNil)
+
+	out, _ := dockerCmd(c, "run", "--rm", id, "cat", "asymlink")
+	c.Assert(out, checker.Matches, "bar")
+
+}

integration-cli/docker_utils.go

@@ -1251,6 +1251,14 @@ func buildImage(name, dockerfile string, useCache bool, buildFlags ...string) (s
 }
 
 func buildImageFromContext(name string, ctx *FakeContext, useCache bool, buildFlags ...string) (string, error) {
+	id, _, err := buildImageFromContextWithOut(name, ctx, useCache, buildFlags...)
+	if err != nil {
+		return "", err
+	}
+	return id, nil
+}
+
+func buildImageFromContextWithOut(name string, ctx *FakeContext, useCache bool, buildFlags ...string) (string, string, error) {
 	args := []string{"build", "-t", name}
 	if !useCache {
 		args = append(args, "--no-cache")
@@ -1261,9 +1269,13 @@ func buildImageFromContext(name string, ctx *FakeContext, useCache bool, buildFl
 	buildCmd.Dir = ctx.Dir
 	out, exitCode, err := runCommandWithOutput(buildCmd)
 	if err != nil || exitCode != 0 {
-		return "", fmt.Errorf("failed to build the image: %s", out)
+		return "", "", fmt.Errorf("failed to build the image: %s", out)
 	}
-	return getIDByName(name)
+	id, err := getIDByName(name)
+	if err != nil {
+		return "", "", err
+	}
+	return id, out, nil
 }
 
 func buildImageFromPath(name, path string, useCache bool, buildFlags ...string) (string, error) {