Fix docker cp Behavior With Symlinks

[pkg/archive] Update archive/copy path handling

  - Remove unused TarOptions.Name field.
  - Add new TarOptions.RebaseNames field.
  - Update some of the logic around path dir/base splitting.
  - Update some of the logic behind archive entry name rebasing.

[api/types] Add LinkTarget field to PathStat

[daemon] Fix stat, archive, extract of symlinks

  These operations *should* resolve symlinks that are in the path but if the
  resource itself is a symlink then it *should not* be resolved. This patch
  puts this logic into a common function `resolvePath` which resolves symlinks
  of the path's dir in scope of the container rootfs but does not resolve the
  final element of the path. Now archive, extract, and stat operations will
  return symlinks if the path is indeed a symlink.

[api/client] Update cp path hanling

[docs/reference/api] Update description of stat

  Add the linkTarget field to the header of the archive endpoint.
  Remove path field.

[integration-cli] Fix/Add cp symlink test cases

  Copying a symlink should do just that: copy the symlink NOT
  copy the target of the symlink. Also, the resulting file from
  the copy should have the name of the symlink NOT the name of
  the target file.

  Copying to a symlink should copy to the symlink target and not
  modify the symlink itself.

Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)

archive/archive.go

@@ -37,11 +37,13 @@ type (
 		Compression      Compression
 		NoLchown         bool
 		ChownOpts        *TarChownOptions
-		Name             string
 		IncludeSourceDir bool
 		// When unpacking, specifies whether overwriting a directory with a
 		// non-directory is allowed and vice versa.
 		NoOverwriteDirNonDir bool
+		// For each include when creating an archive, the included name will be
+		// replaced with the matching name from this map.
+		RebaseNames map[string]string
 	}
 
 	// Archiver allows the reuse of most utility functions of this package
@@ -454,8 +456,9 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error)
 
 		seen := make(map[string]bool)
 
-		var renamedRelFilePath string // For when tar.Options.Name is set
 		for _, include := range options.IncludeFiles {
+			rebaseName := options.RebaseNames[include]
+
 			// We can't use filepath.Join(srcPath, include) because this will
 			// clean away a trailing "." or "/" which may be important.
 			walkRoot := strings.Join([]string{srcPath, include}, string(filepath.Separator))
@@ -503,14 +506,17 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error)
 				}
 				seen[relFilePath] = true
 
