diff --git a/archive.go b/archive.go index 689720a..e031e19 100644 --- a/archive.go +++ b/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,9 +491,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, 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 @@ -492,16 +501,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 +518,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 +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 { 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 b945018..df64791 100644 --- a/diff.go +++ b/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) {