From c28eb90cd90f5ce61cd7dab98461bea34864d979 Mon Sep 17 00:00:00 2001 From: deveshctl Date: Sat, 3 Oct 2026 11:01:48 +0530 Subject: [PATCH 1/3] fix(image): reject missing layer blobs, preserve explicit dir metadata, fix hardlink waste Three correctness fixes to the layer analysis pipeline: 1. parseLayers now returns an error when a manifest-referenced layer blob is absent from the outer tar. Previously the missing layer was silently treated as an empty tree, which could cause CI efficiency rules to pass on genuinely incomplete input. 2. Implicit parent directory nodes (created by insertNode when a tar entry lacks an explicit header for its ancestors) are now marked Inferred. mergeLayerWith skips metadata propagation from inferred nodes, so explicit mode/UID/GID set by an earlier layer is no longer overwritten by a structural placeholder. 3. Hardlink-aliased content is no longer counted as wasted bytes when the original filename is deleted but a hardlink pointing at the same payload survives in the final image. computeEfficiency now builds a set of surviving hardlink targets from the final stacked snapshot and skips waste charging for any deleted path whose content remains reachable through an alias. --- CHANGELOG.md | 3 +++ image/docker.go | 7 ++++++ image/docker_test.go | 23 +++++++++++++++++ image/efficiency.go | 53 +++++++++++++++++++++++++--------------- image/efficiency_test.go | 35 ++++++++++++++++++++++++++ image/filetree.go | 4 +++ image/stack.go | 4 +-- image/stack_test.go | 50 +++++++++++++++++++++++++++++++++++++ image/tree_parser.go | 11 ++++++--- 9 files changed, 164 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f8783c..e594667 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `FormatBytes` now returns a sign-prefixed string for negative inputs instead of silently wrapping to a large positive value via an unchecked `uint64` cast. - Daemon file-viewer (`DockerExtractor.Extract`) now correctly marks the returned `FileContent` as truncated when the extracted file exceeds the 1 MB view limit. Previously the flag was always false for daemon-extracted files, suppressing the truncation notice in the viewer. - `ErrNoEngineFound.Cause` field and its `Unwrap()` method removed; neither construction site ever populated the field, so `errors.Unwrap` always returned `nil`, making the method misleading. +- Archive analysis now rejects an image archive where a manifest-referenced layer blob is absent from the outer tar. Previously the missing layer was silently treated as empty, which could cause CI rules to pass on incomplete input. +- Directory metadata (mode, UID, GID) set by an earlier layer is no longer overwritten by an implicit parent directory node created when a later layer adds a child without including an explicit directory header. The placeholder node is now marked inferred and skipped during metadata merging. +- Hardlink-aliased content is no longer counted as wasted bytes when the original filename is deleted but a hardlink pointing at the same payload survives in the final image. Previously the deleted name's bytes were charged as waste even though the payload remained reachable. ## [v1.6.1] - 2026-08-08 diff --git a/image/docker.go b/image/docker.go index b25e9fe..1f4a73c 100644 --- a/image/docker.go +++ b/image/docker.go @@ -658,6 +658,13 @@ func parseLayers(ctx context.Context, r io.Reader) ([]Layer, error) { layers[idx].Tree = tree } + for i, l := range layers { + if l.Tree == nil && l.Size > 0 { + return nil, fmt.Errorf("layer %d (%s) is referenced in the manifest but absent from the archive", + i, l.ID) + } + } + return layers, nil } diff --git a/image/docker_test.go b/image/docker_test.go index dd1c245..7499676 100644 --- a/image/docker_test.go +++ b/image/docker_test.go @@ -964,3 +964,26 @@ func TestEnsureImage_PlatformMissing_ContainerdStore_NotMisclassifiedAsImageNotF // pinning to exactly one inspect locks the win in. assert.Equal(t, 1, inspectCalls, "platform classifier should reach enumeratePlatforms directly, no probe inspect needed") } + +func TestParseLayers_MissingLayerBlob_ReturnsError(t *testing.T) { + manifest := []dockerManifest{{ + Config: "config.json", + Layers: []string{"layer0/layer.tar", "layer1/layer.tar"}, + }} + manifestData, err := json.Marshal(manifest) + require.NoError(t, err) + + configData := buildConfig(t, []string{"RUN step0", "RUN step1"}) + + // Only layer0 is present; layer1 is absent from the archive. + tarBuf := buildTar(t, map[string][]byte{ + "manifest.json": manifestData, + "config.json": configData, + "layer0/layer.tar": make([]byte, 1024), + }) + + _, err = parseLayers(context.Background(), tarBuf) + require.Error(t, err) + assert.Contains(t, err.Error(), "layer 1") + assert.Contains(t, err.Error(), "absent from the archive") +} diff --git a/image/efficiency.go b/image/efficiency.go index 4839088..5e1c4c3 100644 --- a/image/efficiency.go +++ b/image/efficiency.go @@ -1,6 +1,9 @@ package image -import "sort" +import ( + "sort" + "strings" +) type WastedFile struct { Path string @@ -69,10 +72,6 @@ type pathRun struct { } func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { - // Build a path→FileNode index per stacked snapshot once. pathRuns then does - // O(1) lookups instead of recursing through the tree once per (path, - // snapshot) pair, which restored the analysis from quadratic to linear in - // total file count for layered images with many shared paths. indices := make([]map[string]*FileNode, len(stacked)) for i, tree := range stacked { if tree == nil || tree.Root == nil { @@ -83,6 +82,15 @@ func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { indices[i] = idx } + // Build the set of Linkname values for hardlinks that survive in the final + // stacked snapshot. A deleted path whose content is still reachable via a + // surviving hardlink alias is not truly wasted — the payload stays in the + // image and can be read through the alias. + survivingHardlinkTargets := make(map[string]struct{}) + if n := len(stacked); n > 0 && stacked[n-1] != nil && stacked[n-1].Root != nil { + walkSurvivingHardlinks(stacked[n-1].Root, survivingHardlinkTargets) + } + paths := make(map[string]struct{}) for _, layer := range layers { if layer.Tree == nil || layer.Tree.Root == nil { @@ -101,31 +109,20 @@ func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { var pathWaste int64 var occurrenceCount int for _, run := range runs { - // A run that ended in deletion ships every one of its copies with - // nothing surviving into the final image, so all occurrences are - // waste. A run still live at the top of the stack keeps its last - // occurrence (the copy present in the image); only the earlier, - // shadowed copies are waste. charged := run.occ if !run.endedInDeletion { if len(run.occ) < 2 { - // Single live occurrence with no prior copy in this run: no - // waste, and the path's reinstall copy must not inflate - // LayerCount for waste entries produced by earlier deleted runs. continue } charged = run.occ[:len(run.occ)-1] + } else if _, alive := survivingHardlinkTargets[path]; alive { + // The path was deleted, but a hardlink alias pointing at it + // survives in the final tree — the payload is still live. + continue } for _, occ := range charged { pathWaste += occ.size } - // LayerCount counts byte-contributing occurrences across charged - // copies only. For deleted runs, every occurrence is charged. For - // live runs, the surviving last copy is excluded from charged but - // is still a real byte-contributor visible to the user, so include - // all non-zero occurrences in the run (not just the charged slice). - // The single-occurrence live run above is skipped entirely, so - // reinstalled copies from a separate run do not inflate the count. for _, occ := range run.occ { if occ.size > 0 { occurrenceCount++ @@ -288,3 +285,19 @@ func walkFiles(node *FileNode, fn func(path string, size int64)) { } } } + +// walkSurvivingHardlinks collects the Linkname (target path) of every +// non-removed hardlink in the tree into targets. Used to avoid charging +// deleted original paths as waste when a hardlink alias still references them. +func walkSurvivingHardlinks(node *FileNode, targets map[string]struct{}) { + for _, child := range node.Children { + if isWhiteoutName(child.Name) || child.DiffType == Removed { + continue + } + if child.IsDir { + walkSurvivingHardlinks(child, targets) + } else if child.IsHardlink && child.Linkname != "" { + targets["/"+strings.TrimPrefix(child.Linkname, "/")] = struct{}{} + } + } +} diff --git a/image/efficiency_test.go b/image/efficiency_test.go index b334c65..93ef57e 100644 --- a/image/efficiency_test.go +++ b/image/efficiency_test.go @@ -429,3 +429,38 @@ func TestEfficiency_Golden(t *testing.T) { assert.Equal(t, int64(300), result.WastedFiles[1].TotalWasted) assert.Equal(t, 2, result.WastedFiles[1].LayerCount) } + +// Deleting the original filename of a hardlink pair must not be counted as +// waste when a surviving alias still holds the payload in the final image. +func TestEfficiency_HardlinkAlias_DeletedOriginal_NotWasted(t *testing.T) { + // Layer 0: /data/a is a 1 MiB regular file; /data/b is a hardlink → data/a. + hardlink := &FileNode{ + Name: "b", + Path: "/data/b", + Size: 0, + IsHardlink: true, + Linkname: "data/a", + } + layer0 := makeTree( + makeDir("data", "/data", + makeFile("a", "/data/a", 1<<20), + hardlink, + ), + ) + // Layer 1: .wh.a deletes /data/a; /data/b (the alias) survives. + layer1 := makeTree( + makeDir("data", "/data", + makeFile(".wh.a", "/data/.wh.a", 0), + ), + ) + + layers := []Layer{ + {Index: 0, Tree: layer0}, + {Index: 1, Tree: layer1}, + } + result := Efficiency(layers) + assert.Equal(t, int64(0), result.WastedBytes, + "payload is still reachable via /data/b — must not be charged as waste") + assert.Empty(t, result.WastedFiles) + assert.Equal(t, 1.0, result.Score) +} diff --git a/image/filetree.go b/image/filetree.go index b47f97b..1badbfa 100644 --- a/image/filetree.go +++ b/image/filetree.go @@ -33,6 +33,10 @@ type FileNode struct { Children []*FileNode IsDir bool IsHardlink bool + // Inferred is true for directory nodes created implicitly by insertNode + // because no explicit tar header existed for them. Their Mode/UID/GID are + // structural defaults, not authoritative image metadata. + Inferred bool } func NewFileTree() *FileTree { diff --git a/image/stack.go b/image/stack.go index 54468d7..0f5bce5 100644 --- a/image/stack.go +++ b/image/stack.go @@ -66,9 +66,9 @@ func mergeLayerWith(cumulative, layerRoot *FileNode, layerIdx int, carry carryFo IntroducedInLayer: cumulative.IntroducedInLayer, } - metadataChanged := cumulative.Mode != layerRoot.Mode || + metadataChanged := !layerRoot.Inferred && (cumulative.Mode != layerRoot.Mode || cumulative.UID != layerRoot.UID || - cumulative.GID != layerRoot.GID + cumulative.GID != layerRoot.GID) if metadataChanged { merged.Mode = layerRoot.Mode merged.UID = layerRoot.UID diff --git a/image/stack_test.go b/image/stack_test.go index 21453a8..9aa7388 100644 --- a/image/stack_test.go +++ b/image/stack_test.go @@ -1,6 +1,8 @@ package image import ( + "archive/tar" + "bytes" "io/fs" "testing" @@ -954,3 +956,51 @@ func TestBuildAggregatedTrees_EmptyLayerSnapshotsBaseline(t *testing.T) { require.NotNil(t, b2) assert.Equal(t, Added, b2.DiffType) } + +// An implicit parent directory node must not overwrite the real metadata an +// earlier layer recorded for that directory. +func TestStack_ImplicitParentDoesNotOverwriteMetadata(t *testing.T) { + // Layer 0: explicit /app dir header with mode 0700 and uid/gid 1000. + var buf0 bytes.Buffer + tw0 := tar.NewWriter(&buf0) + require.NoError(t, tw0.WriteHeader(&tar.Header{ + Typeflag: tar.TypeDir, + Name: "app/", + Mode: 0700, + Uid: 1000, + Gid: 1000, + })) + require.NoError(t, tw0.Close()) + tree0, err := ParseLayerTar(&buf0) + require.NoError(t, err) + + // Layer 1: only app/new.txt — no explicit app/ header, so insertNode + // creates an inferred placeholder for app/ with mode 0755 and uid/gid 0. + var buf1 bytes.Buffer + tw1 := tar.NewWriter(&buf1) + require.NoError(t, tw1.WriteHeader(&tar.Header{ + Typeflag: tar.TypeReg, + Name: "app/new.txt", + Size: 10, + Mode: 0644, + })) + _, err = tw1.Write(make([]byte, 10)) + require.NoError(t, err) + require.NoError(t, tw1.Close()) + tree1, err := ParseLayerTar(&buf1) + require.NoError(t, err) + + layers := []Layer{ + {Index: 0, Tree: tree0}, + {Index: 1, Tree: tree1}, + } + stacked := Stack(layers) + require.Len(t, stacked, 2) + + app := stacked[1].Root.FindChild("app") + require.NotNil(t, app) + assert.Equal(t, fs.ModeDir|fs.FileMode(0700), app.Mode, + "layer 0's explicit mode 0700 must survive the implicit parent in layer 1") + assert.Equal(t, 1000, app.UID, "UID must not be reset to 0 by the inferred placeholder") + assert.Equal(t, 1000, app.GID, "GID must not be reset to 0 by the inferred placeholder") +} diff --git a/image/tree_parser.go b/image/tree_parser.go index 7883489..4e5be67 100644 --- a/image/tree_parser.go +++ b/image/tree_parser.go @@ -103,6 +103,7 @@ func insertNode(root *FileNode, fullPath string, size int64, isDir, isHardlink b existing.GID = gid existing.IsHardlink = isHardlink existing.Linkname = linkname + existing.Inferred = false } else { node := &FileNode{ Name: part, @@ -121,10 +122,11 @@ func insertNode(root *FileNode, fullPath string, size int64, isDir, isHardlink b if existing == nil { dirPath := "/" + path.Join(parts[:i+1]...) existing = &FileNode{ - Name: part, - Path: dirPath, - IsDir: true, - Mode: fs.ModeDir | 0755, + Name: part, + Path: dirPath, + IsDir: true, + Mode: fs.ModeDir | 0755, + Inferred: true, } current.AddChild(existing) } else if !existing.IsDir { @@ -137,6 +139,7 @@ func insertNode(root *FileNode, fullPath string, size int64, isDir, isHardlink b existing.Linkname = "" existing.IsHardlink = false existing.Mode = fs.ModeDir | 0755 + existing.Inferred = true } current = existing } From 89696171b9a0bba9196c0a8c0f8eec53707a7c00 Mon Sep 17 00:00:00 2001 From: deveshctl Date: Sat, 3 Oct 2026 11:16:27 +0530 Subject: [PATCH 2/3] fix(image): normalize Linkname, propagate Inferred in clones, restore comments Follow-up to the layer analysis correctness fixes: - Apply cleanTarPath to hdr.Linkname in ParseLayerTar so hardlink target paths receive the same ./- and backslash-normalization as hdr.Name. Without this, images from Windows-native or some older build tools emit ./data/a as the linkname, which failed to match the stored /data/a key in walkSurvivingHardlinks and caused the deleted original to be incorrectly charged as waste. - Propagate Inferred in all four FileNode clone functions (cloneAsUnchanged, cloneAsRemoved, cloneAsAdded, cloneWithDiffType, cloneStructure). The field is only read from raw layer-side nodes today, but omitting it from clones would silently break any future reader inspecting the cumulative tree. - Restore the four explanatory comments removed during the hardlink-waste refactor in computeEfficiency: the O(1) index rationale and the LayerCount accounting rule. The LayerCount comment prevents a future reader from treating the run.occ loop as a copy-paste error. - Fix TestParseLayers_MissingLayerBlob_ReturnsError: replace the 1 KiB of raw zeros for layer0 with a valid empty tar so the test exercises the missing-blob guard specifically, not the corrupt-tar parse error. --- image/docker_test.go | 7 ++++++- image/efficiency.go | 19 +++++++++++++++++++ image/stack.go | 5 +++++ image/tree_parser.go | 2 +- 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/image/docker_test.go b/image/docker_test.go index 7499676..136c96a 100644 --- a/image/docker_test.go +++ b/image/docker_test.go @@ -975,11 +975,16 @@ func TestParseLayers_MissingLayerBlob_ReturnsError(t *testing.T) { configData := buildConfig(t, []string{"RUN step0", "RUN step1"}) + // Build a valid (empty) tar for layer0 so parseLayers succeeds Pass 2 for it; + // layer1 is absent entirely, which is what we want the new check to catch. + var layer0Buf bytes.Buffer + require.NoError(t, tar.NewWriter(&layer0Buf).Close()) + // Only layer0 is present; layer1 is absent from the archive. tarBuf := buildTar(t, map[string][]byte{ "manifest.json": manifestData, "config.json": configData, - "layer0/layer.tar": make([]byte, 1024), + "layer0/layer.tar": layer0Buf.Bytes(), }) _, err = parseLayers(context.Background(), tarBuf) diff --git a/image/efficiency.go b/image/efficiency.go index 5e1c4c3..4767f1a 100644 --- a/image/efficiency.go +++ b/image/efficiency.go @@ -72,6 +72,10 @@ type pathRun struct { } func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { + // Build a path→FileNode index per stacked snapshot once. pathRuns then does + // O(1) lookups instead of recursing through the tree once per (path, + // snapshot) pair, which keeps the analysis linear in total file count for + // layered images with many shared paths. indices := make([]map[string]*FileNode, len(stacked)) for i, tree := range stacked { if tree == nil || tree.Root == nil { @@ -109,9 +113,17 @@ func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { var pathWaste int64 var occurrenceCount int for _, run := range runs { + // A run that ended in deletion ships every one of its copies with + // nothing surviving into the final image, so all occurrences are + // waste. A run still live at the top of the stack keeps its last + // occurrence (the copy present in the image); only the earlier, + // shadowed copies are waste. charged := run.occ if !run.endedInDeletion { if len(run.occ) < 2 { + // Single live occurrence with no prior copy in this run: no + // waste, and the path's reinstall copy must not inflate + // LayerCount for waste entries produced by earlier deleted runs. continue } charged = run.occ[:len(run.occ)-1] @@ -123,6 +135,13 @@ func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { for _, occ := range charged { pathWaste += occ.size } + // LayerCount counts byte-contributing occurrences across charged + // copies only. For deleted runs, every occurrence is charged. For + // live runs, the surviving last copy is excluded from charged but + // is still a real byte-contributor visible to the user, so include + // all non-zero occurrences in the run (not just the charged slice). + // The single-occurrence live run above is skipped entirely, so + // reinstalled copies from a separate run do not inflate the count. for _, occ := range run.occ { if occ.size > 0 { occurrenceCount++ diff --git a/image/stack.go b/image/stack.go index 0f5bce5..3cd2e5c 100644 --- a/image/stack.go +++ b/image/stack.go @@ -222,6 +222,7 @@ func cloneAsUnchanged(node *FileNode) *FileNode { Size: node.Size, IsDir: node.IsDir, IsHardlink: node.IsHardlink, + Inferred: node.Inferred, DiffType: Unchanged, Mode: node.Mode, UID: node.UID, @@ -242,6 +243,7 @@ func cloneAsRemoved(node *FileNode) *FileNode { Size: node.Size, IsDir: node.IsDir, IsHardlink: node.IsHardlink, + Inferred: node.Inferred, DiffType: Removed, Mode: node.Mode, UID: node.UID, @@ -262,6 +264,7 @@ func cloneAsAdded(node *FileNode, layerIdx int) *FileNode { Size: node.Size, IsDir: node.IsDir, IsHardlink: node.IsHardlink, + Inferred: node.Inferred, DiffType: Added, Mode: node.Mode, UID: node.UID, @@ -287,6 +290,7 @@ func cloneStructure(node *FileNode) *FileNode { Size: node.Size, IsDir: node.IsDir, IsHardlink: node.IsHardlink, + Inferred: node.Inferred, Mode: node.Mode, UID: node.UID, GID: node.GID, @@ -314,6 +318,7 @@ func cloneWithDiffType(node *FileNode) *FileNode { Size: node.Size, IsDir: node.IsDir, IsHardlink: node.IsHardlink, + Inferred: node.Inferred, DiffType: node.DiffType, Mode: node.Mode, UID: node.UID, diff --git a/image/tree_parser.go b/image/tree_parser.go index 4e5be67..e89c89a 100644 --- a/image/tree_parser.go +++ b/image/tree_parser.go @@ -49,7 +49,7 @@ func ParseLayerTar(r io.Reader) (*FileTree, error) { uid := hdr.Uid gid := hdr.Gid - insertNode(tree.Root, name, size, isDir, isHardlink, hdr.Linkname, mode, uid, gid) + insertNode(tree.Root, name, size, isDir, isHardlink, cleanTarPath(hdr.Linkname), mode, uid, gid) } return tree, nil From 2361fd19c299bab7f15bd671baa0a1fb03fe324d Mon Sep 17 00:00:00 2001 From: deveshctl Date: Sat, 3 Oct 2026 11:20:36 +0530 Subject: [PATCH 3/3] fix(image): detect absent layer blobs by manifest presence, not declared size The previous guard used l.Size > 0 (the declared tar header size) to distinguish a legitimately empty layer from a missing one. A layer absent from the archive entirely gets Size == 0 from a missing-key map lookup, so the check never fired and parseLayers returned nil error. Use the headers map from Pass 1 instead: every entry that appeared in the outer tar has a key there. A manifest-referenced path with no key in headers was never in the archive, regardless of its declared size. --- image/docker.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/image/docker.go b/image/docker.go index 1f4a73c..f560b2a 100644 --- a/image/docker.go +++ b/image/docker.go @@ -658,10 +658,10 @@ func parseLayers(ctx context.Context, r io.Reader) ([]Layer, error) { layers[idx].Tree = tree } - for i, l := range layers { - if l.Tree == nil && l.Size > 0 { + for i, layerPath := range manifest.Layers { + if _, present := headers[layerPath]; !present { return nil, fmt.Errorf("layer %d (%s) is referenced in the manifest but absent from the archive", - i, l.ID) + i, layers[i].ID) } }