-				// TODO Windows: Verify if this needs to be os.Pathseparator
+				// Rename the base resource.
-				// Rename the base resource
+				if rebaseName != "" {
-				if options.Name != "" && filePath == srcPath+"/"+filepath.Base(relFilePath) {
+					var replacement string
-					renamedRelFilePath = relFilePath
+					if rebaseName != string(filepath.Separator) {
-				}
+						// Special case the root directory to replace with an
-				// Set this to make sure the items underneath also get renamed
+						// empty string instead so that we don't end up with
-				if options.Name != "" {
+						// double slashes in the paths.
-					relFilePath = strings.Replace(relFilePath, renamedRelFilePath, options.Name, 1)
+						replacement = rebaseName
+					}
+
+					relFilePath = strings.Replace(relFilePath, include, replacement, 1)
 				}
 
 				if err := ta.addTarFile(filePath, relFilePath); err != nil {

archive/archive_test.go

@@ -695,7 +695,7 @@ func TestTarWithOptions(t *testing.T) {
 		{&TarOptions{ExcludePatterns: []string{"2"}}, 1},
 		{&TarOptions{ExcludePatterns: []string{"1", "folder*"}}, 2},
 		{&TarOptions{IncludeFiles: []string{"1", "1"}}, 2},
-		{&TarOptions{Name: "test", IncludeFiles: []string{"1"}}, 4},
+		{&TarOptions{IncludeFiles: []string{"1"}, RebaseNames: map[string]string{"1": "test"}}, 4},
 	}
 	for _, testCase := range cases {
 		changes, err := tarUntar(t, origin, testCase.opts)

archive/copy.go

@@ -6,7 +6,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 
@@ -64,34 +63,33 @@ func SpecifiesCurrentDir(path string) bool {
 	return filepath.Base(path) == "."
 }
 
-// SplitPathDirEntry splits the given path between its
+// SplitPathDirEntry splits the given path between its directory name and its
-// parent directory and its basename in that directory.
+// basename by first cleaning the path but preserves a trailing "." if the
-func SplitPathDirEntry(localizedPath string) (dir, base string) {
+// original path specified the current directory.
-	normalizedPath := filepath.ToSlash(localizedPath)
+func SplitPathDirEntry(path string) (dir, base string) {
-	vol := filepath.VolumeName(normalizedPath)
+	cleanedPath := filepath.Clean(path)
-	normalizedPath = normalizedPath[len(vol):]
 
-	if normalizedPath == "/" {
+	if SpecifiesCurrentDir(path) {
-		// Specifies the root path.
+		cleanedPath += string(filepath.Separator) + "."
-		return filepath.FromSlash(vol + normalizedPath), "."
 	}
 
-	trimmedPath := vol + strings.TrimRight(normalizedPath, "/")
+	return filepath.Dir(cleanedPath), filepath.Base(cleanedPath)
-
-	dir = filepath.FromSlash(path.Dir(trimmedPath))
-	base = filepath.FromSlash(path.Base(trimmedPath))
-
-	return dir, base
 }
 
-// TarResource archives the resource at the given sourcePath into a Tar
+// TarResource archives the resource described by the given CopyInfo to a Tar
 // archive. A non-nil error is returned if sourcePath does not exist or is
 // asserted to be a directory but exists as another type of file.
 //
 // This function acts as a convenient wrapper around TarWithOptions, which
 // requires a directory as the source path. TarResource accepts either a
 // directory or a file path and correctly sets the Tar options.
-func TarResource(sourcePath string) (content Archive, err error) {
+func TarResource(sourceInfo CopyInfo) (content Archive, err error) {
+	return TarResourceRebase(sourceInfo.Path, sourceInfo.RebaseName)
+}
+
+// TarResourceRebase is like TarResource but renames the first path element of
+// items in the resulting tar archive to match the given rebaseName if not "".
+func TarResourceRebase(sourcePath, rebaseName string) (content Archive, err error) {
 	if _, err = os.Lstat(sourcePath); err != nil {
 		// Catches the case where the source does not exist or is not a
 		// directory if asserted to be a directory, as this also causes an
@@ -99,22 +97,6 @@ func TarResource(sourcePath string) (content Archive, err error) {
 		return
 	}
 
-	if len(sourcePath) > 1 && HasTrailingPathSeparator(sourcePath) {
-		// In the case where the source path is a symbolic link AND it ends
-		// with a path separator, we will want to evaluate the symbolic link.
-		trimmedPath := sourcePath[:len(sourcePath)-1]
-		stat, err := os.Lstat(trimmedPath)
-		if err != nil {
-			return nil, err
-		}
-
-		if stat.Mode()&os.ModeSymlink != 0 {
-			if sourcePath, err = filepath.EvalSymlinks(trimmedPath); err != nil {
-				return nil, err
-			}
-		}
-	}
-
 	// Separate the source path between it's directory and
 	// the entry in that directory which we are archiving.
 	sourceDir, sourceBase := SplitPathDirEntry(sourcePath)
@@ -127,32 +109,137 @@ func TarResource(sourcePath string) (content Archive, err error) {
 		Compression:      Uncompressed,
 		IncludeFiles:     filter,
 		IncludeSourceDir: true,
+		RebaseNames: map[string]string{
+			sourceBase: rebaseName,
+		},
 	})
 }
 
 // CopyInfo holds basic info about the source
 // or destination path of a copy operation.
 type CopyInfo struct {
-	Path   string
+	Path       string
-	Exists bool
+	Exists     bool
-	IsDir  bool
+	IsDir      bool
+	RebaseName string
 }
 
-// CopyInfoStatPath stats the given path to create a CopyInfo
+// CopyInfoSourcePath stats the given path to create a CopyInfo
-// struct representing that resource. If mustExist is true, then
+// struct representing that resource for the source of an archive copy
-// it is an error if there is no file or directory at the given path.
+// operation. The given path should be an absolute local path. A source path
-func CopyInfoStatPath(path string, mustExist bool) (CopyInfo, error) {
+// has all symlinks evaluated that appear before the last path separator ("/"
-	pathInfo := CopyInfo{Path: path}
+// on Unix). As it is to be a copy source, the path must exist.
+func CopyInfoSourcePath(path string) (CopyInfo, error) {
+	// Split the given path into its Directory and Base components. We will
+	// evaluate symlinks in the directory component then append the base.
+	dirPath, basePath := filepath.Split(path)
 
-	fileInfo, err := os.Lstat(path)
+	resolvedDirPath, err := filepath.EvalSymlinks(dirPath)
-
+	if err != nil {
-	if err == nil {
+		return CopyInfo{}, err
-		pathInfo.Exists, pathInfo.IsDir = true, fileInfo.IsDir()
-	} else if os.IsNotExist(err) && !mustExist {
-		err = nil
 	}
 
-	return pathInfo, err
+	// resolvedDirPath will have been cleaned (no trailing path separators) so
+	// we can manually join it with the base path element.
+	resolvedPath := resolvedDirPath + string(filepath.Separator) + basePath
+
+	var rebaseName string
+	if HasTrailingPathSeparator(path) && filepath.Base(path) != filepath.Base(resolvedPath) {
+		// In the case where the path had a trailing separator and a symlink
+		// evaluation has changed the last path component, we will need to
+		// rebase the name in the archive that is being copied to match the
+		// originally requested name.
+		rebaseName = filepath.Base(path)
+	}
+
+	stat, err := os.Lstat(resolvedPath)
+	if err != nil {
+		return CopyInfo{}, err
+	}
+
+	return CopyInfo{
+		Path:       resolvedPath,
+		Exists:     true,
+		IsDir:      stat.IsDir(),
+		RebaseName: rebaseName,
+	}, nil
+}
+
+// CopyInfoDestinationPath stats the given path to create a CopyInfo
+// struct representing that resource for the destination of an archive copy
+// operation. The given path should be an absolute local path.
+func CopyInfoDestinationPath(path string) (info CopyInfo, err error) {
+	maxSymlinkIter := 10 // filepath.EvalSymlinks uses 255, but 10 already seems like a lot.
+	originalPath := path
+
+	stat, err := os.Lstat(path)
+
+	if err == nil && stat.Mode()&os.ModeSymlink == 0 {
+		// The path exists and is not a symlink.
+		return CopyInfo{
+			Path:   path,
+			Exists: true,
+			IsDir:  stat.IsDir(),
+		}, nil
+	}
+
+	// While the path is a symlink.
+	for n := 0; err == nil && stat.Mode()&os.ModeSymlink != 0; n++ {
+		if n > maxSymlinkIter {
+			// Don't follow symlinks more than this arbitrary number of times.
+			return CopyInfo{}, errors.New("too many symlinks in " + originalPath)
+		}
+
+		// The path is a symbolic link. We need to evaluate it so that the
+		// destination of the copy operation is the link target and not the
+		// link itself. This is notably different than CopyInfoSourcePath which
+		// only evaluates symlinks before the last appearing path separator.
+		// Also note that it is okay if the last path element is a broken
+		// symlink as the copy operation should create the target.
+		var linkTarget string
+
+		linkTarget, err = os.Readlink(path)
+		if err != nil {
+			return CopyInfo{}, err
+		}
+
+		if !filepath.IsAbs(linkTarget) {
+			// Join with the parent directory.
+			dstParent, _ := SplitPathDirEntry(path)
+			linkTarget = filepath.Join(dstParent, linkTarget)
+		}
+
+		path = linkTarget
+		stat, err = os.Lstat(path)
+	}
+
+	if err != nil {
+		// It's okay if the destination path doesn't exist. We can still
+		// continue the copy operation if the parent directory exists.
+		if !os.IsNotExist(err) {
+			return CopyInfo{}, err
+		}
+
+		// Ensure destination parent dir exists.
+		dstParent, _ := SplitPathDirEntry(path)
+
+		parentDirStat, err := os.Lstat(dstParent)
+		if err != nil {
+			return CopyInfo{}, err
+		}
+		if !parentDirStat.IsDir() {
+			return CopyInfo{}, ErrNotDirectory
+		}
+
+		return CopyInfo{Path: path}, nil
+	}
+
+	// The path exists after resolving symlinks.
+	return CopyInfo{
+		Path:   path,
+		Exists: true,
+		IsDir:  stat.IsDir(),
+	}, nil
 }
 
 // PrepareArchiveCopy prepares the given srcContent archive, which should
@@ -210,6 +297,13 @@ func PrepareArchiveCopy(srcContent ArchiveReader, srcInfo, dstInfo CopyInfo) (ds
 // rebaseArchiveEntries rewrites the given srcContent archive replacing
 // an occurance of oldBase with newBase at the beginning of entry names.
 func rebaseArchiveEntries(srcContent ArchiveReader, oldBase, newBase string) Archive {
+	if oldBase == "/" {
+		// If oldBase specifies the root directory, use an empty string as
+		// oldBase instead so that newBase doesn't replace the path separator
+		// that all paths will start with.
+		oldBase = ""
+	}
+
 	rebased, w := io.Pipe()
 
 	go func() {
@@ -259,11 +353,11 @@ func CopyResource(srcPath, dstPath string) error {
 	srcPath = PreserveTrailingDotOrSeparator(filepath.Clean(srcPath), srcPath)
 	dstPath = PreserveTrailingDotOrSeparator(filepath.Clean(dstPath), dstPath)
 
-	if srcInfo, err = CopyInfoStatPath(srcPath, true); err != nil {
+	if srcInfo, err = CopyInfoSourcePath(srcPath); err != nil {
 		return err
 	}
 
-	content, err := TarResource(srcPath)
+	content, err := TarResource(srcInfo)
 	if err != nil {
 		return err
 	}
@@ -275,24 +369,13 @@ func CopyResource(srcPath, dstPath string) error {
 // CopyTo handles extracting the given content whose
 // entries should be sourced from srcInfo to dstPath.
 func CopyTo(content ArchiveReader, srcInfo CopyInfo, dstPath string) error {
-	dstInfo, err := CopyInfoStatPath(dstPath, false)
+	// The destination path need not exist, but CopyInfoDestinationPath will
+	// ensure that at least the parent directory exists.
+	dstInfo, err := CopyInfoDestinationPath(dstPath)
 	if err != nil {
 		return err
 	}
 
-	if !dstInfo.Exists {
-		// Ensure destination parent dir exists.
-		dstParent, _ := SplitPathDirEntry(dstPath)
-
-		dstStat, err := os.Lstat(dstParent)
-		if err != nil {
-			return err
-		}
-		if !dstStat.IsDir() {
-			return ErrNotDirectory
-		}
-	}
-
 	dstDir, copyArchive, err := PrepareArchiveCopy(content, srcInfo, dstInfo)
 	if err != nil {
 		return err

archive/copy_test.go

@@ -138,13 +138,7 @@ func TestCopyErrSrcNotExists(t *testing.T) {
 	tmpDirA, tmpDirB := getTestTempDirs(t)
 	defer removeAllPaths(tmpDirA, tmpDirB)
 
-	content, err := TarResource(filepath.Join(tmpDirA, "file1"))
+	if _, err := CopyInfoSourcePath(filepath.Join(tmpDirA, "file1")); !os.IsNotExist(err) {
-	if err == nil {
-		content.Close()
-		t.Fatal("expected IsNotExist error, but got nil instead")
-	}
-
-	if !os.IsNotExist(err) {
 		t.Fatalf("expected IsNotExist error, but got %T: %s", err, err)
 	}
 }
@@ -158,13 +152,7 @@ func TestCopyErrSrcNotDir(t *testing.T) {
 	// Load A with some sample files and directories.
 	createSampleDir(t, tmpDirA)
 
-	content, err := TarResource(joinTrailingSep(tmpDirA, "file1"))
+	if _, err := CopyInfoSourcePath(joinTrailingSep(tmpDirA, "file1")); !isNotDir(err) {
-	if err == nil {
-		content.Close()
-		t.Fatal("expected IsNotDir error, but got nil instead")
-	}
-
-	if !isNotDir(err) {
 		t.Fatalf("expected IsNotDir error, but got %T: %s", err, err)
 	}
 }
@@ -181,7 +169,7 @@ func TestCopyErrDstParentNotExists(t *testing.T) {
 	srcInfo := CopyInfo{Path: filepath.Join(tmpDirA, "file1"), Exists: true, IsDir: false}
 
 	// Try with a file source.
-	content, err := TarResource(srcInfo.Path)
+	content, err := TarResource(srcInfo)
 	if err != nil {
 		t.Fatalf("unexpected error %T: %s", err, err)
 	}
@@ -199,7 +187,7 @@ func TestCopyErrDstParentNotExists(t *testing.T) {
 	// Try with a directory source.
 	srcInfo = CopyInfo{Path: filepath.Join(tmpDirA, "dir1"), Exists: true, IsDir: true}
 
-	content, err = TarResource(srcInfo.Path)
+	content, err = TarResource(srcInfo)
 	if err != nil {
 		t.Fatalf("unexpected error %T: %s", err, err)
 	}
@@ -228,7 +216,7 @@ func TestCopyErrDstNotDir(t *testing.T) {
 	// Try with a file source.
 	srcInfo := CopyInfo{Path: filepath.Join(tmpDirA, "file1"), Exists: true, IsDir: false}
 
-	content, err := TarResource(srcInfo.Path)
+	content, err := TarResource(srcInfo)
 	if err != nil {
 		t.Fatalf("unexpected error %T: %s", err, err)
 	}
@@ -245,7 +233,7 @@ func TestCopyErrDstNotDir(t *testing.T) {
 	// Try with a directory source.
 	srcInfo = CopyInfo{Path: filepath.Join(tmpDirA, "dir1"), Exists: true, IsDir: true}
 
-	content, err = TarResource(srcInfo.Path)
+	content, err = TarResource(srcInfo)
 	if err != nil {
 		t.Fatalf("unexpected error %T: %s", err, err)
 	}