diff --git a/internal/archive/extract.go b/internal/archive/extract.go index fe11a3a55..801a6efa8 100644 --- a/internal/archive/extract.go +++ b/internal/archive/extract.go @@ -156,6 +156,11 @@ func (r *Resolver) extract(ctx context.Context, format archives.Extractor, conte r.truncate("extraction stopped: the archive and those nested in it decompressed to far more than its size (possible decompression bomb)") return nil } + if err != nil && ctx.Err() == nil && len(r.files) > 0 { + // the stream broke partway (a truncated .tar.gz); what was read before that is still good + r.truncate(fmt.Sprintf("extraction stopped early: %v", err)) + return nil + } return err } diff --git a/internal/archive/extract_test.go b/internal/archive/extract_test.go index 3b174be08..70456a54f 100644 --- a/internal/archive/extract_test.go +++ b/internal/archive/extract_test.go @@ -9,6 +9,7 @@ import ( "fmt" "io" "io/fs" + "math/rand" "os" "path/filepath" "strings" @@ -344,6 +345,25 @@ func TestExtract_heldContentWrittenToDiskStaysUntilCleanup(t *testing.T) { assert.Zero(t, disk) } +func TestExtract_aTruncatedStreamKeepsWhatWasRead(t *testing.T) { + // a .tar.gz cut off partway breaks the stream for every entry after the cut; the ones before it are + // still good and must not be thrown away + noise := make([]byte, 64*1024) + rand.New(rand.NewSource(1)).Read(noise) + data := tarGzBytes(t, map[string]string{"a.txt": "first", "b.bin": string(noise)}) + cut := data[:len(data)/2] + + extracted, err := Extract(context.Background(), bytes.NewReader(cut), "", "app.tar.gz", NewLimiter(unboundedLimits), nil) + require.NoError(t, err) + require.NotNil(t, extracted) + t.Cleanup(extracted.Cleanup) + + assert.True(t, extracted.Truncated) + locations, err := extracted.FilesByGlob("**") + require.NoError(t, err) + assert.Equal(t, []string{"a.txt"}, realPaths(locations)) +} + func TestExtract_aPanicStillReleasesEverything(t *testing.T) { // a decoder panicking on hostile bytes must not leak the charge or the spill file ctx, root := scanContext(t) diff --git a/internal/archive/resolver.go b/internal/archive/resolver.go index 44c6d3f01..83f3e0efc 100644 --- a/internal/archive/resolver.go +++ b/internal/archive/resolver.go @@ -167,10 +167,12 @@ func (r *Resolver) add(hdr tar.Header, content io.Reader) error { r.truncate(fmt.Sprintf("an entry larger than %s was skipped", humanize.IBytes(uint64(maxEntryBytes)))) return nil } - if !errors.Is(err, ErrDiskLimitReached) { - return fmt.Errorf("unable to read archive entry %q: %w", hdr.Name, err) + if errors.Is(err, ErrDiskLimitReached) || errors.Is(err, errBudgetSpent) { + return err } - return err + // a damaged entry (a CRC mismatch, a corrupt deflate stream) costs only that entry + r.truncate(fmt.Sprintf("entry %q could not be read: %v", hdr.Name, err)) + return nil } } diff --git a/internal/task/archive_tasks_test.go b/internal/task/archive_tasks_test.go index 24aa36c2e..71e376dbd 100644 --- a/internal/task/archive_tasks_test.go +++ b/internal/task/archive_tasks_test.go @@ -221,34 +221,29 @@ func Test_archiveCataloger_truncationStillCatalogs(t *testing.T) { assert.Contains(t, reasons[0], ArchiveCatalogerTaskName, "the unknown names the task that recorded it") } -func Test_archiveCataloger_failureIsRecordedAndSkipsSubPipeline(t *testing.T) { +func Test_archiveCataloger_aDamagedEntryCostsOnlyThatEntry(t *testing.T) { rootDir := t.TempDir() - // entry data overwritten but header and end-of-central-directory intact: detected as an archive, - // fails when the entry is read - corrupt := makeZip(t, map[string][]byte{"data.bin": incompressible(4096)}) + // entry data overwritten but headers and end-of-central-directory intact: detected as an archive, + // and data.bin fails when it is read. ok.txt comes after it and is intact. + corrupt := makeZip(t, map[string][]byte{"data.bin": incompressible(4096), "ok.txt": []byte("ok")}) require.Greater(t, len(corrupt), 1024) for i := 100; i < 600; i++ { corrupt[i] ^= 0xff } require.NoError(t, os.WriteFile(filepath.Join(rootDir, "corrupt.zip"), corrupt, 0o600)) - goodZip := makeZip(t, map[string][]byte{"ok.txt": []byte("ok")}) - require.NoError(t, os.WriteFile(filepath.Join(rootDir, "good.zip"), goodZip, 0o600)) + var seen []string + fileCounts := map[string]int{} + tsk := newTestTask(t, cataloging.DefaultArchiveSearchConfig().WithMaxDepth(1), capturingTask(t, &seen, &fileCounts)) - var ran int - tsk := newTestTask(t, cataloging.DefaultArchiveSearchConfig().WithMaxDepth(1), countingTask(&ran)) + s := newTestSBOM() + require.NoError(t, tsk.Execute(context.Background(), dirTestResolver{dir: rootDir}, sbomsync.NewBuilder(s))) - err := tsk.Execute(context.Background(), dirTestResolver{dir: rootDir}, sbomsync.NewBuilder(newTestSBOM())) - - // a coordinate error is what executor.go pulls out into sbom.Artifacts.Unknowns; a plain error would - // fail the scan instead - require.Error(t, err) - coordErrs, remaining := unknown.ExtractCoordinateErrors(err) - assert.NoError(t, remaining, "nothing may escape as a non-coordinate error") - require.Len(t, coordErrs, 1) - assert.Equal(t, "/corrupt.zip", coordErrs[0].Coordinates.RealPath) - - assert.Equal(t, 1, ran, "the sub-pipeline must still run for the archive that extracted cleanly") + assert.Equal(t, []string{"/corrupt.zip"}, seen, "the archive is still cataloged") + assert.Equal(t, 1, fileCounts["/corrupt.zip"], "from the entries that could be read") + reasons := s.Artifacts.Unknowns[file.Coordinates{RealPath: "/corrupt.zip"}] + require.Len(t, reasons, 1) + assert.Contains(t, reasons[0], `entry "data.bin" could not be read`) } func Test_NewArchiveCatalogerTask_gating(t *testing.T) {