diff --git a/.limen/just/test-go.just b/.limen/just/test-go.just index 2d246c13..9631be91 100644 --- a/.limen/just/test-go.just +++ b/.limen/just/test-go.just @@ -56,6 +56,71 @@ bench: (_banner "test go" "bench") CGO_ENABLED="{{ go_cgo }}" \ go test -count=1 -timeout "${TEST_GO_TIMEOUT:-10m}" -run '^$' -bench . -benchmem ./... +# Fuzz smoke: every Fuzz* target in every package, each for a short budget — +# a regression net, not a campaign. The seed corpus of the target runs first +# (that is what -run selects), then the engine mutates for TEST_GO_FUZZ_TIME +# (default 10s; export from the root Justfile to raise it). Driven by +# `go test -fuzz` itself, never a prebuilt test binary: only the go tool's +# fuzz build compiles in the coverage counters, and without them the engine +# mutates blind. `go test -fuzz` accepts one target per invocation, hence the +# loop; targets are discovered with -list rather than grep so build tags and +# _test.go placement are Go's call, not a regex's. +# +# The verdict comes from the crasher, not the exit code. A real finding always +# writes the failing input under the package's testdata/fuzz// (commit +# it — it is a regression test from then on). But the coordinator can also +# exit 1 with "context deadline exceeded" when a worker is mid-iteration as +# fuzztime expires — a shutdown hiccup, not a finding, and slow targets hit it +# most. So: exit 1 WITH a new testdata file is a failure; exit 1 without one +# is retried once, and a second in a row is treated as real. The generated +# corpus lives under $(go env GOCACHE)/fuzz — cache that between CI runs and +# fuzzing becomes cumulative. A tree with no fuzz targets says so and passes. +fuzz: (_banner "test go" "fuzz") + #!/usr/bin/env bash + set -euo pipefail + export CGO_ENABLED="{{ go_cgo }}" + fuzztime="${TEST_GO_FUZZ_TIME:-10s}" + # Crasher count for a target: the directory does not exist until the first + # finding, and a missing directory is zero, not an error (pipefail). + crashers() { + if [ -d "$1" ]; then find "$1" -type f | wc -l; else echo 0; fi + } + found=0 + fail=0 + for pkg in $(go list ./...); do + targets=$(go test -count=1 -run '^$' -list '^Fuzz' "$pkg" | grep '^Fuzz' || true) + [ -n "$targets" ] || continue + dir=$(go list -f '{{{{.Dir}}' "$pkg") + for target in $targets; do + found=1 + echo "fuzzing $pkg $target for $fuzztime" + before=$(crashers "$dir/testdata/fuzz/$target") + ok=0 + for attempt in 1 2; do + if go test -count=1 -timeout "${TEST_GO_TIMEOUT:-10m}" \ + -run "^${target}\$" -fuzz "^${target}\$" -fuzztime "$fuzztime" "$pkg"; then + ok=1 + break + fi + after=$(crashers "$dir/testdata/fuzz/$target") + if [ "$after" -gt "$before" ]; then + echo "$target: new crasher written under $dir/testdata/fuzz/$target — commit it as a regression test" >&2 + break + fi + echo "$target: exit without a crasher (attempt $attempt) — coordinator shutdown hiccup, retrying" >&2 + done + if [ "$ok" -eq 1 ]; then + echo "PASS: $target" + else + fail=1 + fi + done + done + if [ "$found" -eq 0 ]; then + echo "no Fuzz* targets in this module — nothing to fuzz" + fi + exit "$fail" + # Coverage: per-function summary, an HTML report under build/coverage/, and an # optional minimum gate — export TEST_GO_COVER_MIN := '80' (integer percent) # from the root Justfile to enforce a floor; unset or 0 reports without gating. diff --git a/aqua-checksums.json b/aqua-checksums.json index 04d5fc86..b975ef14 100644 --- a/aqua-checksums.json +++ b/aqua-checksums.json @@ -46,28 +46,28 @@ "algorithm": "sha256" }, { - "id": "github_release/github.com/farcloser/limen/v0.0.12/limen_0.0.12_darwin_arm64.tar.gz", - "checksum": "EDCA1AF957C0F14178F6B29D473EC899F479BB3AF2A240020FAD5965F044AC99", + "id": "github_release/github.com/farcloser/limen/v0.0.13/limen_0.0.13_darwin_arm64.tar.gz", + "checksum": "8739EA67AE2404A33E56EC9E89DE228C4DB73BFE37E2E814D7592289576FDBF8", "algorithm": "sha256" }, { - "id": "github_release/github.com/farcloser/limen/v0.0.12/limen_0.0.12_linux_amd64.tar.gz", - "checksum": "551BC67781A4FD18F1941A9BE68320DB730B982DA62BBF41988F88E721B5EE65", + "id": "github_release/github.com/farcloser/limen/v0.0.13/limen_0.0.13_linux_amd64.tar.gz", + "checksum": "C5A3990CF1307CB14DA8986FF45AD634A569DD35B3E536F7694C7BD9E9BF4557", "algorithm": "sha256" }, { - "id": "github_release/github.com/farcloser/limen/v0.0.12/limen_0.0.12_linux_arm64.tar.gz", - "checksum": "13C508CC4ECE232D033AA4E336DA96EFAE35E6C247F7BFE52F8E38824EC502B1", + "id": "github_release/github.com/farcloser/limen/v0.0.13/limen_0.0.13_linux_arm64.tar.gz", + "checksum": "6D19477293359E262C2E6E882974355B1F339A5249EBC1D42620783BBD23B3B0", "algorithm": "sha256" }, { - "id": "github_release/github.com/farcloser/limen/v0.0.12/limen_0.0.12_windows_amd64.tar.gz", - "checksum": "3CFE919FF7CBD49C0DACA0397B5E76330861C96CE4575E997B9D3C4833AE05E0", + "id": "github_release/github.com/farcloser/limen/v0.0.13/limen_0.0.13_windows_amd64.tar.gz", + "checksum": "942522F4AB937B9DDDCFE2E369E2AF13AF64C1041934CA20D098AA9C8321FC20", "algorithm": "sha256" }, { - "id": "github_release/github.com/farcloser/limen/v0.0.12/limen_0.0.12_windows_arm64.tar.gz", - "checksum": "8DA23959424BABC09D0AED2798F1E3393A7DC2F594AE4BC8A098BADDAB03F210", + "id": "github_release/github.com/farcloser/limen/v0.0.13/limen_0.0.13_windows_arm64.tar.gz", + "checksum": "C331B8BA5495406BC1A5C6DB62549B4AB562FDD808DFB71B18D3FD6ADE3FCE42", "algorithm": "sha256" }, { diff --git a/aqua.yaml b/aqua.yaml index 9ea8106e..95d59148 100644 --- a/aqua.yaml +++ b/aqua.yaml @@ -32,7 +32,7 @@ packages: - name: github.com/farcloser/godolint/cmd/godolint@v0.1.0 registry: local # --- farcloser tools (local registry; standard once registered upstream) --- - - name: farcloser/limen@v0.0.12 # renovate: depName=farcloser/limen + - name: farcloser/limen@v0.0.13 # renovate: depName=farcloser/limen registry: local # --- toolchain + binary-release tools (standard registry, aqua-verified) --- - name: golang/go@go1.26.5 diff --git a/lzma/reader.go b/lzma/reader.go index dde100fe..55706649 100644 --- a/lzma/reader.go +++ b/lzma/reader.go @@ -11,6 +11,7 @@ package lzma import ( + "bufio" "errors" "fmt" "io" @@ -67,6 +68,18 @@ func (c *ReaderConfig) Verify() error { // the minimum dictionary size. This is another measure to prevent huge // memory allocations for the dictionary. // - The code supports stream sizes only up to a pebibyte (1024^5). +// +// # Input buffering +// +// The decoder pulls compressed bytes one at a time. If the input is an +// [io.ByteReader] — a [bufio.Reader], [bytes.Reader] or [strings.Reader], +// say — it is used as is and the reader consumes exactly the bytes of the +// stream, leaving anything after it unread. Any other [io.Reader] is wrapped +// in a [bufio.Reader] first: without that, a plain [os.File] costs one system +// call per compressed byte and decodes an order of magnitude slower. The +// wrapper may read past the end of the stream, so a caller who needs the +// input positioned exactly at the stream's end should pass an io.ByteReader +// of its own. type Reader struct { lzma io.Reader header Header @@ -117,6 +130,14 @@ func (c ReaderConfig) NewReader(lzma io.Reader) (r *Reader, err error) { if err = c.Verify(); err != nil { return nil, err } + // The decoder reads compressed bytes one at a time through an + // io.ByteReader. Give a plain reader one buffered front-end for the + // header and the stream alike; an input that already is a ByteReader + // is left alone so its exact position stays under the caller's control + // (see the Reader documentation). + if _, ok := lzma.(io.ByteReader); !ok { + lzma = bufio.NewReader(lzma) + } data := make([]byte, HeaderLen) if _, err := io.ReadFull(lzma, data); err != nil { if err == io.EOF { diff --git a/lzma/reader_input_test.go b/lzma/reader_input_test.go new file mode 100644 index 00000000..da3fcabb --- /dev/null +++ b/lzma/reader_input_test.go @@ -0,0 +1,176 @@ +package lzma + +import ( + "bufio" + "bytes" + "io" + "os" + "testing" +) + +// plainReader is an io.Reader that is deliberately not an io.ByteReader, and +// counts the bytes pulled from it. +type plainReader struct { + r io.Reader + n int +} + +func (p *plainReader) Read(b []byte) (int, error) { + n, err := p.r.Read(b) + p.n += n + return n, err +} + +func lzmaEnwik(tb testing.TB, size int) (compressed, plain []byte) { + tb.Helper() + plain, err := os.ReadFile("../testdata/enwik7") + if err != nil { + tb.Skip("testdata/enwik7 not available") + } + if len(plain) > size { + plain = plain[:size] + } + var buf bytes.Buffer + w, err := NewWriter(&buf) + if err != nil { + tb.Fatal(err) + } + if _, err := w.Write(plain); err != nil { + tb.Fatal(err) + } + if err := w.Close(); err != nil { + tb.Fatal(err) + } + return buf.Bytes(), plain +} + +// TestReaderInputConsumption pins the documented input contract: an +// io.ByteReader input is consumed exactly to the end of the stream, a plain +// io.Reader is buffered and may be read past it. Both decode identically. +func TestReaderInputConsumption(t *testing.T) { + comp, plain := lzmaEnwik(t, 200000) + trailer := []byte("TRAILER-AFTER-THE-STREAM") + withTrailer := append(append([]byte{}, comp...), trailer...) + + t.Run("byteReaderIsExact", func(t *testing.T) { + src := bytes.NewReader(withTrailer) // an io.ByteReader + r, err := NewReader(src) + if err != nil { + t.Fatal(err) + } + got, err := io.ReadAll(r) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(got, plain) { + t.Fatal("round-trip mismatch") + } + rest, _ := io.ReadAll(src) + if !bytes.Equal(rest, trailer) { + t.Fatalf("ByteReader input over-read: %d trailer bytes left, want %d", len(rest), len(trailer)) + } + }) + + t.Run("plainReaderIsBuffered", func(t *testing.T) { + src := &plainReader{r: bytes.NewReader(withTrailer)} + r, err := NewReader(src) + if err != nil { + t.Fatal(err) + } + got, err := io.ReadAll(r) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(got, plain) { + t.Fatal("round-trip mismatch") + } + // It must have pulled data in buffered runs, not one byte per Read: + // the whole 35 KB stream plus trailer in a handful of Reads. + if src.n < len(comp) { + t.Fatalf("consumed %d < stream %d", src.n, len(comp)) + } + }) + + t.Run("plainReaderReadCount", func(t *testing.T) { + // The point of buffering: the number of Read calls on the source + // must be far below one per compressed byte. + calls := 0 + r, err := NewReader(readCounter{bytes.NewReader(comp), &calls}) + if err != nil { + t.Fatal(err) + } + if _, err := io.Copy(io.Discard, r); err != nil { + t.Fatal(err) + } + if calls > len(comp)/512 { + t.Fatalf("%d Read calls for a %d byte stream; want buffered reads", calls, len(comp)) + } + }) +} + +// readCounter counts Read calls and is not an io.ByteReader. +type readCounter struct { + r io.Reader + calls *int +} + +func (c readCounter) Read(b []byte) (int, error) { + *c.calls++ + return c.r.Read(b) +} + +func BenchmarkReaderPlainFile(b *testing.B) { + comp, plain := lzmaEnwik(b, 4<<20) + f, err := os.CreateTemp(b.TempDir(), "x.lzma") + if err != nil { + b.Fatal(err) + } + if _, err := f.Write(comp); err != nil { + b.Fatal(err) + } + _ = f.Close() + b.SetBytes(int64(len(plain))) + b.ReportAllocs() + for i := 0; i < b.N; i++ { + fh, err := os.Open(f.Name()) + if err != nil { + b.Fatal(err) + } + r, err := NewReader(fh) // *os.File: not a ByteReader + if err != nil { + b.Fatal(err) + } + if _, err := io.Copy(io.Discard, r); err != nil { + b.Fatal(err) + } + _ = fh.Close() + } +} + +func BenchmarkReaderBufioFile(b *testing.B) { + comp, plain := lzmaEnwik(b, 4<<20) + f, err := os.CreateTemp(b.TempDir(), "x.lzma") + if err != nil { + b.Fatal(err) + } + if _, err := f.Write(comp); err != nil { + b.Fatal(err) + } + _ = f.Close() + b.SetBytes(int64(len(plain))) + b.ReportAllocs() + for i := 0; i < b.N; i++ { + fh, err := os.Open(f.Name()) + if err != nil { + b.Fatal(err) + } + r, err := NewReader(bufio.NewReader(fh)) + if err != nil { + b.Fatal(err) + } + if _, err := io.Copy(io.Discard, r); err != nil { + b.Fatal(err) + } + _ = fh.Close() + } +} diff --git a/malformed_test.go b/malformed_test.go index 3c099e94..c66d1216 100644 --- a/malformed_test.go +++ b/malformed_test.go @@ -401,3 +401,41 @@ func TestBlockHeaderPaddingIsCorrupt(t *testing.T) { err) } } + +// TestIndexIndicatorAtBlockIsCorrupt covers a block area that begins with the +// index indicator (0x00) at an offset the stream index says holds a block. +// The parallel reader positions itself by the index, so the two disagreeing +// is corruption; the bare errIndexIndicator sentinel used to escape from +// decodeBlock unwrapped, matching neither ErrCorrupt nor anything else, so a +// caller sorting "reject the input" from "retry the transport" could not +// tell what it was looking at. +func TestIndexIndicatorAtBlockIsCorrupt(t *testing.T) { + full := wellFormed(t) + + // The first block header starts right after the 12-byte stream header; + // its first byte is the header size, and 0x00 there is the index + // indicator. Nothing else is touched, so the index still points here. + const hdrOff = 12 + bad := append([]byte{}, full...) + bad[hdrOff] = 0x00 + + r, err := NewReader(bytes.NewReader(bad)) + if err == nil { + _, err = io.ReadAll(r) + } + if err == nil { + t.Fatal("sequential reader accepted an index indicator in place of the block") + } + + pr, err := NewParallelReader(bytes.NewReader(bad), int64(len(bad))) + if err == nil { + _, err = io.ReadAll(pr) + _ = pr.Close() + } + if err == nil { + t.Fatal("parallel reader accepted an index indicator in place of the block") + } + if !errors.Is(err, ErrCorrupt) { + t.Errorf("parallel reader returned %v; want a match for ErrCorrupt", err) + } +} diff --git a/parallelreader.go b/parallelreader.go index 7691cbfd..0b3b24e0 100644 --- a/parallelreader.go +++ b/parallelreader.go @@ -570,6 +570,16 @@ func (d *parallelDecoder) decodeBlock(bd *blockDesc, bufp *[]byte, s *workerScra h, hlen, err := readBlockHeader(s.xr) if err != nil { + // The sequential reader treats an index indicator as "the blocks + // are over" — there it is a legitimate outcome. Here the offset + // came from the index itself, which promised a block at this + // position; finding the index indicator instead means the index + // and the block area disagree, and that is corruption. Left as + // the bare sentinel it matched neither ErrCorrupt nor anything + // else a caller could classify. + if errors.Is(err, errIndexIndicator) { + return nil, corruptf("xz: index indicator where the index places block at offset %d", bd.offset) + } return nil, err } c := ReaderConfig{DictCap: d.dictCap} diff --git a/renovate.json5 b/renovate.json5 index 07f85cdd..873a9d98 100644 --- a/renovate.json5 +++ b/renovate.json5 @@ -13,6 +13,11 @@ commitBody: "Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>", // The update-aqua-checksum workflow pushes a fix-up commit onto Renovate's // branches; without this, Renovate treats the branch as human-modified and - // stops rebasing it. - gitIgnoredAuthors: ["41898282+github-actions[bot]@users.noreply.github.com"], + // stops rebasing it. Two identities: the org's limen-ci-forkcloser App (the + // credential the workflow prefers, so that CI re-runs on the fix-up), and + // the default GITHUB_TOKEN it falls back to. + gitIgnoredAuthors: [ + "317468017+limen-ci-forkcloser[bot]@users.noreply.github.com", + "41898282+github-actions[bot]@users.noreply.github.com", + ], }