Merge pull request #44196 from neersighted/createImpliedDirectories

refactor(pkg/archive): factor out createImpliedDirectories helper

pkg/archive/archive.go

@@ -31,6 +31,18 @@ import (
 	exec "golang.org/x/sys/execabs"
 )
 
+// ImpliedDirectoryMode represents the mode (Unix permissions) applied to directories that are implied by files in a
+// tar, but that do not have their own header entry.
+//
+// The permissions mask is stored in a constant instead of locally to ensure that magic numbers do not
+// proliferate in the codebase. The default value 0755 has been selected based on the default umask of 0022, and
+// a convention of mkdir(1) calling mkdir(2) with permissions of 0777, resulting in a final value of 0755.
+//
+// This value is currently implementation-defined, and not captured in any cross-runtime specification. Thus, it is
+// subject to change in Moby at any time -- image authors who require consistent or known directory permissions
+// should explicitly control them by ensuring that header entries exist for any applicable path.
+const ImpliedDirectoryMode = 0755
+
 type (
 	// Compression is the state represents if compressed or not.
 	Compression int
@@ -810,7 +822,6 @@ func Tar(path string, compression Compression) (io.ReadCloser, error) {
 // TarWithOptions creates an archive from the directory at `path`, only including files whose relative
 // paths are included in `options.IncludeFiles` (if non-nil) or not in `options.ExcludePatterns`.
 func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) {
-
 	// Fix the source path to work with long path names. This is a no-op
 	// on platforms other than Windows.
 	srcPath = fixVolumePathPrefix(srcPath)
@@ -1018,7 +1029,6 @@ func Unpack(decompressedArchive io.Reader, dest string, options *TarOptions) err
 	defer pools.BufioReader32KPool.Put(trBuf)
 
 	var dirs []*tar.Header
-	rootIDs := options.IDMap.RootPair()
 	whiteoutConverter, err := getWhiteoutConverter(options.WhiteoutFormat, options.InUserNS)
 	if err != nil {
 		return err
@@ -1053,19 +1063,10 @@ loop:
 			}
 		}
 
-		// After calling filepath.Clean(hdr.Name) above, hdr.Name will now be in
+		// Ensure that the parent directory exists.
-		// the filepath format for the OS on which the daemon is running. Hence
+		err = createImpliedDirectories(dest, hdr, options)
-		// the check for a slash-suffix MUST be done in an OS-agnostic way.
+		if err != nil {
-		if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) {
+			return err
-			// Not the root directory, ensure that the parent directory exists
-			parent := filepath.Dir(hdr.Name)
-			parentPath := filepath.Join(dest, parent)
-			if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) {
-				err = idtools.MkdirAllAndChownNew(parentPath, 0755, rootIDs)
-				if err != nil {
-					return err
-				}
-			}
 		}
 
 		// #nosec G305 -- The joined path is checked for path traversal.
@@ -1143,6 +1144,35 @@ loop:
 	return nil
 }
 
+// createImpliedDirectories will create all parent directories of the current path with default permissions, if they do
+// not already exist. This is possible as the tar format supports 'implicit' directories, where their existence is
+// defined by the paths of files in the tar, but there are no header entries for the directories themselves, and thus
+// we most both create them and choose metadata like permissions.
+//
+// The caller should have performed filepath.Clean(hdr.Name), so hdr.Name will now be in the filepath format for the OS
+// on which the daemon is running. This precondition is required because this function assumes a OS-specific path
+// separator when checking that a path is not the root.
+func createImpliedDirectories(dest string, hdr *tar.Header, options *TarOptions) error {
+	// Not the root directory, ensure that the parent directory exists
+	if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) {
+		parent := filepath.Dir(hdr.Name)
+		parentPath := filepath.Join(dest, parent)
+		if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) {
+			// RootPair() is confined inside this loop as most cases will not require a call, so we can spend some
+			// unneeded function calls in the uncommon case to encapsulate logic -- implied directories are a niche
+			// usage that reduces the portability of an image.
+			rootIDs := options.IDMap.RootPair()
+
+			err = idtools.MkdirAllAndChownNew(parentPath, ImpliedDirectoryMode, rootIDs)
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
 // Untar reads a stream of bytes from `archive`, parses it as a tar archive,
 // and unpacks it into the directory at `dest`.
 // The archive may be compressed with one of the following algorithms:

pkg/archive/archive_test.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -1216,7 +1217,7 @@ func TestTempArchiveCloseMultipleTimes(t *testing.T) {
 	}
 }
 
-// TestXGlobalNoParent is a regression test to check parent directories are not crated for PAX headers
+// TestXGlobalNoParent is a regression test to check parent directories are not created for PAX headers
 func TestXGlobalNoParent(t *testing.T) {
 	buf := &bytes.Buffer{}
 	w := tar.NewWriter(buf)
@@ -1236,6 +1237,53 @@ func TestXGlobalNoParent(t *testing.T) {
 	assert.Check(t, errors.Is(err, os.ErrNotExist))
 }
 
+// TestImpliedDirectoryPermissions ensures that directories implied by paths in the tar file, but without their own
+// header entries are created recursively with the default mode (permissions) stored in ImpliedDirectoryMode. This test
+// also verifies that the permissions of explicit directories are respected.
+func TestImpliedDirectoryPermissions(t *testing.T) {
+	skip.If(t, runtime.GOOS == "windows", "skipping test that requires Unix permissions")
+
+	buf := &bytes.Buffer{}
+	headers := []tar.Header{{
+		Name: "deeply/nested/and/implied",
+	}, {
+		Name: "explicit/",
+		Mode: 0644,
+	}, {
+		Name: "explicit/permissions/",
+		Mode: 0600,
+	}, {
+		Name: "explicit/permissions/specified",
+		Mode: 0400,
+	}}
+
+	w := tar.NewWriter(buf)
+	for _, header := range headers {
+		err := w.WriteHeader(&header)
+		assert.NilError(t, err)
+	}
+
+	tmpDir := t.TempDir()
+
+	err := Untar(buf, tmpDir, nil)
+	assert.NilError(t, err)
+
+	assertMode := func(path string, expected uint32) {
+		t.Helper()
+		stat, err := os.Lstat(filepath.Join(tmpDir, path))
+		assert.Check(t, err)
+		assert.Check(t, is.Equal(stat.Mode().Perm(), fs.FileMode(expected)))
+	}
+
+	assertMode("deeply", ImpliedDirectoryMode)
+	assertMode("deeply/nested", ImpliedDirectoryMode)
+	assertMode("deeply/nested/and", ImpliedDirectoryMode)
+
+	assertMode("explicit", 0644)
+	assertMode("explicit/permissions", 0600)
+	assertMode("explicit/permissions/specified", 0400)
+}
+
 func TestReplaceFileTarWrapper(t *testing.T) {
 	filesInArchive := 20
 	testcases := []struct {

pkg/archive/diff.go

@@ -72,20 +72,10 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64,
 			}
 		}
 
-		// Note as these operations are platform specific, so must the slash be.
+		// Ensure that the parent directory exists.
-		if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) {
+		err = createImpliedDirectories(dest, hdr, options)
-			// Not the root directory, ensure that the parent directory exists.
+		if err != nil {
-			// This happened in some tests where an image had a tarfile without any
+			return 0, err
-			// parent directories.
-			parent := filepath.Dir(hdr.Name)
-			parentPath := filepath.Join(dest, parent)
-
-			if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) {
-				err = system.MkdirAll(parentPath, 0600)
-				if err != nil {
-					return 0, err
-				}
-			}
 		}
 
 		// Skip AUFS metadata dirs