chunked: handle creating root directory

pkg/chunked/dump/dump.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/containers/storage/pkg/chunked/internal"
+	storagePath "github.com/containers/storage/pkg/chunked/internal/path"
 	"golang.org/x/sys/unix"
 )
 
@@ -103,18 +104,8 @@ func getStMode(mode uint32, typ string) (uint32, error) {
 	return mode, nil
 }
 
-func sanitizeName(name string) string {
-	path := filepath.Clean(name)
-	if path == "." {
-		path = "/"
-	} else if path[0] != '/' {
-		path = "/" + path
-	}
-	return path
-}
-
 func dumpNode(out io.Writer, added map[string]*internal.FileMetadata, links map[string]int, verityDigests map[string]string, entry *internal.FileMetadata) error {
-	path := sanitizeName(entry.Name)
+	path := storagePath.CleanAbsPath(entry.Name)
 
 	parent := filepath.Dir(path)
 	if _, found := added[parent]; !found && path != "/" {
@@ -172,7 +163,7 @@ func dumpNode(out io.Writer, added map[string]*internal.FileMetadata, links map[
 		if entry.Type == internal.TypeSymlink {
 			payload = entry.Linkname
 		} else {
-			payload = sanitizeName(entry.Linkname)
+			payload = storagePath.CleanAbsPath(entry.Linkname)
 		}
 	} else {
 		if len(entry.Digest) > 10 {

pkg/chunked/filesystem_linux.go

@@ -16,6 +16,7 @@ import (
 	driversCopy "github.com/containers/storage/drivers/copy"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/chunked/internal"
+	storagePath "github.com/containers/storage/pkg/chunked/internal/path"
 	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/vbatts/tar-split/archive/tar"
 	"golang.org/x/sys/unix"
@@ -49,10 +50,37 @@ type fileMetadata struct {
 	skipSetAttrs bool
 }
 
+// splitPath takes a file path as input and returns two components: dir and base.
+// Differently than filepath.Split(), this function handles some edge cases.
+// If the path refers to a file in the root directory, the returned dir is "/".
+// The returned base value is never empty, it never contains any slash and the
+// value "..".
+func splitPath(path string) (string, string, error) {
+	path = storagePath.CleanAbsPath(path)
+	dir, base := filepath.Split(path)
+	if base == "" {
+		base = "."
+	}
+	// Remove trailing slashes from dir, but make sure that "/" is preserved.
+	dir = strings.TrimSuffix(dir, "/")
+	if dir == "" {
+		dir = "/"
+	}
+
+	if strings.Contains(base, "/") {
+		// This should never happen, but be safe as the base is passed to *at syscalls.
+		return "", "", fmt.Errorf("internal error: splitPath(%q) contains a slash", path)
+	}
+	return dir, base, nil
+}
+
 func doHardLink(dirfd, srcFd int, destFile string) error {
-	destDir, destBase := filepath.Split(destFile)
+	destDir, destBase, err := splitPath(destFile)
+	if err != nil {
+		return err
+	}
 	destDirFd := dirfd
-	if destDir != "" && destDir != "." {
+	if destDir != "/" {
 		f, err := openOrCreateDirUnderRoot(dirfd, destDir, 0)
 		if err != nil {
 			return err
@@ -72,7 +100,7 @@ func doHardLink(dirfd, srcFd int, destFile string) error {
 		return nil
 	}
 
-	err := doLink()
+	err = doLink()
 
 	// if the destination exists, unlink it first and try again
 	if err != nil && os.IsExist(err) {
@@ -281,8 +309,11 @@ func openFileUnderRootFallback(dirfd int, name string, flags uint64, mode os.Fil
 	// If O_NOFOLLOW is specified in the flags, then resolve only the parent directory and use the
 	// last component as the path to openat().
 	if hasNoFollow {
-		dirName, baseName := filepath.Split(name)
-		if dirName != "" && dirName != "." {
+		dirName, baseName, err := splitPath(name)
+		if err != nil {
+			return -1, err
+		}
+		if dirName != "/" {
 			newRoot, err := securejoin.SecureJoin(root, dirName)
 			if err != nil {
 				return -1, err
@@ -409,7 +440,8 @@ func openOrCreateDirUnderRoot(dirfd int, name string, mode os.FileMode) (*os.Fil
 
 	if errors.Is(err, unix.ENOENT) {
 		parent := filepath.Dir(name)
-		if parent != "" {
+		// do not create the root directory, it should always exist
+		if parent != name {
 			pDir, err2 := openOrCreateDirUnderRoot(dirfd, parent, mode)
 			if err2 != nil {
 				return nil, err
@@ -448,9 +480,12 @@ func appendHole(fd int, name string, size int64) error {
 }
 
 func safeMkdir(dirfd int, mode os.FileMode, name string, metadata *fileMetadata, options *archive.TarOptions) error {
-	parent, base := filepath.Split(name)
+	parent, base, err := splitPath(name)
+	if err != nil {
+		return err
+	}
 	parentFd := dirfd
-	if parent != "" && parent != "." {
+	if parent != "/" {
 		parentFile, err := openOrCreateDirUnderRoot(dirfd, parent, 0)
 		if err != nil {
 			return err
@@ -506,9 +541,12 @@ func safeLink(dirfd int, mode os.FileMode, metadata *fileMetadata, options *arch
 }
 
 func safeSymlink(dirfd int, metadata *fileMetadata) error {
-	destDir, destBase := filepath.Split(metadata.Name)
+	destDir, destBase, err := splitPath(metadata.Name)
+	if err != nil {
+		return err
+	}
 	destDirFd := dirfd
-	if destDir != "" && destDir != "." {
+	if destDir != "/" {
 		f, err := openOrCreateDirUnderRoot(dirfd, destDir, 0)
 		if err != nil {
 			return err
@@ -542,9 +580,12 @@ func (d whiteoutHandler) Setxattr(path, name string, value []byte) error {
 }
 
 func (d whiteoutHandler) Mknod(path string, mode uint32, dev int) error {
-	dir, base := filepath.Split(path)
+	dir, base, err := splitPath(path)
+	if err != nil {
+		return err
+	}
 	dirfd := d.Dirfd
-	if dir != "" && dir != "." {
+	if dir != "/" {
 		dir, err := openOrCreateDirUnderRoot(d.Dirfd, dir, 0)
 		if err != nil {
 			return err

pkg/chunked/filesystem_linux_test.go

@@ -2,6 +2,7 @@ package chunked
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"os"
 	"path"
@@ -123,11 +124,14 @@ func TestSafeMkdir(t *testing.T) {
 		IgnoreChownErrors: true,
 	}
 
+	err = safeMkdir(rootFd, 0o755, "/", &metadata, options)
+	require.NoError(t, err)
+
 	err = safeMkdir(rootFd, 0o755, dirName, &metadata, options)
 	require.NoError(t, err)
 
 	dir, err := openFileUnderRoot(rootFd, dirName, syscall.O_DIRECTORY|syscall.O_CLOEXEC, 0)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	err = dir.Close()
 	assert.NoError(t, err)
 }
@@ -167,7 +171,7 @@ func TestSafeLink(t *testing.T) {
 
 	// validate it was created
 	newFile, err := openFileUnderRoot(rootFd, linkName, syscall.O_RDONLY, 0)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	st := syscall.Stat_t{}
 	err = syscall.Fstat(int(newFile.Fd()), &st)
@@ -216,7 +220,7 @@ func TestSafeSymlink(t *testing.T) {
 
 	// validate it was created
 	newFile, err := openFileUnderRoot(rootFd, linkName, syscall.O_RDONLY, 0)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	st2 := syscall.Stat_t{}
 	err = syscall.Fstat(int(newFile.Fd()), &st2)
@@ -334,3 +338,49 @@ func createTempFile(t *testing.T, dir, name string) *os.File {
 	require.NoError(t, err)
 	return tmpFile
 }
+
+func TestSplitPath(t *testing.T) {
+	tests := []struct {
+		path         string
+		expectedDir  string
+		expectedBase string
+	}{
+		{"", "/", "."},
+		{".", "/", "."},
+		{"..", "/", "."},
+		{"../..", "/", "."},
+		{"../../..", "/", "."},
+		{"../../../foo", "/", "foo"},
+		{"../../../foo/..", "/", "."},
+		{"../../../foo/./../foo/bar/baz", "/foo/bar", "baz"},
+		{"../../../foo/bar", "/foo", "bar"},
+		{"/", "/", "."},
+		{"/.", "/", "."},
+		{"/..", "/", "."},
+		{"////foo////bar////", "/foo", "bar"},
+		{"/foo", "/", "foo"},
+		{"/foo/", "/", "foo"},
+		{"/foo/bar", "/foo", "bar"},
+		{"/foo/bar/", "/foo", "bar"},
+		{"/foo/////bar/", "/foo", "bar"},
+		{"/home/foo/file.txt", "/home/foo", "file.txt"},
+		{"/home/foo////file.txt", "/home/foo", "file.txt"},
+		{"file", "/", "file"},
+		{"foo/", "/", "foo"},
+		{"foo/.", "/", "foo"},
+		{"foo/..", "/", "."},
+		{"foo/../../bar", "/", "bar"},
+		{"foo/bar/", "/foo", "bar"},
+		{"foo/bar/..", "/", "foo"},
+		{"foo/bar/../baz", "/foo", "baz"},
+		{"foo/bar/baz/", "/foo/bar", "baz"},
+		{"foo/file.txt", "/foo", "file.txt"},
+	}
+
+	for _, test := range tests {
+		dir, base, err := splitPath(test.path)
+		assert.NoError(t, err)
+		assert.Equal(t, test.expectedDir, dir, fmt.Sprintf("path %q: expected dir %q, got %q", test.path, test.expectedDir, dir))
+		assert.Equal(t, test.expectedBase, base, fmt.Sprintf("path %q: expected base %q, got %q", test.path, test.expectedBase, base))
+	}
+}

pkg/chunked/internal/path/path.go

@@ -0,0 +1,12 @@
+package path
+
+import (
+	"path/filepath"
+)
+
+// CleanAbsPath removes any ".." and "." from the path
+// and ensures it starts with a "/".  If the path refers to the root
+// directory, it returns "/".
+func CleanAbsPath(path string) string {
+	return filepath.Clean("/" + path)
+}

pkg/chunked/internal/path/path_test.go

@@ -0,0 +1,48 @@
+package path
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCleanAbsPath(t *testing.T) {
+	tests := []struct {
+		path     string
+		expected string
+	}{
+		{"", "/"},
+		{".", "/"},
+		{"..", "/"},
+		{"foo/../..", "/"},
+		{"/foo/../..", "/"},
+		{"./", "/"},
+		{"../", "/"},
+		{"/../", "/"},
+		{"/./", "/"},
+		{"foo", "/foo"},
+		{"foo/bar", "/foo/bar"},
+		{"/foo/bar/../baz", "/foo/baz"},
+		{"/foo/./bar", "/foo/bar"},
+		{"/foo/bar/../../baz", "/baz"},
+		{"/././foo", "/foo"},
+		{"../foo", "/foo"},
+		{"./foo/bar/../..", "/"},
+		{"foo/..", "/"},
+		{"foo/../bar", "/bar"},
+		{"//foo//bar", "/foo/bar"},
+		{"foo/bar//baz/..", "/foo/bar"},
+		{"../..", "/"},
+		{".././..", "/"},
+		{"../../.", "/"},
+		{"/../../foo", "/foo"},
+		{"../foo/bar/../baz", "/foo/baz"},
+		{"../.././/.//../foo/./../bar/..", "/"},
+		{"a/../.././/.//../foo/./../bar/..", "/"},
+	}
+
+	for _, test := range tests {
+		assert.Equal(t, test.expected, CleanAbsPath(test.path), fmt.Sprintf("path %q failed", test.path))
+	}
+}

pkg/chunked/storage_linux.go

@@ -23,6 +23,7 @@ import (
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/chunked/compressor"
 	"github.com/containers/storage/pkg/chunked/internal"
+	path "github.com/containers/storage/pkg/chunked/internal/path"
 	"github.com/containers/storage/pkg/chunked/toc"
 	"github.com/containers/storage/pkg/fsverity"
 	"github.com/containers/storage/pkg/idtools"
@@ -1544,10 +1545,10 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
 			}
 		}
 
-		r.Name = filepath.Clean(r.Name)
+		r.Name = path.CleanAbsPath(r.Name)
 		// do not modify the value of symlinks
 		if r.Linkname != "" && t != tar.TypeSymlink {
-			r.Linkname = filepath.Clean(r.Linkname)
+			r.Linkname = path.CleanAbsPath(r.Linkname)
 		}
 
 		if whiteoutConverter != nil {
@@ -1595,7 +1596,7 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
 			}
 
 		case tar.TypeDir:
-			if r.Name == "" || r.Name == "." {
+			if r.Name == "/" {
 				output.RootDirMode = &mode
 			}
 			if err := safeMkdir(dirfd, mode, r.Name, r, options); err != nil {