From 43091f4cda8a157059e5d1f4b66199fb4590a37b Mon Sep 17 00:00:00 2001 From: deveshctl Date: Fri, 2 Oct 2026 16:06:34 +0530 Subject: [PATCH] fix(image): dead code, FormatBytes safety, and Truncated for daemon extraction - Remove ErrNoEngineFound.Cause and Unwrap(): the field was never set at either construction site, making Unwrap() always return nil. Callers match on the type, not the wrapped cause, so removing it makes the interface honest without breaking anything. - Guard FormatBytes against negative int64 inputs: the uint64 cast silently wrapped negatives to large positive values. Current callers pass non-negative sizes, but the contract was invisible. Returns a sign-prefixed string for negative inputs, consistent with FormatSignedBytes. - Fix Truncated flag suppression in DockerExtractor.Extract: readFirstFileFromTar truncated the stream to MaxViewSize before returning, so processContent always saw len(data) <= MaxViewSize and never set fc.Truncated. Now returns the declared tar header size alongside the data; the caller passes the larger of the stat size and the header size to processContent so the truncated notice is shown when a daemon-extracted file exceeds 1 MB. - Remove stale internal-docs reference from compare_regression_test.go: the file is gitignored and absent from a clean clone. --- CHANGELOG.md | 5 +++-- image/compare_regression_test.go | 2 -- image/errors.go | 3 --- image/extractor.go | 28 +++++++++++++++++++++------- image/format.go | 5 ++++- 5 files changed, 28 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9ceaf4..1f8783c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,8 +13,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Status bar "toggle / view" hint in split-pane mode now tracks the correct pane's cursor; previously it read the top pane's cursor position even when the bottom pane had focus. - File viewer (`readFirstFileFromTar`) now reads up to the full `MaxViewSize` limit regardless of the tar entry's declared size, consistent with the save-file path. A crafted archive with an understated size header no longer silently truncates the viewed content. - `layerx build` iidfile setup now surfaces a removal error instead of silently discarding it; on Windows a failed removal no longer leaves a zero-byte file that causes the engine to report an empty image ID. -- `ErrNoEngineFound` now implements `Unwrap()`, consistent with all other error types in the package. -- `FormatBytes` now delegates to the internal `formatUnsignedBytes` helper, removing ~15 lines of duplicated formatting logic. - CLI error message for `--engine podman` now mentions `DOCKER_HOST` alongside `CONTAINER_HOST`, matching the hint in the library error. - Usage synopsis now uses `IMAGE_OR_ARCHIVE` consistently where both image references and local archives are accepted. - `layerx compare` now uses the pre-computed stacked trees from the analysis for efficiency scoring, consistent with every other caller. Previously it re-stacked from raw layers, which could produce mismatched efficiency and file-diff results when the analysis carried custom stacked trees (e.g. from a cache path). @@ -23,6 +21,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The file-permission display in the layer browser now shows setuid (`s`/`S`), setgid (`s`/`S`), and sticky (`t`/`T`) bits. Previously, a file with mode `04755` was shown as `-rwxr-xr-x` instead of `-rwsr-xr-x`. +- `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. ## [v1.6.1] - 2026-08-08 diff --git a/image/compare_regression_test.go b/image/compare_regression_test.go index fcd680a..b2de1ac 100644 --- a/image/compare_regression_test.go +++ b/image/compare_regression_test.go @@ -1,8 +1,6 @@ package image // Regression tests for confirmed compare bugs. -// See internal-docs/compare-audit.md for full analysis. -// // All tests use in-process fixtures only — no Docker required. import ( diff --git a/image/errors.go b/image/errors.go index 58def8b..87dd043 100644 --- a/image/errors.go +++ b/image/errors.go @@ -134,7 +134,6 @@ func (e *ErrPodmanSocketNotSet) Error() string { type ErrNoEngineFound struct { Tried []string - Cause error } func (e *ErrNoEngineFound) Error() string { @@ -142,8 +141,6 @@ func (e *ErrNoEngineFound) Error() string { strings.Join(e.Tried, ", ")) } -func (e *ErrNoEngineFound) Unwrap() error { return e.Cause } - // ErrPlatformInvalid is returned when --platform cannot be parsed at all // (empty component, too many slashes). Distinct from ErrPlatformNotInImage: // the spec itself is malformed, no image lookup happened. diff --git a/image/extractor.go b/image/extractor.go index ef3b77a..b43fec7 100644 --- a/image/extractor.go +++ b/image/extractor.go @@ -145,10 +145,13 @@ func (e *DockerExtractor) Extract(ctx context.Context, imageRef string, filePath totalSize := copyResult.Stat.Size - data, err := readFirstFileFromTar(copyResult.Content) + data, declaredSize, err := readFirstFileFromTar(copyResult.Content) if err != nil { return nil, fmt.Errorf("failed to read %s: %w", filePath, err) } + if declaredSize > totalSize { + totalSize = declaredSize + } return processContent(filePath, data, totalSize), nil } @@ -178,19 +181,23 @@ func (e *DockerExtractor) ExtractRaw(ctx context.Context, imageRef string, fileP return readFullFileFromTar(copyResult.Content) } -// readFirstFileFromTar reads the first regular file from a tar stream. +// readFirstFileFromTar reads the first regular file from a tar stream, +// returning the (possibly truncated) data and the declared file size from the +// tar header. The declared size lets the caller set FileContent.Truncated when +// the stream exceeds MaxViewSize — without it, processContent always sees +// len(data) <= MaxViewSize and silently suppresses the truncated notice. // Docker's CopyFromContainer wraps the file in a single-entry tar. // Non-regular entries (directories, symlinks, hardlinks, devices, fifos) are // skipped — the contract is "read the first *regular file* in the stream". -func readFirstFileFromTar(r io.Reader) ([]byte, error) { +func readFirstFileFromTar(r io.Reader) ([]byte, int64, error) { tr := tar.NewReader(r) for { hdr, err := tr.Next() if errors.Is(err, io.EOF) { - return nil, fmt.Errorf("no file found in tar stream") + return nil, 0, fmt.Errorf("no file found in tar stream") } if err != nil { - return nil, err + return nil, 0, err } if hdr.Typeflag != tar.TypeReg { continue @@ -201,12 +208,19 @@ func readFirstFileFromTar(r io.Reader) ([]byte, error) { limit := int64(MaxViewSize + 1) data, err := io.ReadAll(io.LimitReader(tr, limit)) if err != nil { - return nil, err + return nil, 0, err + } + // Return the declared size from the header so the caller can set + // Truncated correctly. If the stream was larger than the header claimed, + // use the actual bytes read as the size floor. + declaredSize := hdr.Size + if int64(len(data)) > declaredSize { + declaredSize = int64(len(data)) } if int64(len(data)) > MaxViewSize { data = data[:MaxViewSize] } - return data, nil + return data, declaredSize, nil } } diff --git a/image/format.go b/image/format.go index b4a29c6..83dd5f3 100644 --- a/image/format.go +++ b/image/format.go @@ -24,7 +24,10 @@ func formatUnsignedBytes(b uint64) string { } func FormatBytes(b int64) string { - return formatUnsignedBytes(uint64(b)) //nolint:gosec // callers always pass non-negative sizes + if b < 0 { + return "-" + formatUnsignedBytes(uint64(-b)) + } + return formatUnsignedBytes(uint64(b)) } // FormatSignedBytes formats b like FormatBytes but with an explicit sign