cp: keep host-to-container writes inside the container mount

pkg/containerutil/cp_linux.go

@@ -25,6 +25,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"strings"
 
@@ -60,6 +61,81 @@ var (
 	ErrCannotCopyDirToFile        = errors.New("cannot copy a directory to a file")                              // cp SOMEDIR foo:/etc/issue
 )
 
+const (
+	// procSelfFd is where a process finds its own file descriptors by number.
+	procSelfFd = "/proc/self/fd/"
+	// pinnedDirArg is how the tar process refers to the directory handed to it as the first entry of ExtraFiles,
+	// which becomes its fd 3.
+	pinnedDirArg = procSelfFd + "3"
+)
+
+// openMountRoot opens the container mount whose host path is mountRoot as an os.Root, and returns the path of
+// relativePath inside it. A mount of a single file is opened on its parent directory.
+func openMountRoot(mountRoot, relativePath string) (*os.Root, string, error) {
+	if !filepath.IsAbs(mountRoot) {
+		return nil, "", fmt.Errorf("cannot copy into mount with non-path source %q", mountRoot)
+	}
+	st, err := os.Stat(mountRoot)
+	if err != nil {
+		return nil, "", err
+	}
+	if !st.IsDir() {
+		relativePath = filepath.Join(filepath.Base(mountRoot), relativePath)
+		mountRoot = filepath.Dir(mountRoot)
+	}
+	root, err := os.OpenRoot(mountRoot)
+	if err != nil {
+		return nil, "", err
+	}
+	if relativePath == "" {
+		relativePath = "."
+	}
+	return root, relativePath, nil
+}
+
+// openExtractDir prepares a copy into the container: it creates the destination when it does not exist, and returns
+// a handle on the directory tar extracts into, which is the destination itself if intoDest, and its parent otherwise.
+// A process in the container can replace any component of the destination with a symlink after it has been
+// resolved, and absolute symlinks under /proc/<pid>/root are followed against the host root. Both operations go
+// through an os.Root on the container mount so that they stay inside it, and tar gets the handle rather than a path.
+func openExtractDir(dest *pathSpecifier, intoDest bool) (*os.File, error) {
+	root, rel, err := openMountRoot(dest.mountRoot, dest.relativePath)
+	if err != nil {
+		return nil, errors.Join(ErrFilesystem, err)
+	}
+	defer root.Close()
+
+	// XXX FIXME: this seems wrong. What about ownership? We could be doing that inside a container
+	if !dest.exists {
+		if err = root.Mkdir(rel, 0o755); err != nil {
+			return nil, errors.Join(ErrFilesystem, err)
+		}
+	}
+
+	if !intoDest {
+		rel = filepath.Dir(rel)
+	}
+	dir, err := root.OpenFile(rel, unix.O_PATH|unix.O_DIRECTORY, 0)
+	if err != nil {
+		return nil, errors.Join(ErrFilesystem, err)
+	}
+	return dir, nil
+}
+
+// tarExtractCmd returns the command running tarX, a tar -x command line to which it appends -C and -f, so that tar
+// extracts the archive read from stdin into dir. If pinned (the handle from openExtractDir) is not nil, tar receives
+// it as its fd 3 and extracts there instead of resolving dir again.
+func tarExtractCmd(ctx context.Context, tarX []string, dir string, pinned *os.File) *exec.Cmd {
+	var extraFiles []*os.File
+	if pinned != nil {
+		dir = pinnedDirArg
+		extraFiles = []*os.File{pinned}
+	}
+	cmd := exec.CommandContext(ctx, tarX[0], append(slices.Clone(tarX[1:]), "-C", dir, "-f", "-")...)
+	cmd.ExtraFiles = extraFiles
+	return cmd
+}
+
 // getRoot will tentatively return the root of the container on the host (/proc/pid/root), along with the pid,
 // (eg: doable when the container is running)
 func getRoot(ctx context.Context, container containerd.Container) (string, int, error) {
@@ -213,8 +289,17 @@ func CopyFiles(ctx context.Context, client *containerd.Client, container contain
 		return ErrDestinationDirMustExist
 	}
 
-	// XXX FIXME: this seems wrong. What about ownership? We could be doing that inside a container
-	if !destinationSpec.exists {
+	// tar extracts into the destination itself, or into its parent when copying a file to a new name
+	intoDest := sourceSpec.isADir || destinationSpec.endsWithSeparator || (destinationSpec.exists && destinationSpec.isADir)
+
+	var tarXDirFile *os.File
+	if !options.Container2Host {
+		tarXDirFile, err = openExtractDir(destinationSpec, intoDest)
+		if err != nil {
+			return err
+		}
+		defer tarXDirFile.Close()
+	} else if !destinationSpec.exists {
 		if err = os.Mkdir(destinationSpec.resolvedPath, 0o755); err != nil {
 			return errors.Join(ErrFilesystem, err)
 		}
@@ -272,7 +357,7 @@ func CopyFiles(ctx context.Context, client *containerd.Client, container contain
 	}
 
 	tarXDir := destinationSpec.resolvedPath
-	if !sourceSpec.isADir && !destinationSpec.endsWithSeparator && !(destinationSpec.exists && destinationSpec.isADir) {
+	if !intoDest {
 		tarXDir = filepath.Dir(destinationSpec.resolvedPath)
 	}
 	var tarX []string
@@ -283,11 +368,15 @@ func CopyFiles(ctx context.Context, client *containerd.Client, container contain
 		if options.Container2Host && isGNUTar {
 			tarX = append(tarX, "--no-same-owner")
 		}
-		tarX = append(tarX, "-C", tarXDir, "-f", "-")
 
 		// tar opens the -C directory by itself and fails with an unhelpful error when the directory
 		// is not accessible, so detect this beforehand.
-		if accessErr := unix.Access(tarXDir, unix.X_OK); errors.Is(accessErr, unix.EACCES) {
+		accessPath := tarXDir
+		if tarXDirFile != nil {
+			// this process has the handle under another number than tar
+			accessPath = procSelfFd + strconv.Itoa(int(tarXDirFile.Fd()))
+		}
+		if accessErr := unix.Access(accessPath, unix.X_OK); errors.Is(accessErr, unix.EACCES) {
 			return ErrTargetIsReadOnly
 		}
 	}
@@ -308,7 +397,12 @@ func CopyFiles(ctx context.Context, client *containerd.Client, container contain
 	tarCCmd.Stdin = nil
 	tarCCmd.Stderr = os.Stderr
 
-	tarXCmd := exec.CommandContext(ctx, tarX[0], tarX[1:]...)
+	var tarXCmd *exec.Cmd
+	if destinationSpec.toStdout {
+		tarXCmd = exec.CommandContext(ctx, tarX[0], tarX[1:]...)
+	} else {
+		tarXCmd = tarExtractCmd(ctx, tarX, tarXDir, tarXDirFile)
+	}
 	if sourceSpec.fromStdin {
 		// Reading from tar should pipe stdin into dst
 		tarXCmd.Stdin = bufio.NewReader(os.Stdin)

pkg/containerutil/cp_linux_test.go

@@ -0,0 +1,219 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package containerutil
+
+import (
+	"context"
+	"errors"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strconv"
+	"testing"
+
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"gotest.tools/v3/assert"
+
+	"github.com/containerd/containerd/v2/pkg/oci"
+
+	"github.com/containerd/nerdctl/v2/pkg/tarutil"
+)
+
+// tempDir returns a new temporary directory with symlinks resolved, the way the kernel reports open files.
+func tempDir(t *testing.T) string {
+	t.Helper()
+	dir, err := filepath.EvalSymlinks(t.TempDir())
+	assert.NilError(t, err)
+	return dir
+}
+
+// containerRoot returns a directory standing in for /proc/<pid>/root, with a volume mounted at /vol.
+func containerRoot(t *testing.T) (string, string, *oci.Spec) {
+	t.Helper()
+	root := tempDir(t)
+	volume := tempDir(t)
+	assert.NilError(t, os.MkdirAll(filepath.Join(root, "data", "x"), 0o755))
+	assert.NilError(t, os.MkdirAll(filepath.Join(root, "vol"), 0o755))
+	assert.NilError(t, os.MkdirAll(filepath.Join(volume, "sub"), 0o755))
+	return root, volume, &oci.Spec{
+		Root:   &specs.Root{Path: "rootfs"},
+		Mounts: []specs.Mount{{Destination: "/vol", Source: volume, Options: []string{"rbind"}}},
+	}
+}
+
+// swapForSymlink replaces the directory at path with a symlink to target, as a process in the container can
+// between the time cp resolves the destination and the time it writes to it.
+func swapForSymlink(t *testing.T, path, target string) {
+	t.Helper()
+	assert.NilError(t, os.Rename(path, path+".orig"))
+	assert.NilError(t, os.Symlink(target, path))
+}
+
+func handlePath(t *testing.T, f *os.File) string {
+	t.Helper()
+	p, err := os.Readlink("/proc/self/fd/" + strconv.Itoa(int(f.Fd())))
+	assert.NilError(t, err)
+	return p
+}
+
+func TestGetPathSpecFromContainerRecordsMount(t *testing.T) {
+	root, volume, conSpec := containerRoot(t)
+
+	spec, err := getPathSpecFromContainer("/data/newfile", conSpec, root)
+	assert.NilError(t, err)
+	assert.Equal(t, spec.mountRoot, root)
+	assert.Equal(t, spec.relativePath, filepath.Join("data", "newfile"))
+
+	spec, err = getPathSpecFromContainer("/vol/sub", conSpec, root)
+	assert.NilError(t, err)
+	assert.Equal(t, spec.mountRoot, volume)
+	assert.Equal(t, spec.relativePath, "sub")
+}
+
+func TestOpenExtractDir(t *testing.T) {
+	root, volume, conSpec := containerRoot(t)
+
+	// a file copied to a new name: the destination is created, as before, and tar extracts into the parent
+	spec, err := getPathSpecFromContainer("/data/x/newname", conSpec, root)
+	assert.NilError(t, err)
+	dir, err := openExtractDir(spec, false)
+	assert.NilError(t, err)
+	assert.Equal(t, handlePath(t, dir), filepath.Join(root, "data", "x"))
+	assert.NilError(t, dir.Close())
+	st, err := os.Lstat(filepath.Join(root, "data", "x", "newname"))
+	assert.NilError(t, err)
+	assert.Assert(t, st.IsDir())
+
+	// a directory copied to a new name in a volume: the destination is created and tar extracts into it
+	spec, err = getPathSpecFromContainer("/vol/newdir", conSpec, root)
+	assert.NilError(t, err)
+	dir, err = openExtractDir(spec, true)
+	assert.NilError(t, err)
+	assert.Equal(t, handlePath(t, dir), filepath.Join(volume, "newdir"))
+	assert.NilError(t, dir.Close())
+}
+
+func TestOpenExtractDirFileBindMount(t *testing.T) {
+	root := tempDir(t)
+	hostDir := tempDir(t)
+	hosts := filepath.Join(hostDir, "hosts")
+	assert.NilError(t, os.MkdirAll(filepath.Join(root, "etc"), 0o755))
+	assert.NilError(t, os.WriteFile(filepath.Join(root, "etc", "hosts"), nil, 0o644))
+	assert.NilError(t, os.WriteFile(hosts, nil, 0o644))
+	conSpec := &oci.Spec{
+		Root:   &specs.Root{Path: "rootfs"},
+		Mounts: []specs.Mount{{Destination: "/etc/hosts", Source: hosts, Options: []string{"rbind"}}},
+	}
+
+	spec, err := getPathSpecFromContainer("/etc/hosts", conSpec, root)
+	assert.NilError(t, err)
+	dir, err := openExtractDir(spec, false)
+	assert.NilError(t, err)
+	defer dir.Close()
+	assert.Equal(t, handlePath(t, dir), hostDir)
+}
+
+func TestOpenExtractDirRejectsNonPathSource(t *testing.T) {
+	_, err := openExtractDir(&pathSpecifier{mountRoot: "tmpfs", relativePath: "x"}, true)
+	assert.Assert(t, errors.Is(err, ErrFilesystem))
+	assert.ErrorContains(t, err, "non-path source")
+}
+
+// TestOpenExtractDirSwapped swaps a component of the destination for a symlink leaving the container after it has
+// been resolved, both for a destination that exists and for one that has to be created.
+func TestOpenExtractDirSwapped(t *testing.T) {
+	for _, tc := range []struct {
+		name     string
+		relative bool
+		dest     string
+		intoDest bool
+	}{
+		{"absolute symlink, existing destination", false, "/data/x", true},
+		{"absolute symlink, created destination", false, "/data/x/new", true},
+		{"relative symlink, existing destination", true, "/data/x", true},
+		{"relative symlink, created destination", true, "/data/x/new", true},
+		{"absolute symlink, file to new name", false, "/data/x/newname", false},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			root, _, conSpec := containerRoot(t)
+			outside := tempDir(t)
+			spec, err := getPathSpecFromContainer(tc.dest, conSpec, root)
+			assert.NilError(t, err)
+
+			target := outside
+			if tc.relative {
+				target, err = filepath.Rel(filepath.Join(root, "data"), outside)
+				assert.NilError(t, err)
+			}
+			swapForSymlink(t, filepath.Join(root, "data", "x"), target)
+
+			dir, err := openExtractDir(spec, tc.intoDest)
+			if err == nil {
+				p := handlePath(t, dir)
+				dir.Close()
+				t.Fatalf("opened %s outside of the container", p)
+			}
+			assert.Assert(t, errors.Is(err, ErrFilesystem), err)
+			entries, err := os.ReadDir(outside)
+			assert.NilError(t, err)
+			assert.Equal(t, len(entries), 0, "created something outside of the container")
+		})
+	}
+}
+
+func TestTarExtractCmdUnpinned(t *testing.T) {
+	cmd := tarExtractCmd(context.Background(), []string{"tar", "-x", "--no-same-owner"}, "/some/dir", nil)
+	assert.DeepEqual(t, cmd.Args, []string{"tar", "-x", "--no-same-owner", "-C", "/some/dir", "-f", "-"})
+	assert.Assert(t, cmd.ExtraFiles == nil)
+}
+
+// TestTarExtractCmd runs the tar command CopyFiles uses for a copy into the container: after the handle is opened,
+// swapping the directory for a symlink does not redirect the extraction.
+func TestTarExtractCmd(t *testing.T) {
+	tarBinary, _, err := tarutil.FindTarBinary()
+	if err != nil {
+		t.Skipf("no tar binary: %v", err)
+	}
+
+	src := tempDir(t)
+	outside := tempDir(t)
+	root, _, conSpec := containerRoot(t)
+	assert.NilError(t, os.WriteFile(filepath.Join(src, "payload"), []byte("x"), 0o644))
+
+	spec, err := getPathSpecFromContainer("/data/x", conSpec, root)
+	assert.NilError(t, err)
+	dir, err := openExtractDir(spec, true)
+	assert.NilError(t, err)
+	defer dir.Close()
+
+	swapForSymlink(t, filepath.Join(root, "data", "x"), outside)
+
+	archive := exec.Command(tarBinary, "-C", src, "-c", "-f", "-", "payload")
+	extract := tarExtractCmd(context.Background(), []string{tarBinary, "-x"}, filepath.Join(root, "data", "x"), dir)
+	extract.Stdin, err = archive.StdoutPipe()
+	assert.NilError(t, err)
+	assert.NilError(t, archive.Start())
+	out, err := extract.CombinedOutput()
+	archiveErr := archive.Wait()
+	assert.NilError(t, err, string(out))
+	assert.NilError(t, archiveErr)
+
+	_, err = os.Lstat(filepath.Join(outside, "payload"))
+	assert.Assert(t, os.IsNotExist(err), "extraction followed the swapped symlink out of the container")
+	_, err = os.Lstat(filepath.Join(root, "data", "x.orig", "payload"))
+	assert.NilError(t, err, "extraction did not land in the pinned directory")
+}

pkg/containerutil/cp_resolve_linux.go

@@ -58,6 +58,11 @@ type pathSpecifier struct {
 	readOnly             bool
 	fromStdin            bool
 	toStdout             bool
+
+	// For a container path: the host path of the container mount that holds the resource, and the path of the
+	// resource relative to it. Writes into the container go through these (see openMountRoot).
+	mountRoot    string
+	relativePath string
 }
 
 // getPathSpecFromHost builds a pathSpecifier from a host location
@@ -202,6 +207,8 @@ func getPathSpecFromContainer(originalPath string, conSpec *oci.Spec, containerH
 	// Now, finally get the location of the fully resolved containerPath (in the root? in a volume?)
 	containerMount, relativePath := pathResolver.getMount(resolvedContainerPath)
 	pathSpec.resolvedPath = filepath.Join(containerMount.hostPath, relativePath)
+	pathSpec.mountRoot = containerMount.hostPath
+	pathSpec.relativePath = relativePath
 	// If the endpoint is readonly, flag it as such
 	if containerMount.readonly {
 		pathSpec.readOnly = true