From 98808a549f25c3a6b3f35ea3f4885c14913f1941 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 13 Aug 2026 13:52:08 +0200 Subject: [PATCH 1/2] archive: Return archive path resolution details MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Represent the result of resolveArchivePath as a resolvedArchivePath so callers can consume path-resolution details without adding parallel return values. Update existing callers to use the resolved path without changing extraction behavior. Signed-off-by: Paweł Gronowski --- archive.go | 31 ++++++++++++++++++++----------- diff.go | 3 ++- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/archive.go b/archive.go index 689720a..c42f146 100644 --- a/archive.go +++ b/archive.go @@ -453,6 +453,10 @@ func (ta *tarAppender) addTarFile(srcPath, archivePath string) error { return nil } +type resolvedArchivePath struct { + path string +} + // 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. @@ -469,10 +473,10 @@ func (ta *tarAppender) addTarFile(srcPath, archivePath string) error { // 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}, nil } parent = filepath.Clean(parent) @@ -482,9 +486,9 @@ func resolveArchivePath(root *os.Root, name string) (string, error) { _, statErr := root.Stat(parent) switch { case statErr == nil: - return name, nil + return resolvedArchivePath{path: name}, nil case !os.IsNotExist(statErr) && !isPathEscapes(statErr): - return "", statErr + return resolvedArchivePath{}, statErr } // Resolve the parent both to handle ENOENT from missing components or dangling @@ -492,16 +496,16 @@ func resolveArchivePath(root *os.Root, name string) (string, error) { // 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(), @@ -509,14 +513,14 @@ func resolveArchivePath(root *os.Root, name string) (string, error) { )) } 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 +538,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 +1042,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 diff --git a/diff.go b/diff.go index b945018..221ff36 100644 --- a/diff.go +++ b/diff.go @@ -104,10 +104,11 @@ 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 { return 0, err From 1de6a8800b905daec1f8cf12c160f2d9d7d9ff76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 13 Aug 2026 13:52:52 +0200 Subject: [PATCH 2/2] archive: Avoid duplicate implied-parent checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveArchivePath already checks each entry's parent before createImpliedDirectories checks the same path again. Archives containing many files therefore perform a redundant filesystem lookup for every entry whose parent already exists. Record that observation in resolvedArchivePath and reuse it only while processing the current entry. This removes the duplicate lookup without caching filesystem state across entries, so a parent removed during extraction is observed and recreated for the next entry. Co-authored-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> Signed-off-by: Paweł Gronowski --- archive.go | 22 +++--- archive_test.go | 180 ++++++++++++++++++++++++++++++++++++++++++++++++ diff.go | 2 +- 3 files changed, 194 insertions(+), 10 deletions(-) diff --git a/archive.go b/archive.go index c42f146..e031e19 100644 --- a/archive.go +++ b/archive.go @@ -454,7 +454,8 @@ func (ta *tarAppender) addTarFile(srcPath, archivePath string) error { } type resolvedArchivePath struct { - path string + path string + parentWasPresent bool } // resolveArchivePath resolves intermediate symlinks in name using chroot-like @@ -470,13 +471,17 @@ type resolvedArchivePath struct { // 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) (resolvedArchivePath, error) { parent, base := filepath.Split(name) if parent == "" { - return resolvedArchivePath{path: name}, nil + return resolvedArchivePath{path: name, parentWasPresent: true}, nil } parent = filepath.Clean(parent) @@ -486,7 +491,7 @@ func resolveArchivePath(root *os.Root, name string) (resolvedArchivePath, error) _, statErr := root.Stat(parent) switch { case statErr == nil: - return resolvedArchivePath{path: name}, nil + return resolvedArchivePath{path: name, parentWasPresent: true}, nil case !os.IsNotExist(statErr) && !isPathEscapes(statErr): return resolvedArchivePath{}, statErr } @@ -1084,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 } @@ -1145,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 { diff --git a/archive_test.go b/archive_test.go index 5948b4d..29e9bdc 100644 --- a/archive_test.go +++ b/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 --git a/diff.go b/diff.go index 221ff36..df64791 100644 --- a/diff.go +++ b/diff.go @@ -110,7 +110,7 @@ func UnpackLayer(dest string, layer io.Reader, options *TarOptions) (size int64, } 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) {