Fix windows path handling in podman cp

Fixes: #14862

Signed-off-by: David Negstad <[email protected]>

Author: danegsta

cmd/podman/containers/cp.go

@@ -5,6 +5,7 @@ import (
 	"io"
 	"os"
 	"os/user"
+	"path"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -147,7 +148,7 @@ func copyContainerToContainer(sourceContainer string, sourcePath string, destCon
 	sourceContainerTarget := sourceContainerInfo.LinkTarget
 	destContainerTarget := destContainerInfo.LinkTarget
 	if !destContainerInfo.IsDir {
-		destContainerTarget = filepath.Dir(destPath)
+		destContainerTarget = path.Dir(destPath)
 	}
 
 	// If we copy a directory via the "." notation and the container path
@@ -158,8 +159,8 @@ func copyContainerToContainer(sourceContainer string, sourcePath string, destCon
 	// Hence, whenever "." is the source and the destination does not
 	// exist, we copy the source's parent and let the copier package create
 	// the destination via the Rename option.
-	if destResolvedToParentDir && sourceContainerInfo.IsDir && filepath.Base(sourcePath) == "." {
-		sourceContainerTarget = filepath.Dir(sourceContainerTarget)
+	if destResolvedToParentDir && sourceContainerInfo.IsDir && path.Base(sourcePath) == "." {
+		sourceContainerTarget = path.Dir(sourceContainerTarget)
 	}
 
 	reader, writer := io.Pipe()
@@ -183,7 +184,7 @@ func copyContainerToContainer(sourceContainer string, sourcePath string, destCon
 		if (!sourceContainerInfo.IsDir && !destContainerInfo.IsDir) || destResolvedToParentDir {
 			// If we're having a file-to-file copy, make sure to
 			// rename accordingly.
-			copyOptions.Rename = map[string]string{filepath.Base(sourceContainerTarget): destContainerBaseName}
+			copyOptions.Rename = map[string]string{path.Base(sourceContainerTarget): destContainerBaseName}
 		}
 
 		copyFunc, err := registry.ContainerEngine().ContainerCopyFromArchive(registry.Context(), destContainer, destContainerTarget, reader, copyOptions)
@@ -261,8 +262,8 @@ func copyFromContainer(container string, containerPath string, hostPath string)
 	// we copy the source's parent and let the copier package create the
 	// destination via the Rename option.
 	containerTarget := containerInfo.LinkTarget
-	if resolvedToHostParentDir && containerInfo.IsDir && filepath.Base(containerTarget) == "." {
-		containerTarget = filepath.Dir(containerTarget)
+	if resolvedToHostParentDir && containerInfo.IsDir && path.Base(containerTarget) == "." {
+		containerTarget = path.Dir(containerTarget)
 	}
 
 	if !isStdout && containerInfo.IsDir && !hostInfo.IsDir {
@@ -307,7 +308,7 @@ func copyFromContainer(container string, containerPath string, hostPath string)
 		if (!containerInfo.IsDir && !hostInfo.IsDir) || resolvedToHostParentDir {
 			// If we're having a file-to-file copy, make sure to
 			// rename accordingly.
-			putOptions.Rename = map[string]string{filepath.Base(containerTarget): hostBaseName}
+			putOptions.Rename = map[string]string{path.Base(containerTarget): hostBaseName}
 		}
 		dir := hostInfo.LinkTarget
 		if !hostInfo.IsDir {
@@ -428,7 +429,11 @@ func copyToContainer(container string, containerPath string, hostPath string) er
 			// rename accordingly.
 			getOptions.Rename = map[string]string{filepath.Base(hostTarget): containerBaseName}
 		}
-		if err := buildahCopiah.Get("/", "", getOptions, []string{hostTarget}, writer); err != nil {
+
+		// On Windows, the root path needs to be <drive>:\, while otherwise
+		// it needs to be /. Combining filepath.VolumeName() + string(os.PathSeparator)
+		// gives us the correct path for the current OS.
+		if err := buildahCopiah.Get(filepath.VolumeName(hostTarget)+string(os.PathSeparator), "", getOptions, []string{hostTarget}, writer); err != nil {
 			return fmt.Errorf("copying from host: %w", err)
 		}
 		return nil
@@ -438,7 +443,7 @@ func copyToContainer(container string, containerPath string, hostPath string) er
 		defer reader.Close()
 		target := containerInfo.FileInfo.LinkTarget
 		if !containerInfo.IsDir {
-			target = filepath.Dir(target)
+			target = path.Dir(target)
 		}
 
 		copyFunc, err := registry.ContainerEngine().ContainerCopyFromArchive(registry.Context(), container, target, reader, entities.CopyOptions{Chown: chown, NoOverwriteDirNonDir: !cpOpts.OverwriteDirNonDir})
@@ -460,7 +465,7 @@ func copyToContainer(container string, containerPath string, hostPath string) er
 func resolvePathOnDestinationContainer(container string, containerPath string, isStdin bool) (baseName string, containerInfo *entities.ContainerStatReport, resolvedToParentDir bool, err error) {
 	containerInfo, err = registry.ContainerEngine().ContainerStat(registry.Context(), container, containerPath)
 	if err == nil {
-		baseName = filepath.Base(containerInfo.LinkTarget)
+		baseName = path.Base(containerInfo.LinkTarget)
 		return //nolint: nilerr
 	}
 
@@ -476,17 +481,17 @@ func resolvePathOnDestinationContainer(container string, containerPath string, i
 	// NOTE: containerInfo may actually be set.  That happens when
 	// the container path is a symlink into nirvana.  In that case,
 	// we must use the symlinked path instead.
-	path := containerPath
+	parentPath := containerPath
 	if containerInfo != nil {
-		baseName = filepath.Base(containerInfo.LinkTarget)
-		path = containerInfo.LinkTarget
+		baseName = path.Base(containerInfo.LinkTarget)
+		parentPath = containerInfo.LinkTarget
 	} else {
-		baseName = filepath.Base(containerPath)
+		baseName = path.Base(containerPath)
 	}
 
-	parentDir, err := containerParentDir(container, path)
+	parentDir, err := containerParentDir(container, parentPath)
 	if err != nil {
-		err = fmt.Errorf("could not determine parent dir of %q on container %s: %w", path, container, err)
+		err = fmt.Errorf("could not determine parent dir of %q on container %s: %w", parentPath, container, err)
 		return
 	}
 
@@ -504,8 +509,11 @@ func resolvePathOnDestinationContainer(container string, containerPath string, i
 // container.  If the path is relative, it will be resolved relative to the
 // container's working directory (or "/" if the work dir isn't set).
 func containerParentDir(container string, containerPath string) (string, error) {
-	if filepath.IsAbs(containerPath) {
-		return filepath.Dir(containerPath), nil
+	// This is specifically a path in the (linux) container, so we need to intentionally use
+	// path instead of filepath to ensure we don't try to parse container paths using the
+	// host OS conventions.
+	if path.IsAbs(containerPath) {
+		return path.Dir(containerPath), nil
 	}
 	inspectData, _, err := registry.ContainerEngine().ContainerInspect(registry.Context(), []string{container}, entities.InspectOptions{})
 	if err != nil {
@@ -514,9 +522,9 @@ func containerParentDir(container string, containerPath string) (string, error)
 	if len(inspectData) != 1 {
 		return "", fmt.Errorf("inspecting container %q: expected 1 data item but got %d", container, len(inspectData))
 	}
-	workDir := filepath.Join("/", inspectData[0].Config.WorkingDir)
-	workDir = filepath.Join(workDir, containerPath)
-	return filepath.Dir(workDir), nil
+	workDir := path.Join("/", inspectData[0].Config.WorkingDir)
+	workDir = path.Join(workDir, containerPath)
+	return path.Dir(workDir), nil
 }
 
 // validateFileInfo returns an error if the specified FileInfo doesn't point to

pkg/copy/fileinfo.go

@@ -83,23 +83,28 @@ func ResolveHostPath(path string) (*FileInfo, error) {
 // is preserved.  The filepath API among tends to clean up a bit too much but
 // we *must* preserve this data by all means.
 func PreserveBasePath(original, resolved string) string {
-	// Handle "/"
-	if strings.HasSuffix(original, "/") {
-		if !strings.HasSuffix(resolved, "/") {
-			resolved += "/"
+	// Ensure paths are in platform semantics (replace / with \ on Windows)
+	resolved = filepath.FromSlash(resolved)
+	original = filepath.FromSlash(original)
+
+	if filepath.Base(resolved) != "." && filepath.Base(original) == "." {
+		if !hasTrailingPathSeparator(resolved) {
+			// Add a separator if it doesn't already end with one (a cleaned
+			// path would only end in a separator if it is the root).
+			resolved += string(filepath.Separator)
 		}
-		return resolved
+		resolved += "."
 	}
 
-	// Handle "/."
-	if strings.HasSuffix(original, "/.") {
-		if strings.HasSuffix(resolved, "/") { // could be root!
-			resolved += "."
-		} else if !strings.HasSuffix(resolved, "/.") {
-			resolved += "/."
-		}
-		return resolved
+	if !hasTrailingPathSeparator(resolved) && hasTrailingPathSeparator(original) {
+		resolved += string(filepath.Separator)
 	}
 
 	return resolved
 }
+
+// hasTrailingPathSeparator returns whether the given
+// path ends with the system's path separator character.
+func hasTrailingPathSeparator(path string) bool {
+	return len(path) > 0 && os.IsPathSeparator(path[len(path)-1])
+}

pkg/copy/parse.go

@@ -2,6 +2,7 @@ package copy
 
 import (
 	"fmt"
+	"path/filepath"
 	"strings"
 )
 
@@ -40,6 +41,14 @@ func parseUserInput(input string) (container string, path string) {
 		return
 	}
 
+	// If the input is an absolute path, it cannot refer to a container.
+	// This is necessary because absolute paths on Windows will include
+	// a colon, which would cause the drive letter to be parsed as a
+	// container name.
+	if filepath.IsAbs(input) {
+		return
+	}
+
 	if parsedContainer, parsedPath, ok := strings.Cut(path, ":"); ok {
 		container = parsedContainer
 		path = parsedPath

pkg/machine/e2e/basic_test.go

@@ -1,11 +1,8 @@
 package e2e_test
 
 import (
-	"archive/tar"
-	"bytes"
 	"fmt"
 	"io"
-	"io/fs"
 	"net"
 	"net/http"
 	"net/url"
@@ -288,87 +285,6 @@ var _ = Describe("run basic podman commands", func() {
 		Expect(run).To(Exit(0))
 		Expect(build.outputToString()).To(ContainSubstring(name))
 	})
-
-	It("Copy ops", func() {
-		var (
-			stdinDirectory = "stdin-dir"
-			stdinFile      = "file.txt"
-		)
-
-		now := time.Now()
-
-		tarBuffer := &bytes.Buffer{}
-		tw := tar.NewWriter(tarBuffer)
-
-		// Write a directory header to the tar
-		err := tw.WriteHeader(&tar.Header{
-			Name:       stdinDirectory,
-			Mode:       int64(0640 | fs.ModeDir),
-			Gid:        1000,
-			ModTime:    now,
-			ChangeTime: now,
-			AccessTime: now,
-			Typeflag:   tar.TypeDir,
-		})
-		Expect(err).ToNot(HaveOccurred())
-
-		// Write a file header to the tar
-		err = tw.WriteHeader(&tar.Header{
-			Name:       path.Join(stdinDirectory, stdinFile),
-			Mode:       0755,
-			Uid:        1000,
-			ModTime:    now,
-			ChangeTime: now,
-			AccessTime: now,
-		})
-		Expect(err).ToNot(HaveOccurred())
-
-		err = tw.Close()
-		Expect(err).ToNot(HaveOccurred())
-
-		name := randomString()
-		i := new(initMachine)
-		session, err := mb.setName(name).setCmd(i.withImage(mb.imagePath).withNow()).run()
-		Expect(err).ToNot(HaveOccurred())
-		Expect(session).To(Exit(0))
-
-		bm := basicMachine{}
-		newImgs, err := mb.setCmd(bm.withPodmanCommand([]string{"pull", TESTIMAGE})).run()
-		Expect(err).ToNot(HaveOccurred())
-		Expect(newImgs).To(Exit(0))
-		Expect(newImgs.outputToStringSlice()).To(HaveLen(1))
-
-		createAlp, err := mb.setCmd(bm.withPodmanCommand([]string{"create", TESTIMAGE, "top"})).run()
-		Expect(err).ToNot(HaveOccurred())
-		Expect(createAlp).To(Exit(0))
-		Expect(createAlp.outputToStringSlice()).To(HaveLen(1))
-
-		// Testing stdin copy with archive mode disabled (ownership will be determined by the tar file)
-		containerID := createAlp.outputToStringSlice()[0]
-		cpTar, err := mb.setCmd(bm.withPodmanCommand([]string{"cp", "-a=false", "-", containerID + ":/tmp"})).setStdin(tarBuffer).run()
-		Expect(err).ToNot(HaveOccurred())
-		Expect(cpTar).To(Exit(0))
-
-		start, err := mb.setCmd(bm.withPodmanCommand([]string{"start", containerID})).run()
-		Expect(err).ToNot(HaveOccurred())
-		Expect(start).To(Exit(0))
-
-		// Check the directory is created with the appropriate mode, uid, gid
-		exec, err := mb.setCmd(bm.withPodmanCommand([]string{"exec", containerID, "stat", "-c", "%a %u %g", "/tmp/stdin-dir"})).run()
-		Expect(err).ToNot(HaveOccurred())
-		Expect(exec).To(Exit(0))
-		execStdOut := exec.outputToStringSlice()
-		Expect(execStdOut).To(HaveLen(1))
-		Expect(execStdOut[0]).To(Equal("640 0 1000"))
-
-		// Check the file is created with the appropriate mode, uid, gid
-		exec, err = mb.setCmd(bm.withPodmanCommand([]string{"exec", containerID, "stat", "-c", "%a %u %g", "/tmp/stdin-dir/file.txt"})).run()
-		Expect(err).ToNot(HaveOccurred())
-		Expect(exec).To(Exit(0))
-		execStdOut = exec.outputToStringSlice()
-		Expect(execStdOut).To(HaveLen(1))
-		Expect(execStdOut[0]).To(Equal("755 1000 0"))
-	})
 })
 
 func testHTTPServer(port string, shouldErr bool, expectedResponse string) {