From 845d252145403e5ece03d8477959e1a48ae98344 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 30 Aug 2026 17:34:09 +0200 Subject: [PATCH] FlateDecode: a stored first block begins with NUL, which is white-space flateDecode skipped leading white-space before inflating, because producers put the stream's EOL there. NUL is one of the six bytes the specification calls white-space, and a bare deflate stream whose first block is STORED begins with a NUL. Skipping it ate a real byte. The result was not an error. 00 03 00 fc ff "abc", once its NUL is gone, reads as a fixed-Huffman final block that ends at once: the stream inflated to NOTHING and reported no problem. A silently empty stream, not a diagnostic. Stored blocks are ordinary -- they are what a deflater emits for data that will not compress, which in a PDF is every image that arrived already compressed. Where the data starts is genuinely ambiguous, so it is now resolved by reading rather than guessing: every split point inside the leading white-space run is tried, and the read that yields bytes wins. "\r\n" before a stored block needs exactly two bytes skipped, not three, and no single rule gets both that and a bare stored block right. zlib is still tried only past the white-space, because a valid zlib stream cannot begin inside it: RFC 1950 fixes CM to 8, so the low nibble of the first byte is 8, and none of 00, 09, 0a, 0c, 0d, 20 has one. Also fixes TestDecodeRecoveringDamagedFlateKeepsPredictor, which asserted the recovered data was a prefix of the undamaged output. That only held because the halfway cut happened to fall on a row boundary. pngPredictor deliberately keeps a truncated final row, zero-filled -- TestPNGPredictorTruncatedRow pins that -- so the row the damage landed in cannot be a prefix of anything. The assertion now covers the whole rows and excludes the last. Found because Go 1.27's deflater stores this test's data where 1.26 compressed it. The library was wrong before that; nothing about it changed. 100% coverage under both 1.26.4 and 1.27.0. Co-Authored-By: Claude Opus 5 --- filter.go | 72 +++++++++++++++++++++++++++++++++++++++---------- filter_test.go | 42 +++++++++++++++++++++++++++++ recover_test.go | 21 +++++++++++++-- 3 files changed, 119 insertions(+), 16 deletions(-) diff --git a/filter.go b/filter.go index c26248b..4747617 100644 --- a/filter.go +++ b/filter.go @@ -315,25 +315,69 @@ func intParm(parm Dict, key Name, def int, r Resolver) int { // a prefix comes back with the error that ended it, never dressed up as a whole // stream, so the caller can tell the difference. func flateDecode(data []byte) ([]byte, error) { - i := 0 - for i < len(data) && isSpace(data[i]) { - i++ + // Where the stream really starts is ambiguous, and the ambiguity has to be + // resolved by reading rather than by guessing. + // + // Producers put the stream's EOL before the data, so leading white-space is + // skipped. But NUL is one of the six bytes the specification calls + // white-space, and a bare deflate stream whose first block is STORED begins + // with a NUL. Skip that and the stream does not merely fail: 00 03 00 fc ff + // "abc" reads, once its NUL is gone, as a fixed-Huffman final block that + // ends at once, so it inflates to NOTHING with no error. Silence, not a + // diagnostic. + // + // Stored blocks are ordinary -- they are what a deflater emits for data + // that will not compress, which in a PDF is every image that arrived + // already compressed -- and "\r\n" followed by such a block needs exactly + // two bytes skipped, not three. So every split point inside the leading + // white-space run is tried, and the read that actually yields bytes wins. + ws := 0 + for ws < len(data) && isSpace(data[ws]) { + ws++ } - data = data[i:] - if zr, err := zlib.NewReader(bytes.NewReader(data)); err == nil { - out, err := readAllCapped(zr) - if err == nil { - return out, nil + + var best []byte + var bestErr error + have := false + + try := func(b []byte, wrapped bool) bool { + var out []byte + var err error + if wrapped { + zr, zerr := zlib.NewReader(bytes.NewReader(b)) + if zerr != nil { + return false + } + out, err = readAllCapped(zr) + } else { + out, err = readAllCapped(flate.NewReader(bytes.NewReader(b))) } - if len(out) > 0 { - return out, fmt.Errorf("reader: FlateDecode: %w", err) + if err == nil && len(out) > 0 { + best, bestErr, have = out, nil, true + return true } + if !have || len(out) > len(best) { + best, bestErr, have = out, err, true + } + return false } - out, err := readAllCapped(flate.NewReader(bytes.NewReader(data))) - if err != nil { - return out, fmt.Errorf("reader: FlateDecode: %w", err) + + // zlib is tried only past the white-space, because a valid zlib stream + // cannot begin inside it: RFC 1950 fixes CM to 8, so the low nibble of the + // first byte is 8, and none of 00, 09, 0a, 0c, 0d, 20 has a low nibble of 8. + if try(data[ws:], true) { + return best, nil + } + // Bare deflate, from the far end of the white-space run back to the start. + for i := ws; i >= 0; i-- { + if try(data[i:], false) { + return best, nil + } } - return out, nil + if bestErr != nil { + return best, fmt.Errorf("reader: FlateDecode: %w", bestErr) + } + return best, nil } // readAllCapped reads r, refusing to grow past maxDecodedSize. diff --git a/filter_test.go b/filter_test.go index e966dd5..a75bccd 100644 --- a/filter_test.go +++ b/filter_test.go @@ -311,3 +311,45 @@ func TestDecodeFlateFailure(t *testing.T) { t.Error("want an error") } } + +// A bare deflate stream whose first block is STORED begins with a NUL, and NUL +// is one of the six bytes the specification calls white-space. Skipping it eats +// a real byte and the stream dies one byte in. +// +// The stream is built by hand rather than by a deflater, because which block +// type a deflater picks is its own business and changes between releases: Go +// 1.26 compressed this test's data, Go 1.27 stored it, and that is how this was +// found. +func TestFlateDecodeStoredBlockStartingWithNUL(t *testing.T) { + want := []byte("abc") + + var raw []byte + // Non-final stored block: BFINAL=0, BTYPE=00, then LEN and ^LEN, little-endian. + raw = append(raw, 0x00, byte(len(want)), 0x00, ^byte(len(want)), 0xff) + raw = append(raw, want...) + // Final empty stored block. + raw = append(raw, 0x01, 0x00, 0x00, 0xff, 0xff) + + if raw[0] != 0x00 { + t.Fatalf("test is not exercising what it claims: first byte %#x, want NUL", raw[0]) + } + if !isSpace(raw[0]) { + t.Fatal("test is not exercising what it claims: NUL is not treated as white-space") + } + + got, err := flateDecode(raw) + if err != nil || !bytes.Equal(got, want) { + t.Errorf("stored block: got %q, %v; want %q", got, err, want) + } +} + +// A bare deflate stream preceded by the stream's EOL: here the white-space skip +// is the thing that makes it readable, which is why it is there. +func TestFlateDecodeRawAfterLeadingEOL(t *testing.T) { + want := []byte("some data worth compressing, twice over") + padded := append([]byte("\r\n"), rawDeflateBytes(t, want)...) + got, err := flateDecode(padded) + if err != nil || !bytes.Equal(got, want) { + t.Errorf("raw deflate after EOL: got %q, %v", got, err) + } +} diff --git a/recover_test.go b/recover_test.go index a5c6b22..1018f25 100644 --- a/recover_test.go +++ b/recover_test.go @@ -105,8 +105,25 @@ func TestDecodeRecoveringDamagedFlateKeepsPredictor(t *testing.T) { if len(dec.Data) < 4*columns { t.Fatalf("recovered only %d bytes", len(dec.Data)) } - if !bytes.HasPrefix(want, dec.Data) { - t.Errorf("predictor not undone over the prefix: got %v, want a prefix of %v", dec.Data[:8], want[:8]) + // Every row but the last is true. The last one may not be: the stream can + // end mid-row, and pngPredictor deliberately pads the remainder with zeroes + // and emits the row rather than dropping it -- TestPNGPredictorTruncatedRow + // pins that. So the row the damage landed in is excluded here. + // + // Asserting over the whole of dec.Data only ever passed because the + // truncation happened to fall on a row boundary. Go 1.27's deflater picks + // different block types for this data than 1.26's, the halfway cut moved, + // and the assertion failed on a library that had not changed. + whole := len(dec.Data) - columns + if whole < 0 { + whole = 0 + } + if !bytes.HasPrefix(want, dec.Data[:whole]) { + n := whole + if n > 8 { + n = 8 + } + t.Errorf("predictor not undone over the whole rows: got %v, want a prefix of %v", dec.Data[:n], want[:n]) } }