archive: Avoid duplicate implied-parent checks

archive.go

@@ -453,6 +453,11 @@ func (ta *tarAppender) addTarFile(srcPath, archivePath string) error {
 	return nil
 }
 
+type resolvedArchivePath struct {
+	path             string
+	parentWasPresent bool
+}
+
 // resolveArchivePath resolves intermediate symlinks in name using chroot-like
 // semantics when os.Root cannot traverse them. The final path component is
 // intentionally preserved because archive extraction may create or replace it.
@@ -466,13 +471,17 @@ func (ta *tarAppender) addTarFile(srcPath, archivePath string) error {
 // Paths with missing components are supported. Existing symlinks are resolved,
 // and any remaining nonexistent components are retained for later creation.
 //
+// The result records whether this resolution observed the parent directory.
+// Callers may use that observation to avoid a duplicate check for this entry,
+// but must not reuse it for later entries.
+//
 // This helper should eventually be replaced by handle-relative resolution and
 // operations with resolve-in-root semantics, avoiding the resolution/use race
 // and repeated path traversal.
-func resolveArchivePath(root *os.Root, name string) (string, error) {
+func resolveArchivePath(root *os.Root, name string) (resolvedArchivePath, error) {
 	parent, base := filepath.Split(name)
 	if parent == "" {
-		return name, nil
+		return resolvedArchivePath{path: name, parentWasPresent: true}, nil
 	}
 
 	parent = filepath.Clean(parent)
@@ -482,41 +491,41 @@ func resolveArchivePath(root *os.Root, name string) (string, error) {
 	_, statErr := root.Stat(parent)
 	switch {
 	case statErr == nil:
-		return name, nil
+		return resolvedArchivePath{path: name, parentWasPresent: true}, nil
 	case !os.IsNotExist(statErr) && !isPathEscapes(statErr):
-		return "", statErr
+		return resolvedArchivePath{}, statErr
 	}
 
 	// Resolve the parent both to handle ENOENT from missing components or dangling
 	// symlinks, and to determine whether an os.Root breakout was caused by an
 	// absolute symlink. Relative symlink escapes preserve the original Stat error.
 	resolved, err := resolveFSRootPath(root.Name(), parent)
 	if err != nil {
-		return "", err
+		return resolvedArchivePath{}, err
 	}
 
 	if isPathEscapes(statErr) && (!resolved.followedAbsoluteLink || resolved.relativeEscapeBeforeAbsolute) {
-		return "", statErr
+		return resolvedArchivePath{}, statErr
 	}
 
 	relParent, err := filepath.Rel(root.Name(), resolved.path)
 	if err != nil {
-		return "", breakoutError(fmt.Errorf(
+		return resolvedArchivePath{}, breakoutError(fmt.Errorf(
 			"could not make resolved parent %q relative to root %q: %w",
 			resolved.path,
 			root.Name(),
 			err,
 		))
 	}
 	if relParent != "." && !filepath.IsLocal(relParent) {
-		return "", breakoutError(fmt.Errorf(
+		return resolvedArchivePath{}, breakoutError(fmt.Errorf(
 			"resolved parent %q escapes root %q",
 			resolved.path,
 			root.Name(),
 		))
 	}
 
-	return filepath.Join(relParent, base), nil
+	return resolvedArchivePath{path: filepath.Join(relParent, base)}, nil
 }
 
 // resolveHardlinkTarget validates a POSIX hardlink target and resolves it to
@@ -534,7 +543,11 @@ func resolveHardlinkTarget(root *os.Root, linkname string) (string, error) {
 	if cleaned == "." || !filepath.IsLocal(cleaned) {
 		return "", breakoutError(fmt.Errorf("invalid hardlink target %q", linkname))
 	}
-	return resolveArchivePath(root, filepath.FromSlash(cleaned))
+	resolved, err := resolveArchivePath(root, filepath.FromSlash(cleaned))
+	if err != nil {
+		return "", err
+	}
+	return resolved.path, nil
 }
 
 // createTarFile extracts a single tar entry into the given root. dstPath is the
@@ -1034,10 +1047,11 @@ loop:
 		// dstPath is the native (host-separator) form of the entry name,
 		// used at all filesystem boundaries (os.Root methods, fsRootPath).
 		// hdr.Name stays POSIX (forward-slash) for logical string checks.
-		dstPath, err := resolveArchivePath(root, filepath.FromSlash(hdr.Name))
+		resolvedPath, err := resolveArchivePath(root, filepath.FromSlash(hdr.Name))
 		if err != nil {
 			return err
 		}
+		dstPath := resolvedPath.path
 
 		// If dstPath exists we almost always just want to remove and replace it.
 		// The only exception is when it is a directory *and* the file from
@@ -1075,7 +1089,7 @@ loop:
 		//
 		// This must be done before whiteoutConverter.ConvertRead, which
 		// may set xattrs on the directory or create whiteout files.
-		if err := createImpliedDirectories(root, dstPath, options); err != nil {
+		if err := createImpliedDirectories(root, resolvedPath, options); err != nil {
 			return err
 		}
 
@@ -1136,15 +1150,14 @@ func unrepresentableOnWindows(hdr *tar.Header) error {
 // by file paths, without corresponding directory headers from which metadata
 // could be restored.
 //
-// The caller must pass a normalized, root-relative local path. Any archive-path
-// conversion and resolve-in-root handling must already have been applied.
+// The caller must pass the result of resolving the current archive entry.
 // Directory creation is performed through root, so it remains confined to the
 // extraction destination even if the destination tree changes concurrently.
-func createImpliedDirectories(root *os.Root, dstPath string, options *TarOptions) error {
-	parent := filepath.Dir(dstPath)
+func createImpliedDirectories(root *os.Root, resolved resolvedArchivePath, options *TarOptions) error {
+	parent := filepath.Dir(resolved.path)
 
 	// Skip when the parent is the root itself; nothing to create.
-	if parent == "." || parent == "" {
+	if parent == "." || parent == "" || resolved.parentWasPresent {
 		return nil
 	}
 	if _, err := root.Lstat(parent); err == nil {

archive_test.go

@@ -11,6 +11,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
+	"sync"
 	"testing"
 
 	"github.com/moby/sys/user"
@@ -695,6 +696,33 @@ func prepareUntarSourceDirectory(numberOfFiles int, targetPath string, makeLinks
 	return totalSize, nil
 }
 
+func BenchmarkUnpackManyFilesSameParent(b *testing.B) {
+	const files = 4096
+
+	tarData := makeTarWithFiles(b, "dir/subdir", files)
+	target := filepath.Join(b.TempDir(), "dest")
+	options := &TarOptions{NoLchown: true}
+
+	b.ReportAllocs()
+	b.SetBytes(int64(len(tarData)))
+	b.ResetTimer()
+	for range b.N {
+		b.StopTimer()
+		if err := os.RemoveAll(target); err != nil {
+			b.Fatal(err)
+		}
+		if err := os.Mkdir(target, 0o755); err != nil {
+			b.Fatal(err)
+		}
+		r := bytes.NewReader(tarData)
+		b.StartTimer()
+
+		if err := Unpack(r, target, options); err != nil {
+			b.Fatal(err)
+		}
+	}
+}
+
 func BenchmarkTarUntar(b *testing.B) {
 	origin, err := os.MkdirTemp(b.TempDir(), "docker-test-untar-origin")
 	if err != nil {
@@ -856,6 +884,133 @@ func TestUntarSiblingPrefixContained(t *testing.T) {
 	assert.ErrorIs(t, statErr, os.ErrNotExist, "hardlink to prefix-sibling created")
 }
 
+func TestApplyLayerImpliedDirAfterWhiteout(t *testing.T) {
+	dest := t.TempDir()
+
+	var buf bytes.Buffer
+	tw := tar.NewWriter(&buf)
+	writeFile := func(name, contents string) {
+		t.Helper()
+		assert.NilError(t, tw.WriteHeader(&tar.Header{
+			Name:     name,
+			Typeflag: tar.TypeReg,
+			Mode:     0o644,
+			Size:     int64(len(contents)),
+		}))
+		if contents != "" {
+			_, err := tw.Write([]byte(contents))
+			assert.NilError(t, err)
+		}
+	}
+	writeFile("dir/file1", "one")
+	writeFile(".wh.dir", "")
+	writeFile("dir/file2", "two")
+	assert.NilError(t, tw.Close())
+
+	_, err := ApplyUncompressedLayer(dest, &buf, &TarOptions{NoLchown: true})
+	assert.NilError(t, err)
+
+	_, err = os.Stat(filepath.Join(dest, "dir", "file1"))
+	assert.ErrorIs(t, err, os.ErrNotExist)
+	dt, err := os.ReadFile(filepath.Join(dest, "dir", "file2"))
+	assert.NilError(t, err)
+	assert.Equal(t, string(dt), "two")
+}
+
+func TestUnpackRecreatesImpliedDirectoryRemovedBetweenEntries(t *testing.T) {
+	tarData := makeTarWithFiles(t, "dir", 2)
+
+	for _, tc := range []struct {
+		name   string
+		unpack func(io.Reader, string) error
+	}{
+		{
+			name: "Unpack",
+			unpack: func(r io.Reader, dest string) error {
+				return Unpack(r, dest, &TarOptions{NoLchown: true})
+			},
+		},
+		{
+			name: "UnpackLayer",
+			unpack: func(r io.Reader, dest string) error {
+				_, err := UnpackLayer(dest, r, &TarOptions{NoLchown: true})
+				return err
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			dest := t.TempDir()
+			r := newPausedReader(tarData, 2*tarBlockSize)
+			defer r.Resume()
+
+			errCh := make(chan error, 1)
+			go func() {
+				errCh <- tc.unpack(r, dest)
+			}()
+
+			select {
+			case <-r.Paused():
+			case err := <-errCh:
+				t.Fatalf("unpack returned before the second entry: %v", err)
+			}
+			_, err := os.Stat(filepath.Join(dest, "dir", "file-0"))
+			assert.NilError(t, err)
+			assert.NilError(t, os.RemoveAll(filepath.Join(dest, "dir")))
+			r.Resume()
+
+			assert.NilError(t, <-errCh)
+			data, err := os.ReadFile(filepath.Join(dest, "dir", "file-1"))
+			assert.NilError(t, err)
+			assert.Equal(t, string(data), "fooo")
+		})
+	}
+}
+
+const tarBlockSize = 512
+
+type pausedReader struct {
+	data       []byte
+	pauseAt    int
+	pos        int
+	paused     chan struct{}
+	resume     chan struct{}
+	pauseOnce  sync.Once
+	resumeOnce sync.Once
+}
+
+func newPausedReader(data []byte, pauseAt int) *pausedReader {
+	return &pausedReader{
+		data:    data,
+		pauseAt: pauseAt,
+		paused:  make(chan struct{}),
+		resume:  make(chan struct{}),
+	}
+}
+
+func (r *pausedReader) Read(p []byte) (int, error) {
+	if r.pos == len(r.data) {
+		return 0, io.EOF
+	}
+	if r.pos >= r.pauseAt {
+		r.pauseOnce.Do(func() { close(r.paused) })
+		<-r.resume
+	}
+	if r.pos < r.pauseAt && r.pos+len(p) > r.pauseAt {
+		p = p[:r.pauseAt-r.pos]
+	}
+	n := copy(p, r.data[r.pos:])
+	r.pos += n
+	return n, nil
+}
+
+func (r *pausedReader) Paused() <-chan struct{} {
+	return r.paused
+}
+
+func (r *pausedReader) Resume() {
+	r.resumeOnce.Do(func() { close(r.resume) })
+}
+
 func TestUntarHardlinkToSymlink(t *testing.T) {
 	skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root")
 	for i, headers := range [][]*tar.Header{
@@ -1295,3 +1450,28 @@ func readFileFromArchive(t *testing.T, archive io.ReadCloser, name string, expec
 	assert.Check(t, err)
 	return string(content)
 }
+
+func makeTarWithFiles(tb testing.TB, parent string, numberOfFiles int) []byte {
+	tb.Helper()
+
+	fileData := []byte("fooo")
+	var buf bytes.Buffer
+	tw := tar.NewWriter(&buf)
+	for n := range numberOfFiles {
+		if err := tw.WriteHeader(&tar.Header{
+			Name:     fmt.Sprintf("%s/file-%d", parent, n),
+			Typeflag: tar.TypeReg,
+			Mode:     0o700,
+			Size:     int64(len(fileData)),
+		}); err != nil {
+			tb.Fatal(err)
+		}
+		if _, err := tw.Write(fileData); err != nil {
+			tb.Fatal(err)
+		}
+	}
+	if err := tw.Close(); err != nil {
+		tb.Fatal(err)
+	}
+	return buf.Bytes()
+}

diff.go

@@ -104,12 +104,13 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64,
 		// dstPath is the native (host-separator) form of the entry name,
 		// used at all filesystem boundaries (os.Root methods, fsRootPath).
 		// The tar-header name (hdr.Name) is POSIX, so convert it here.
-		dstPath, err := resolveArchivePath(root, filepath.FromSlash(hdr.Name))
+		resolvedPath, err := resolveArchivePath(root, filepath.FromSlash(hdr.Name))
 		if err != nil {
 			return 0, err
 		}
+		dstPath := resolvedPath.path
 		// Ensure that the parent directory exists.
-		if err := createImpliedDirectories(root, dstPath, options); err != nil {
+		if err := createImpliedDirectories(root, resolvedPath, options); err != nil {
 			return 0, err
 		}
 		if base := filepath.Base(dstPath); strings.HasPrefix(base, WhiteoutPrefix) {