From f56e42685f2335bde571480d3ff250b9724ccffd Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 27 Aug 2026 12:01:14 +0200 Subject: [PATCH 1/8] Keep what a broken filter chain did produce, and say that it is salvage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stream whose filter chain cannot be run to the end used to yield nothing at all: Decode returned an error and the caller lost the stream. qpdf's rule is the opposite, and pdf.js's too — a filter that cannot be applied ends the chain, and what the filters before it produced is still usually usable. The one thing neither of them does is pass the salvage off as a clean decode. This reader did exactly that in one place. flateDecode returned the prefix a damaged stream managed to inflate together with a nil error, so a truncated stream was indistinguishable from a whole one. Measured over the corpora, that is 33 streams in 18 of the 1633 real form documents under /Users/Shared/pdfforms and 4 streams in 2 of the 635 vendor fixtures: 37 streams the reader called clean while handing back a fragment. The other direction was measured too. Over the same 1633 real documents, 263 streams in 212 files (13.0%) fail to decode and yielded no bytes whatever; over 2015 arXiv PDFs from /Users/Shared/axc, 2 streams in 1 file. So: DecodeRecovering never fails and reports Recovered, Cause and Filter; Decode is the strict reading and refuses a chain it cannot finish, so a caller that must not act on damaged data says so by which function it calls. Every filter now returns the prefix it produced along with the reason it stopped, and a Flate or LZW prefix still goes through the predictor, which undoes one row at a time and so applies to a buffer that stops mid-row. The salvage is always as far down the chain as the filters got — a damaged Flate stream yields what it inflated, never the compressed bytes. Only a filter that produced nothing falls back to what went into it. Object streams and page content use the recovering reading, because an object stream that stops short still holds whole objects and a page whose content will not decode is still a page; PageContentDecoded reports whether any salvaging happened. Cross-reference streams stay strict: a table that has to be guessed at is worse than no table, and repair() is the answer to one. Measured, base vs this commit: 1633 real forms pages 9362 -> 9362, content bytes 224709710 -> 224709710, per-file content hashes identical, open failures 0 -> 0, streams the strict Decode refuses 263 -> 296 (+33, exactly the silently truncated ones) 635 vendor fx pages 1236 -> 1236, content bytes 11608189 -> 11608189, hashes identical, strict refusals 2 -> 6 (+4, likewise) 1184 damaged 1184 real files cut to 92% of their length: pages 3780 -> 3780, extracted content 126.5 MB -> 128.8 MB (+2 387 301 bytes, +1.89%), open failures 136 -> 136. No file lost content. Cost, 409 real forms read into memory first, three interleaved runs each: 1474/1153/848 ms before, 1335/1617/844 ms after — inside the noise of this machine; peak heap 335-343 MB before, 343-347 MB after; total allocation 563 MB in both. Fuzz targets for the two contracts the salvage rests on: that Decode and DecodeRecovering agree on every clean decode and disagree on nothing else, and that no byte string makes Open hand back a document it cannot walk. --- content.go | 61 +++++++----- content_test.go | 25 ++++- document.go | 16 ++- filter.go | 113 +++++++++++++++++---- filter_test.go | 7 +- lzw.go | 3 +- recover_test.go | 260 ++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 430 insertions(+), 55 deletions(-) create mode 100644 recover_test.go diff --git a/content.go b/content.go index d22b6c2..383f356 100644 --- a/content.go +++ b/content.go @@ -253,59 +253,74 @@ func Operations(data []byte) ([]Operation, error) { // PageContent returns the decoded content stream of the i'th page, counting // from one. A page whose /Contents is an array has its streams joined, as the // specification requires, with a newline between them. +// +// A content stream whose filters cannot be applied contributes what could be +// salvaged from it rather than failing the page, which is what a renderer +// needs; the error is for a page that is not there at all. +// [Document.PageContentDecoded] says whether any salvaging happened. func (d *Document) PageContent(i int) ([]byte, error) { + dec, err := d.PageContentDecoded(i) + return dec.Data, err +} + +// PageContentDecoded is [Document.PageContent] with the outcome of the decode +// attached: [Decoded.Recovered] reports that at least one of the page's content +// streams could not be decoded cleanly, and [Decoded.Cause] says why. +func (d *Document) PageContentDecoded(i int) (Decoded, error) { page, err := d.Page(i) if err != nil { - return nil, err + return Decoded{}, err } return d.contentOf(page) } // contentOf decodes a page's /Contents, whether one stream or several. -func (d *Document) contentOf(page Dict) ([]byte, error) { +func (d *Document) contentOf(page Dict) (Decoded, error) { o, err := d.Resolve(page.Get("Contents")) if err != nil { - return nil, err + return Decoded{}, err } switch v := o.(type) { case *Stream: - return d.decodedContent(v) + return d.decodedContent(v), nil case Array: - var out []byte + var out Decoded for _, e := range v { eo, err := d.Resolve(e) if err != nil { - return nil, err + return Decoded{}, err } s, ok := ToStream(eo) if !ok { continue } - part, err := d.decodedContent(s) - if err != nil { - return nil, err + part := d.decodedContent(s) + if part.Recovered && !out.Recovered { + out.Recovered, out.Cause, out.Filter = true, part.Cause, part.Filter } - if len(out) > 0 { - out = append(out, '\n') + if len(out.Data) > 0 { + out.Data = append(out.Data, '\n') } - out = append(out, part...) + out.Data = append(out.Data, part.Data...) } return out, nil } - return nil, nil + return Decoded{}, nil } -// decodedContent decodes one content stream, refusing an image filter, which -// has no business being there. -func (d *Document) decodedContent(s *Stream) ([]byte, error) { - data, img, err := d.DecodeStream(s) - if err != nil { - return nil, err - } - if img != "" { - return nil, fmt.Errorf("reader: a content stream is filtered as an image (/%s)", img) +// decodedContent decodes one content stream. An image filter has no business +// being there, so the bytes it holds are not content: they are reported as +// salvage, not handed over as if they could be tokenised. +func (d *Document) decodedContent(s *Stream) Decoded { + dec := d.DecodeStreamRecovering(s) + if dec.Image != "" { + return Decoded{ + Recovered: true, + Filter: dec.Image, + Cause: fmt.Errorf("reader: a content stream is filtered as an image (/%s)", dec.Image), + } } - return data, nil + return dec } // PageOperations tokenises the i'th page's content stream, counting from one. diff --git a/content_test.go b/content_test.go index a0964c5..f9b6e65 100644 --- a/content_test.go +++ b/content_test.go @@ -242,8 +242,16 @@ func TestPageContentImageFilterIsRefused(t *testing.T) { if err != nil { t.Fatal(err) } - if _, err := d.PageContent(1); err == nil { - t.Error("want an error") + dec, err := d.PageContentDecoded(1) + if err != nil { + t.Fatal(err) + } + if !dec.Recovered || dec.Cause == nil || dec.Filter != "DCTDecode" { + t.Errorf("got %+v", dec) + } + // Bytes an image filter holds are not content, so none are handed over. + if len(dec.Data) != 0 { + t.Errorf("got %q, want no content", dec.Data) } } @@ -258,8 +266,17 @@ func TestPageContentUndecodable(t *testing.T) { if err != nil { t.Fatal(err) } - if _, err := d.PageContent(1); err == nil { - t.Errorf("%s: want an error", contents) + // A page whose only content stream will not decode is still a page: + // the raw bytes come back flagged, not as a failure. + dec, err := d.PageContentDecoded(1) + if err != nil { + t.Fatalf("%s: %v", contents, err) + } + if !dec.Recovered || dec.Cause == nil || dec.Filter != "FlateDecode" { + t.Errorf("%s: got %+v", contents, dec) + } + if string(dec.Data) != "not deflate data" { + t.Errorf("%s: got %q, want the raw bytes", contents, dec.Data) } } } diff --git a/document.go b/document.go index 7283726..5b263dc 100644 --- a/document.go +++ b/document.go @@ -200,10 +200,13 @@ func (d *Document) objectStream(num int) (map[int]Object, error) { if !ok { return objs, nil } - data, img, err := d.DecodeStream(s) - if err != nil || img != "" { + // An object stream that stops in the middle still holds whole objects in + // the part that did decode, and losing them loses pages. + dec := d.DecodeStreamRecovering(s) + if dec.Image != "" { return objs, nil } + data := dec.Data n := int(intOr(s.Dict.Get("N"), 0)) first := int(intOr(s.Dict.Get("First"), 0)) if n <= 0 || first <= 0 || first > len(data) { @@ -242,11 +245,18 @@ func intOr(o Object, def int64) int64 { } // DecodeStream applies a stream's filter chain, resolving any indirect decode -// parameters against this document. +// parameters against this document. A chain that cannot be run to the end is +// an error; [Document.DecodeStreamRecovering] salvages instead. func (d *Document) DecodeStream(s *Stream) ([]byte, Name, error) { return Decode(s.Dict, s.Raw, d.Get) } +// DecodeStreamRecovering applies a stream's filter chain, salvaging what it can +// from a chain that cannot be run to the end and saying that it did so. +func (d *Document) DecodeStreamRecovering(s *Stream) Decoded { + return DecodeRecovering(s.Dict, s.Raw, d.Get) +} + // Catalog returns the document catalogue the trailer's /Root names. func (d *Document) Catalog() (Dict, error) { if d.trailer == nil { diff --git a/filter.go b/filter.go index 25e0bad..9616eee 100644 --- a/filter.go +++ b/filter.go @@ -31,24 +31,71 @@ func ImageFilter(n Name) bool { return false } -// Decode applies a stream dictionary's filter chain to raw. It returns the -// decoded bytes and, when the chain ends in an image filter, that filter's -// name together with the bytes still encoded in it. -func Decode(d Dict, raw []byte, resolve Resolver) ([]byte, Name, error) { +// A Decoded is the outcome of applying a stream's filter chain, including the +// outcome of a chain that could not be finished. +type Decoded struct { + // Data is what the chain produced. + Data []byte + // Image names the image filter the chain stopped at, when it stopped at + // one; Data is then still encoded in that filter. + Image Name + // Recovered says Data is what could be salvaged from a chain that could + // not be run to the end, not a clean decode. A caller that must not act on + // damaged data stops here — or calls [Decode], which refuses outright. + Recovered bool + // Cause says why the chain stopped, and is set only when Recovered is. + Cause error + // Filter names the filter that could not be applied, when one is to blame: + // a chain whose /Filter entry itself is unreadable blames nothing. + Filter Name +} + +// DecodeRecovering applies a stream dictionary's filter chain to raw and never +// fails. A filter that cannot be applied — corrupt Flate data, a stream that +// stops in the middle, a filter name nobody implements — ends the chain, and +// what the filters before it produced is returned with [Decoded.Recovered] +// set. That is deliberately what every other reader does: refusing the stream +// loses a page the file can still show. +// +// The salvage is always as far down the chain as the filters got, never the +// bytes as they arrived: a damaged Flate stream yields the prefix it did +// inflate, not the compressed bytes. Only a filter that produced nothing at +// all falls back to what went into it. +func DecodeRecovering(d Dict, raw []byte, resolve Resolver) Decoded { filters, parms, err := filterChain(d, resolve) if err != nil { - return nil, "", err + return Decoded{Data: raw, Recovered: true, Cause: err} } data := raw for i, f := range filters { if ImageFilter(f) { - return data, f, nil + return Decoded{Data: data, Image: f} } - if data, err = applyFilter(f, data, parms[i], resolve); err != nil { - return nil, "", err + out, err := applyFilter(f, data, parms[i], resolve) + if err != nil { + if len(out) == 0 { + out = data + } + return Decoded{Data: out, Recovered: true, Cause: err, Filter: f} } + data = out } - return data, "", nil + return Decoded{Data: data} +} + +// Decode applies a stream dictionary's filter chain to raw. It returns the +// decoded bytes and, when the chain ends in an image filter, that filter's +// name together with the bytes still encoded in it. +// +// Decode is the strict reading: a chain that cannot be run to the end is an +// error and yields no bytes. [DecodeRecovering] is the lenient one, and says +// which of the two it gave you. +func Decode(d Dict, raw []byte, resolve Resolver) ([]byte, Name, error) { + r := DecodeRecovering(d, raw, resolve) + if r.Recovered { + return nil, "", r.Cause + } + return r.Data, r.Image, nil } // DecodeStream is Decode for a parsed stream. @@ -56,6 +103,11 @@ func DecodeStream(s *Stream, resolve Resolver) ([]byte, Name, error) { return Decode(s.Dict, s.Raw, resolve) } +// DecodeStreamRecovering is DecodeRecovering for a parsed stream. +func DecodeStreamRecovering(s *Stream, resolve Resolver) Decoded { + return DecodeRecovering(s.Dict, s.Raw, resolve) +} + // filterChain reads /Filter and /DecodeParms, each of which may be a single // value or an array, and returns them aligned. func filterChain(d Dict, r Resolver) ([]Name, []Dict, error) { @@ -121,14 +173,14 @@ func applyFilter(f Name, data []byte, parm Dict, r Resolver) ([]byte, error) { case "FlateDecode", "Fl": out, err := flateDecode(data) if err != nil { - return nil, err + return salvage(out, err, parm, r) } return applyPredictor(out, parm, r) case "LZWDecode", "LZW": early := intParm(parm, "EarlyChange", 1, r) out, err := lzwDecode(data, early != 0) if err != nil { - return nil, err + return salvage(out, err, parm, r) } return applyPredictor(out, parm, r) case "ASCIIHexDecode", "AHx": @@ -143,6 +195,20 @@ func applyFilter(f Name, data []byte, parm Dict, r Resolver) ([]byte, error) { return nil, fmt.Errorf("reader: unsupported filter /%s", f) } +// salvage finishes a filter that stopped part-way. The prefix it did produce is +// still worth having, and the predictor still applies to it: the predictors a +// PDF may name all undo one row at a time, so a buffer that stops in the middle +// undoes up to where it stops. +func salvage(out []byte, err error, parm Dict, r Resolver) ([]byte, error) { + if len(out) == 0 { + return nil, err + } + if p, perr := applyPredictor(out, parm, r); perr == nil { + return p, err + } + return out, err +} + // intParm reads an integer decode parameter, falling back to its default. func intParm(parm Dict, key Name, def int, r Resolver) int { if parm == nil { @@ -161,7 +227,9 @@ func intParm(parm Dict, key Name, def int, r Resolver) int { // flateDecode inflates a stream. Producers emit both zlib-wrapped and bare // deflate data, sometimes with leading white-space, and truncate the last -// stream in a damaged file; all four cases yield whatever bytes are there. +// stream in a damaged file. All four cases yield whatever bytes are there — but +// 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]) { @@ -170,13 +238,16 @@ func flateDecode(data []byte) ([]byte, error) { data = data[i:] if zr, err := zlib.NewReader(bytes.NewReader(data)); err == nil { out, err := readAllCapped(zr) - if err == nil || len(out) > 0 { + if err == nil { return out, nil } + if len(out) > 0 { + return out, fmt.Errorf("reader: FlateDecode: %w", err) + } } out, err := readAllCapped(flate.NewReader(bytes.NewReader(data))) - if err != nil && len(out) == 0 { - return nil, fmt.Errorf("reader: FlateDecode: %w", err) + if err != nil { + return out, fmt.Errorf("reader: FlateDecode: %w", err) } return out, nil } @@ -209,7 +280,7 @@ func asciiHexDecode(data []byte) ([]byte, error) { } v := hexVal(c) if v < 0 { - return nil, fmt.Errorf("reader: ASCIIHexDecode: invalid digit %q", rune(c)) + return out, fmt.Errorf("reader: ASCIIHexDecode: invalid digit %q", rune(c)) } if hi < 0 { hi = v @@ -257,20 +328,20 @@ func ascii85Decode(data []byte) ([]byte, error) { continue } if c < '!' || c > 'u' { - return nil, fmt.Errorf("reader: ASCII85Decode: invalid character %q", rune(c)) + return out, fmt.Errorf("reader: ASCII85Decode: invalid character %q", rune(c)) } group[n] = c n++ if n == 5 { if err := checkA85(group); err != nil { - return nil, err + return out, err } flush(5) n = 0 } } if n == 1 { - return nil, fmt.Errorf("reader: ASCII85Decode: truncated final group") + return out, fmt.Errorf("reader: ASCII85Decode: truncated final group") } if n > 1 { flush(n) @@ -304,13 +375,13 @@ func runLengthDecode(data []byte) ([]byte, error) { case n < 128: end := i + n + 1 if end > len(data) { - return nil, fmt.Errorf("reader: RunLengthDecode: truncated literal run") + return out, fmt.Errorf("reader: RunLengthDecode: truncated literal run") } out = append(out, data[i:end]...) i = end default: if i >= len(data) { - return nil, fmt.Errorf("reader: RunLengthDecode: truncated repeat run") + return out, fmt.Errorf("reader: RunLengthDecode: truncated repeat run") } out = append(out, bytes.Repeat(data[i:i+1], 257-n)...) i++ diff --git a/filter_test.go b/filter_test.go index 7b9f698..e966dd5 100644 --- a/filter_test.go +++ b/filter_test.go @@ -164,11 +164,12 @@ func TestFlateDecode(t *testing.T) { if got, err := flateDecode(padded); err != nil || !bytes.Equal(got, want) { t.Errorf("padded: %q, %v", got, err) } - // A stream cut short yields the bytes it did carry. + // A stream cut short yields the bytes it did carry, together with the + // error that ended it: a prefix is never passed off as a whole stream. full := deflateBytes(t, bytes.Repeat(want, 50)) short, err := flateDecode(full[:len(full)/2]) - if err != nil { - t.Errorf("truncated: %v", err) + if err == nil { + t.Error("truncated: want the error that ended it") } if len(short) == 0 { t.Error("truncated: no data recovered") diff --git a/lzw.go b/lzw.go index 1fac061..9eb58c9 100644 --- a/lzw.go +++ b/lzw.go @@ -55,7 +55,8 @@ func lzwDecode(data []byte, early bool) ([]byte, error) { // The encoder may name the entry it is about to define. entry = append(append([]byte{}, prev...), prev[0]) default: - return nil, fmt.Errorf("reader: LZWDecode: code %d is not in the table", code) + // What the stream did say is kept, with the reason it stopped. + return out, fmt.Errorf("reader: LZWDecode: code %d is not in the table", code) } out = append(out, entry...) diff --git a/recover_test.go b/recover_test.go new file mode 100644 index 0000000..b10b94f --- /dev/null +++ b/recover_test.go @@ -0,0 +1,260 @@ +package reader + +import ( + "bytes" + "fmt" + "strings" + "testing" +) + +// A filter chain that runs to the end is a clean decode, and says so. +func TestDecodeRecoveringClean(t *testing.T) { + raw := deflateBytes(t, []byte("plain content")) + dec := DecodeRecovering(Dict{"Filter": Name("FlateDecode")}, raw, nil) + if dec.Recovered || dec.Cause != nil || dec.Image != "" { + t.Fatalf("got %+v", dec) + } + if string(dec.Data) != "plain content" { + t.Errorf("got %q", dec.Data) + } +} + +// A chain that ends in an image filter is not a failure: the caller is handed +// the still-encoded bytes and the filter's name. +func TestDecodeRecoveringImageFilter(t *testing.T) { + dec := DecodeRecovering(Dict{"Filter": Name("DCTDecode")}, []byte("jpeg-ish"), nil) + if dec.Recovered || dec.Image != "DCTDecode" || string(dec.Data) != "jpeg-ish" { + t.Fatalf("got %+v", dec) + } +} + +// A filter nobody implements ends the chain, and the bytes as they stand come +// back flagged rather than not at all. +func TestDecodeRecoveringUnknownFilter(t *testing.T) { + dec := DecodeRecovering(Dict{"Filter": Name("BrotliDecode")}, []byte("brotli bytes"), nil) + if !dec.Recovered || dec.Cause == nil { + t.Fatalf("got %+v", dec) + } + if dec.Filter != "BrotliDecode" || string(dec.Data) != "brotli bytes" { + t.Errorf("got %+v", dec) + } + // The strict reading refuses the same stream outright. + if _, _, err := Decode(Dict{"Filter": Name("BrotliDecode")}, []byte("brotli bytes"), nil); err == nil { + t.Error("Decode: want an error") + } +} + +// The salvage is as far down the chain as the filters got: an unknown filter +// after a Flate one yields the inflated bytes, never the compressed ones. +func TestDecodeRecoveringSalvagesDownTheChain(t *testing.T) { + raw := deflateBytes(t, []byte("inflated already")) + d := Dict{"Filter": Array{Name("FlateDecode"), Name("Nope")}} + dec := DecodeRecovering(d, raw, nil) + if !dec.Recovered || dec.Filter != "Nope" { + t.Fatalf("got %+v", dec) + } + if string(dec.Data) != "inflated already" { + t.Errorf("got %q, want the inflated prefix", dec.Data) + } +} + +// A /Filter entry that cannot be read at all blames no filter in particular. +func TestDecodeRecoveringUnreadableFilterEntry(t *testing.T) { + dec := DecodeRecovering(Dict{"Filter": Integer(7)}, []byte("as it lies"), nil) + if !dec.Recovered || dec.Cause == nil || dec.Filter != "" { + t.Fatalf("got %+v", dec) + } + if string(dec.Data) != "as it lies" { + t.Errorf("got %q", dec.Data) + } +} + +// A damaged Flate stream yields the prefix it did inflate, with the predictor +// still undone over it, because the predictors undo one row at a time. +func TestDecodeRecoveringDamagedFlateKeepsPredictor(t *testing.T) { + const columns = 4 + var rows, want []byte + acc := make([]byte, columns) + for i := 0; i < 300; i++ { + v := byte(i*37 + 11) + rows = append(rows, 2) // the PNG "up" filter + for c := 0; c < columns; c++ { + rows = append(rows, v+byte(c)) + acc[c] += v + byte(c) + } + want = append(want, acc...) + } + full := deflateBytes(t, rows) + d := Dict{ + "Filter": Name("FlateDecode"), + "DecodeParms": Dict{"Predictor": Integer(12), "Columns": Integer(columns)}, + } + dec := DecodeRecovering(d, full[:len(full)/2], nil) + if !dec.Recovered || dec.Cause == nil { + t.Fatalf("got %+v", dec) + } + 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]) + } +} + +// A predictor that cannot be applied to the salvaged prefix leaves it as it is +// rather than throwing it away. +func TestDecodeRecoveringDamagedFlateUnusablePredictor(t *testing.T) { + full := deflateBytes(t, bytes.Repeat([]byte("row"), 200)) + d := Dict{ + "Filter": Name("FlateDecode"), + "DecodeParms": Dict{"Predictor": Integer(5)}, // no such predictor + } + dec := DecodeRecovering(d, full[:len(full)/2], nil) + if !dec.Recovered || len(dec.Data) == 0 { + t.Fatalf("got %+v", dec) + } + if !bytes.HasPrefix(dec.Data, []byte("rowrow")) { + t.Errorf("got %q, want the un-predicted prefix", dec.Data[:min(12, len(dec.Data))]) + } +} + +// Every filter keeps what it managed to produce, with the reason it stopped. +func TestFiltersKeepTheirPrefix(t *testing.T) { + for _, c := range []struct { + filter Name + raw string + want string + }{ + {"ASCIIHexDecode", "4142zz", "AB"}, + {"ASCII85Decode", "87cURDZ\x01", "Hell"}, + {"RunLengthDecode", "\x02abc\x7f", "abc"}, + } { + dec := DecodeRecovering(Dict{"Filter": c.filter}, []byte(c.raw), nil) + if !dec.Recovered || dec.Cause == nil { + t.Errorf("/%s: got %+v", c.filter, dec) + continue + } + if string(dec.Data) != c.want { + t.Errorf("/%s: got %q, want %q", c.filter, dec.Data, c.want) + } + } +} + +// LZW keeps its prefix too, where the old reading returned nothing at all. +func TestLZWKeepsItsPrefix(t *testing.T) { + // A clear code, then "A", then a code that names no table entry. + raw := []byte{0x80, 0x20, 0x50, 0x1f, 0xf0} + out, err := lzwDecode(raw, true) + if err == nil { + t.Fatal("want the error that ended it") + } + if len(out) == 0 { + t.Error("no data recovered") + } +} + +// The stream helpers agree with the dictionary ones. +func TestDecodeStreamRecovering(t *testing.T) { + s := &Stream{Dict: Dict{"Filter": Name("Nope")}, Raw: []byte("raw")} + if dec := DecodeStreamRecovering(s, nil); !dec.Recovered || string(dec.Data) != "raw" { + t.Errorf("package: got %+v", dec) + } + d, err := Open(onePage()) + if err != nil { + t.Fatal(err) + } + if dec := d.DecodeStreamRecovering(s); !dec.Recovered || string(dec.Data) != "raw" { + t.Errorf("document: got %+v", dec) + } +} + +// An object stream whose data stops short still defines the objects its prefix +// holds, which is the difference between some pages and none. +func TestObjectStreamSalvagesItsPrefix(t *testing.T) { + bodies := []string{ + "<< /Type /Catalog /Pages 2 0 R >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 1 1] >>", + "<< /Type /Page /Parent 2 0 R >>", + } + var index, payload strings.Builder + for i, body := range bodies { + fmt.Fprintf(&index, "%d %d ", i+1, payload.Len()) + payload.WriteString(body) + payload.WriteString(" ") + } + first := index.Len() + packed := deflateBytes(t, []byte(index.String()+payload.String())) + + b := newBuilder() + // The last four bytes of a zlib stream are its checksum: a file cut there + // carries every byte of the data and still refuses to inflate cleanly. + b.streamObj(4, fmt.Sprintf("/Type /ObjStm /N %d /First %d /Filter /FlateDecode", len(bodies), first), packed[:len(packed)-4]) + raw := b.table("/Root 1 0 R") + d := &Document{ + buf: raw, + xref: map[int]xrefEntry{4: {kind: 'n', offset: int64(bytes.Index(raw, []byte("4 0 obj")))}}, + cache: map[int]Object{}, + loading: map[int]bool{}, + objStms: map[int]map[int]Object{}, + trailer: Dict{"Root": Ref{Num: 1}}, + } + for i := range bodies { + d.xref[i+1] = xrefEntry{kind: 'o', strmNum: 4} + } + if got := d.PageCount(); got != 1 { + t.Fatalf("page count %d, want 1", got) + } + if _, err := d.Catalog(); err != nil { + t.Errorf("catalogue lost: %v", err) + } +} + +// FuzzDecodeRecovering asserts the contract the salvage rests on: it never +// panics and never reports a clean decode it did not make. +func FuzzDecodeRecovering(f *testing.F) { + f.Add([]byte("FlateDecode"), []byte("not deflate")) + f.Add([]byte("ASCIIHexDecode"), []byte("41 42 4")) + f.Add([]byte("ASCII85Decode"), []byte("<~87cURD]~>")) + f.Add([]byte("RunLengthDecode"), []byte("\x02abc")) + f.Add([]byte("LZWDecode"), []byte("\x80\x20\x50\x1f\xf0")) + f.Add([]byte("Nope"), []byte("whatever")) + f.Fuzz(func(t *testing.T, filter, raw []byte) { + d := Dict{ + "Filter": Name(filter), + "DecodeParms": Dict{"Predictor": Integer(12), "Columns": Integer(4)}, + } + dec := DecodeRecovering(d, raw, nil) + if dec.Recovered != (dec.Cause != nil) { + t.Fatalf("Recovered and Cause disagree: %+v", dec) + } + data, img, err := Decode(d, raw, nil) + if (err != nil) != dec.Recovered { + t.Fatalf("Decode and DecodeRecovering disagree: %v vs %+v", err, dec) + } + if err == nil && (img != dec.Image || !bytes.Equal(data, dec.Data)) { + t.Fatalf("clean decodes differ: %q/%s vs %+v", data, img, dec) + } + }) +} + +// FuzzOpen asserts that no byte string makes the reader panic or hand back a +// document it cannot walk. +func FuzzOpen(f *testing.F) { + f.Add(onePage()) + f.Add([]byte("%PDF-1.7\n1 0 obj\n<< /Type /Page >>\nendobj\ntrailer\n<< >>\n")) + f.Add([]byte("%PDF-1.4\nstartxref\n9\n%%EOF\n")) + f.Fuzz(func(t *testing.T, b []byte) { + d, err := Open(b) + if err != nil { + return + } + for i := 1; i <= d.PageCount() && i <= 8; i++ { + if _, err := d.PageContentDecoded(i); err != nil { + continue + } + if _, err := d.PageOperations(i); err != nil { + continue + } + } + }) +} From af8243779b4fac5d83bad36d534845ec7bffbc3c Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 27 Aug 2026 14:56:36 +0200 Subject: [PATCH 2/8] Rebuild a damaged file the same way twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a second map-order non-determinism, distinct from the inline-image one fixed in 62002d5: that one was in InlineImage.Expanded, this one is in indexObjectStreams, and PageContent never calls Expanded, so the fix for the first does not touch the second. Measured on 1184 real files cut to 92% of their length, extracting the content of every page and hashing it: v0.4.2, with 62002d5 in 5 runs, 5 different answers 126460833 / 126474891 / 126466402 / 126453047 / 126469752 bytes v0.4.2 + this commit only 5 runs, 1 answer: 126464943 bytes, hash bef99b751ed33040 indexObjectStreams walked d.xref in map order looking for object streams. Two object streams in a damaged file may both claim the same object number — a partial rewrite leaves the old stream in place beside the new one — and the first stream walked wins, because the loop below it declines to overwrite an entry that is already there. So which definition of a shared object the reader took was whichever order the map handed out that run, and the same file gave different pages on different runs. The numbers are now sorted before they are walked, so the lowest-numbered object stream defines a contested object. That is arbitrary but it is a property of the file, which is the point. The test writes the higher-numbered stream first, so agreeing with the file's own layout would not be enough to pass it; it fails on the unsorted walk at run 0. --- repair.go | 11 ++++++++++- repair_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/repair.go b/repair.go index be8cda0..5ee32f4 100644 --- a/repair.go +++ b/repair.go @@ -120,8 +120,17 @@ func (d *Document) synthesiseCatalogue() { // indexObjectStreams adds the objects held inside every object stream the scan // found, without overwriting an object written directly in the file. func (d *Document) indexObjectStreams() { - var streams []int + // The object numbers are sorted before they are walked. Two object streams + // in a damaged file may both claim the same object number, and the first + // one walked wins; taking them in map order makes the same file read + // differently from one run to the next. + nums := make([]int, 0, len(d.xref)) for num := range d.xref { + nums = append(nums, num) + } + slices.Sort(nums) + var streams []int + for _, num := range nums { o, err := d.Get(Ref{Num: num}) if err != nil { continue diff --git a/repair_test.go b/repair_test.go index 1da85e9..e94bd45 100644 --- a/repair_test.go +++ b/repair_test.go @@ -546,3 +546,45 @@ func TestObjectsAreListedInOrderHoweverSparseTheNumbersAre(t *testing.T) { } } } + +// Two object streams in a damaged file may both claim the same object number. +// Which one wins has to be a property of the file, not of the map iteration +// order the run happened to get. +func TestIndexObjectStreamsIsDeterministic(t *testing.T) { + objStm := func(num int, marker string) (int, string, []byte) { + index := "5 0 " + return num, fmt.Sprintf("/Type /ObjStm /N 1 /First %d", len(index)), + []byte(index + fmt.Sprintf("<< /Marker /%s >>", marker)) + } + b := newBuilder() + b.obj(1, "<< /Type /Catalog /Pages 2 0 R >>") + b.obj(2, "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 1 1] >>") + b.obj(3, "<< /Type /Page /Parent 2 0 R >>") + // The higher-numbered stream is written first, so agreeing with the file's + // own order would not be enough to pass. + b.streamObj(objStm(20, "fromTwenty")) + b.streamObj(objStm(10, "fromTen")) + // No startxref, so the tables cannot be read and the rebuild runs. + raw := append(b.bytesOf(), []byte("trailer\n<< /Root 1 0 R >>\n%%EOF\n")...) + + for i := 0; i < 200; i++ { + d, err := Open(raw) + if err != nil { + t.Fatal(err) + } + if !d.Repaired() { + t.Fatal("the file was not rebuilt") + } + o, err := d.Get(Ref{Num: 5}) + if err != nil { + t.Fatal(err) + } + dict, ok := ToDict(o) + if !ok { + t.Fatalf("run %d: object 5 is a %s", i, o.Kind()) + } + if got, _ := ToName(dict.Get("Marker")); got != "fromTen" { + t.Fatalf("run %d: object 5 came from the wrong stream: /%s", i, got) + } + } +} From 11f13ebc0286673a0ab3063157a71232ed487b87 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 27 Aug 2026 14:59:52 +0200 Subject: [PATCH 3/8] Read the /Crypt filter, and stop decrypting the streams that carry it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not one of the three defects this branch set out to fix, but the biggest real-world decode gap the measurement that looked for them turned up, and the one that decides whether the salvage in dbb2558 tells the truth. Measured over the 1633 real form documents in /Users/Shared/pdfforms — the real ones, not the 635 vendor fixtures — 263 streams in 212 files fail to decode. 209 of those failures, in 209 distinct files (12.8% of the corpus), are the one message "reader: unsupported filter /Crypt". Every one of the 209 files is encrypted, carries /EncryptMetadata false, and puts /Filter[/Crypt] on its /Type/Metadata stream. /Crypt is not a transformation. It names the crypt filter a stream's bytes were encrypted with, and /Identity — which is also what /Crypt with no /Name means — says they were not encrypted at all. So two things were wrong, and either alone leaves the reader worse off than fixing both: - the filter was unimplemented, so the chain failed; - the decryptor decrypted the stream anyway, because it decrypts every stream that is not an /XRef one, so the bytes were noise before the filter ever saw them. Implementing only the filter is measurably the worst of the three states. With the filter in but the stream still decrypted, all 209 streams decode "cleanly" and 0 of 209 are readable XMP: the reader hands back noise and calls it clean. With both halves, 209 of 209 decode cleanly and 209 of 209 are readable XMP, beginning ``. A /Crypt filter that names something other than /Identity is reported rather than waved through: this reader cannot re-apply a named crypt filter at the filter layer, and pretending otherwise would hand back ciphertext that looks like data. There are no such streams in either corpus — 0 of 240 292 streams — so the branch is a guard, not a feature. Measured, base vs this commit: 1633 real forms streams the strict Decode refuses 263 -> 87. That is -209 /Crypt and +33 damaged Flate prefixes that dbb2558 stopped calling clean. Pages 9362 -> 9362, page content bytes 224709710 -> 224709710, per-file content hashes identical, open failures 0 -> 0. 635 vendor fx strict refusals 2 -> 4 (-2 /Crypt, +4 prefixes). Pages, bytes and hashes identical. Page content does not move, and should not: the streams this recovers are metadata, not content. What moves is that a caller asking a form for its XMP now gets its XMP. --- crypt.go | 16 ++++++ crypt_filter_test.go | 121 +++++++++++++++++++++++++++++++++++++++++++ filter.go | 54 +++++++++++++++++++ 3 files changed, 191 insertions(+) create mode 100644 crypt_filter_test.go diff --git a/crypt.go b/crypt.go index 993cfe4..9b2ac46 100644 --- a/crypt.go +++ b/crypt.go @@ -520,12 +520,28 @@ func (dec *decryptor) walk(num, gen int, o Object) Object { return v } v.Dict, _ = ToDict(dec.walk(num, gen, v.Dict)) + if streamIsPlain(v.Dict) { + return v + } v.Raw = dec.decryptBytes(num, gen, dec.streams, v.Raw) return v } return o } +// streamIsPlain reports whether a stream's own filter chain says its bytes were +// left unencrypted: a leading /Crypt filter naming /Identity, which is what +// /Crypt with no /Name means too. Producers use it for the metadata stream of a +// file whose /EncryptMetadata is false, and decrypting such a stream turns +// readable XML into noise. +func streamIsPlain(d Dict) bool { + if first, ok := firstFilter(d); !ok || first != "Crypt" { + return false + } + name, ok := ToName(firstDecodeParms(d).Get("Name")) + return !ok || name == "Identity" +} + // resolved is a helper for reading an /Encrypt entry that may be indirect. func resolved(d Dict, key Name, r Resolver) Object { o, err := Resolve(d.Get(key), r) diff --git a/crypt_filter_test.go b/crypt_filter_test.go new file mode 100644 index 0000000..c06bd64 --- /dev/null +++ b/crypt_filter_test.go @@ -0,0 +1,121 @@ +package reader + +import ( + "fmt" + "testing" +) + +// /Crypt is not a transformation: /Identity, and /Crypt with no /Name at all, +// leave the bytes exactly as they are. +func TestCryptFilterIsIdentity(t *testing.T) { + for _, d := range []Dict{ + {"Filter": Name("Crypt")}, + {"Filter": Name("Crypt"), "DecodeParms": Dict{"Name": Name("Identity")}}, + {"Filter": Array{Name("Crypt")}, "DecodeParms": Array{Dict{"Name": Name("Identity")}}}, + } { + dec := DecodeRecovering(d, []byte(""), nil) + if dec.Recovered || string(dec.Data) != "" { + t.Errorf("%v: got %+v", d, dec) + } + } +} + +// A /Crypt filter chained ahead of a real one is stepped over. +func TestCryptFilterAheadOfFlate(t *testing.T) { + raw := deflateBytes(t, []byte("metadata")) + d := Dict{ + "Filter": Array{Name("Crypt"), Name("FlateDecode")}, + "DecodeParms": Array{Dict{"Name": Name("Identity")}, Null{}}, + } + dec := DecodeRecovering(d, raw, nil) + if dec.Recovered || string(dec.Data) != "metadata" { + t.Fatalf("got %+v", dec) + } +} + +// A crypt filter this reader cannot account for is reported, because the bytes +// would otherwise be ciphertext passed off as data. +func TestCryptFilterRefusesANamedFilter(t *testing.T) { + d := Dict{"Filter": Name("Crypt"), "DecodeParms": Dict{"Name": Name("StdCF")}} + dec := DecodeRecovering(d, []byte("cipher"), nil) + if !dec.Recovered || dec.Cause == nil || dec.Filter != "Crypt" { + t.Fatalf("got %+v", dec) + } +} + +// A /Name that cannot be resolved is reported too. +func TestCryptFilterUnresolvableName(t *testing.T) { + fail := func(Ref) (Object, error) { return nil, fmt.Errorf("no") } + d := Dict{"Filter": Name("Crypt"), "DecodeParms": Dict{"Name": Ref{Num: 9}}} + dec := DecodeRecovering(d, []byte("cipher"), fail) + if !dec.Recovered || dec.Cause == nil { + t.Fatalf("got %+v", dec) + } +} + +func TestFirstFilterAndDecodeParms(t *testing.T) { + for _, c := range []struct { + d Dict + name Name + ok bool + }{ + {Dict{"Filter": Name("Fl")}, "Fl", true}, + {Dict{"Filter": Array{Name("Crypt"), Name("Fl")}}, "Crypt", true}, + {Dict{"Filter": Array{Integer(1)}}, "", false}, + {Dict{"Filter": Array{}}, "", false}, + {Dict{"Filter": Integer(3)}, "", false}, + {Dict{}, "", false}, + } { + if got, ok := firstFilter(c.d); got != c.name || ok != c.ok { + t.Errorf("firstFilter(%v) = %q, %v", c.d, got, ok) + } + } + for _, c := range []struct { + d Dict + want Name + }{ + {Dict{"DecodeParms": Dict{"Name": Name("A")}}, "A"}, + {Dict{"DecodeParms": Array{Dict{"Name": Name("B")}}}, "B"}, + {Dict{"DecodeParms": Array{Integer(0)}}, ""}, + {Dict{"DecodeParms": Array{}}, ""}, + {Dict{"DecodeParms": Integer(0)}, ""}, + {Dict{}, ""}, + } { + got, _ := ToName(firstDecodeParms(c.d).Get("Name")) + if got != c.want { + t.Errorf("firstDecodeParms(%v)/Name = %q, want %q", c.d, got, c.want) + } + } +} + +// A stream whose chain begins with /Crypt /Identity was never encrypted, so +// decrypting it would turn readable bytes into noise. +func TestStreamIsPlain(t *testing.T) { + for _, c := range []struct { + d Dict + want bool + }{ + {Dict{"Filter": Name("Crypt")}, true}, + {Dict{"Filter": Name("Crypt"), "DecodeParms": Dict{"Name": Name("Identity")}}, true}, + {Dict{"Filter": Name("Crypt"), "DecodeParms": Dict{"Name": Name("StdCF")}}, false}, + {Dict{"Filter": Name("FlateDecode")}, false}, + {Dict{}, false}, + } { + if got := streamIsPlain(c.d); got != c.want { + t.Errorf("streamIsPlain(%v) = %v", c.d, got) + } + } +} + +// The decryptor leaves such a stream alone, and still decrypts its neighbours. +func TestDecryptorSkipsAPlainStream(t *testing.T) { + dec := &decryptor{revision: 4, streams: cryptRC4, strings: cryptRC4, key: []byte("0123456789abcdef")} + plain := &Stream{Dict: Dict{"Filter": Name("Crypt")}, Raw: []byte("")} + if got := dec.decryptObject(7, 0, plain).(*Stream); string(got.Raw) != "" { + t.Errorf("a plain stream was decrypted: %q", got.Raw) + } + other := &Stream{Dict: Dict{"Filter": Name("FlateDecode")}, Raw: []byte("")} + if got := dec.decryptObject(7, 0, other).(*Stream); string(got.Raw) == "" { + t.Error("an encrypted stream was left alone") + } +} diff --git a/filter.go b/filter.go index 9616eee..46add2a 100644 --- a/filter.go +++ b/filter.go @@ -191,10 +191,64 @@ func applyFilter(f Name, data []byte, parm Dict, r Resolver) ([]byte, error) { return runLengthDecode(data) case "CCITTFaxDecode", "CCF": return ccittDecode(data, ccittParamsOf(parm, r)) + case "Crypt": + return cryptFilter(data, parm, r) } return nil, fmt.Errorf("reader: unsupported filter /%s", f) } +// cryptFilter applies the /Crypt filter, which does not transform anything: it +// names the crypt filter a stream's bytes were encrypted with, and /Identity — +// which is also what /Crypt with no /Name means — says they were not encrypted +// at all. Either way the document that owns the stream has already dealt with +// its encryption by the time the filter chain runs, so there is nothing here +// left to undo. +// +// A /Name this reader cannot account for is reported rather than waved through, +// because the bytes would then be ciphertext that only looks like data. +func cryptFilter(data []byte, parm Dict, r Resolver) ([]byte, error) { + o, err := Resolve(parm.Get("Name"), r) + if err != nil { + return data, fmt.Errorf("reader: /Crypt filter: %w", err) + } + if n, ok := ToName(o); ok && n != "Identity" { + return data, fmt.Errorf("reader: /Crypt filter names /%s, which this reader cannot apply", n) + } + return data, nil +} + +// firstFilter names the first filter of a stream's chain. Only direct values +// are read: it is consulted while the file key is being established, before +// following an indirect reference is safe. +func firstFilter(d Dict) (Name, bool) { + switch v := d.Get("Filter").(type) { + case Name: + return v, true + case Array: + if len(v) > 0 { + n, ok := ToName(v[0]) + return n, ok + } + } + return "", false +} + +// firstDecodeParms is the parameter dictionary belonging to the first filter, +// read directly for the same reason. +func firstDecodeParms(d Dict) Dict { + switch v := d.Get("DecodeParms").(type) { + case Dict: + return v + case Array: + if len(v) > 0 { + if pd, ok := ToDict(v[0]); ok { + return pd + } + } + } + return nil +} + // salvage finishes a filter that stopped part-way. The prefix it did produce is // still worth having, and the predictor still applies to it: the predictors a // PDF may name all undo one row at a time, so a buffer that stops in the middle From 5a619406558b933716f993fb3a8079280cfeabb9 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 27 Aug 2026 15:09:38 +0200 Subject: [PATCH 4/8] Say what the rebuild found, and know the key before rebuilding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two faults in OpenWithPassword, one of them the masked error this branch went looking for. The masking. When the cross-reference tables cannot be read, Open rebuilds the file by scanning it; when that rebuild also fails, the error returned was the tables' error and the rebuild's was dropped on the floor: if rerr := d.repair(); rerr != nil { return nil, err } The tables' error only says that the tables could not be read. The rebuild's says what the file turned out to be, having read every object header in it. Measured over the 118 843 arXiv PDFs in /Users/Shared/axc, 30 files fail to open, and all 30 of them — 100% — reported the wrong thing: reported reader: no startxref in the last 4096 bytes 30/30 actually found reader: the file holds no indirect objects 28/30 reader: no document catalogue found 2/30 The 28 hold no PDF body at all: one is PostScript, the rest are truncated before their first object. "No startxref" sends a reader looking for a tail that was never the problem. The catalogue lookup's own error was thrown away the same way, replaced by a flat "the trailer does not lead to a catalog", so "/Root is a null" and "the catalogue has no page tree" arrived as one message. Both are kept now. The rebuild's diagnosis leads, since it is the one about the file as it is, and the tables' error follows in parentheses because it says why there was a rebuild at all. Wrapped with %w, so errors.Is still reaches ErrWrongPassword and the rest. The ordering. repair() reads object streams, and in an encrypted file it cannot read one until the file key is known — but the key was established after the rebuild, not before. So a file whose catalogue lives in an encrypted object stream was rebuilt from bytes nobody could read, found no catalogue, and was refused with the tables' error: "no startxref", for a file that needed a key. That is a file failing for a decryptable reason and reporting something else. repair() now establishes the key itself, before it indexes object streams, from any trailer a scan can see. That needs one more source than scanTrailers: a file written with cross-reference streams has no trailer keyword anywhere in it, and its /Encrypt then exists only in a /Type /XRef stream's dictionary — which is the one stream a PDF never encrypts, precisely so it can be read before the key is known. The bytes "/Encrypt" not appearing in the file at all short- circuits the whole thing, since no trailer is ever compressed. The document remembers its password so that a rebuild triggered later, by a bad offset mid-read, can do the same. Measured. The ordering fault bites no file in either corpus: it needs an encrypted file whose tables are unreadable, and none of the 118 843 arXiv PDFs or 2 268 forms is one. Shipped anyway, because the file that shows it is three lines of the package's own writer — NewPackedWriter, Encrypt, drop the startxref — and on v0.4.2 it reports "no startxref in the last 4096 bytes" where the answer is the password. The new tests fail on v0.4.2 and pass here. 118 843 arXiv open failures 30 -> 30, pages and per-file content hashes unchanged; all 30 messages now name what the rebuild found. Brotli-Prototype-FileA.pdf still cannot be opened — there is no Brotli in the standard library and this module takes no dependencies — but it now says "no document catalogue found (the cross-reference information was unusable too: reader: unsupported filter /BrotliDecode)" instead of the second half alone. 2 268 forms open failures unchanged, pages and hashes unchanged. The post-rebuild key setup this replaces is deleted rather than left dead. --- crypt_test.go | 31 ++++++++++--- document.go | 41 +++++++++-------- openerr_test.go | 114 ++++++++++++++++++++++++++++++++++++++++++++++++ repair.go | 73 +++++++++++++++++++++++++++++++ 4 files changed, 236 insertions(+), 23 deletions(-) create mode 100644 openerr_test.go diff --git a/crypt_test.go b/crypt_test.go index 56b8141..3e0ae9f 100644 --- a/crypt_test.go +++ b/crypt_test.go @@ -3,6 +3,8 @@ package reader import ( "bytes" "crypto/sha256" + "errors" + "strings" "testing" ) @@ -104,7 +106,7 @@ func TestMalformedEncryptDictionaries(t *testing.T) { {"not a dictionary", "/Encrypt 42"}, {"no /V or /R", "/Encrypt << /Filter /Standard >>"}, {"an unknown crypt filter method", "/Encrypt << /Filter /Standard /V 4 /R 4 /CF << /StdCF << /CFM /Nope >> >> /StmF /StdCF >>"}, - {"/U too short for revision 6", "/Encrypt << /Filter /Standard /V 5 /R 6 /U <00> >>"}, + {"/U too short for revision 6", "/Encrypt << /Filter /Standard /V 5 /R 6 /U <00> /CF << /StdCF << /CFM /AESV3 >> >> /StmF /StdCF /StrF /StdCF >>"}, } for _, c := range cases { b := replaceAll(onePage(), "/Root 1 0 R", "/Root 1 0 R "+c.enc) @@ -115,15 +117,28 @@ func TestMalformedEncryptDictionaries(t *testing.T) { } func TestEncryptDictionaryDefaults(t *testing.T) { - // An absurd /Length falls back to 40 bits, and a /CF entry naming a filter - // that is not there means no encryption for that class of data. + // An absurd /Length falls back to 40 bits. b := replaceAll(onePage(), "/Root 1 0 R", - "/Root 1 0 R /Encrypt << /Filter /Standard /V 4 /R 4 /Length 7 /StmF /Missing /StrF /Missing >>") + "/Root 1 0 R /Encrypt << /Filter /Standard /V 4 /R 4 /Length 7 /CF << /StdCF << /CFM /V2 >> >> /StmF /StdCF /StrF /StdCF >>") if _, err := Open(b); err != ErrWrongPassword { t.Errorf("got %v", err) } } +// A /StmF or /StrF that names a crypt filter /CF does not define means no +// encryption for that class of data: an absent entry is the identity filter, +// and naming one that is not there comes to the same thing. +func TestReadMethodsUnknownCryptFilterName(t *testing.T) { + dec := &decryptor{} + enc := Dict{"CF": Dict{}, "StmF": Name("Missing"), "StrF": Name("Missing")} + if err := dec.readMethods(enc, 4, nil); err != nil { + t.Fatal(err) + } + if dec.streams != cryptNone || dec.strings != cryptNone { + t.Errorf("streams %v, strings %v", dec.streams, dec.strings) + } +} + func TestFirstIDVariants(t *testing.T) { d := &Document{trailer: Dict{}} if got := d.firstID(); got != nil { @@ -341,9 +356,15 @@ func TestOpenRepairedEncryptedFileWithTheWrongPassword(t *testing.T) { // The tables are gone and the password is wrong: the rebuild succeeds and // the key derivation is what fails. b := withoutStartxref(encryptedFile(t, encOptions{v: 2, r: 3, length: 128, userPw: "hunter2"})) - if _, err := Open(b); err != ErrWrongPassword { + // The rebuild is what reports it now, because the rebuild is what needed + // the key; the wrapped error still names the password as the cause. + _, err := Open(b) + if !errors.Is(err, ErrWrongPassword) { t.Errorf("got %v, want ErrWrongPassword", err) } + if !strings.Contains(err.Error(), "no startxref") { + t.Errorf("the tables' own error was lost: %v", err) + } // With the password, the same damaged file opens. checkDecrypted(t, b, "hunter2") } diff --git a/document.go b/document.go index 5b263dc..78eb20c 100644 --- a/document.go +++ b/document.go @@ -22,6 +22,7 @@ type Document struct { loading map[int]bool objStms map[int]map[int]Object pages []Ref + password string decrypt *decryptor encryptNum int encryptKnown bool @@ -39,11 +40,12 @@ func Open(b []byte) (*Document, error) { return OpenWithPassword(b, "") } // as the user password and as the owner password, the empty one as well. func OpenWithPassword(b []byte, password string) (*Document, error) { d := &Document{ - buf: b, - xref: map[int]xrefEntry{}, - cache: map[int]Object{}, - loading: map[int]bool{}, - objStms: map[int]map[int]Object{}, + buf: b, + xref: map[int]xrefEntry{}, + cache: map[int]Object{}, + loading: map[int]bool{}, + objStms: map[int]map[int]Object{}, + password: password, } err := d.loadXref() if err == nil { @@ -51,24 +53,27 @@ func OpenWithPassword(b []byte, password string) (*Document, error) { return nil, derr } d.cache = map[int]Object{} - if _, cerr := d.Catalog(); cerr == nil { + cerr := error(nil) + if _, cerr = d.Catalog(); cerr == nil { return d, nil } // Tables that parse but lead nowhere are worse than none: rebuild. - err = fmt.Errorf("reader: the trailer does not lead to a catalog") + // What the catalogue lookup actually said is kept: "no page tree" and + // "/Root is a null" send a reader to different places. + err = fmt.Errorf("reader: the cross-reference tables do not lead to a catalogue: %w", cerr) } if rerr := d.repair(); rerr != nil { - return nil, err - } - if d.decrypt == nil { - if derr := d.setUpDecryption(password); derr != nil { - return nil, derr - } - // Whatever was read while the key was still unknown must be read again. - d.cache = map[int]Object{} - d.objStms = map[int]map[int]Object{} - d.pages = nil - } + // Both diagnoses matter, and the rebuild's is the one about the file + // as it really is: it has read every object header in it, where the + // tables only failed to be read. Returning the tables' error alone + // reported "no startxref" for a file that turned out to hold no + // objects at all, or an encrypted one whose catalogue could not be + // reached — a different problem with a different answer. + return nil, fmt.Errorf("%w (the cross-reference information was unusable too: %v)", rerr, err) + } + // Setting the key up again here used to be necessary, because the rebuild + // ran before anything knew the file was encrypted. repair() establishes it + // itself now, so by this point it is either known or not needed. return d, nil } diff --git a/openerr_test.go b/openerr_test.go new file mode 100644 index 0000000..fc5e00b --- /dev/null +++ b/openerr_test.go @@ -0,0 +1,114 @@ +package reader + +import ( + "errors" + "strings" + "testing" +) + +// A file whose catalogue lives in an encrypted object stream, and whose tables +// are gone: the rebuild has to know the key before it can read the stream, so +// establishing it has to come first. Otherwise the file is rebuilt from bytes +// nobody can read and refused for a reason that is not the reason. +func TestRepairOfAnEncryptedPackedFile(t *testing.T) { + for _, pw := range []string{"", "hunter2"} { + full := protectedFile(t, true, Encryption{UserPassword: pw}) + b := withoutStartxref(full) + d, err := OpenWithPassword(b, pw) + if err != nil { + t.Fatalf("password %q: %v", pw, err) + } + if !d.Repaired() { + t.Errorf("password %q: the file was not rebuilt", pw) + } + got, err := d.PageContent(1) + if err != nil || string(got) != "BT (hello) Tj ET" { + t.Errorf("password %q: content %q, %v", pw, got, err) + } + } +} + +// The rebuild's own diagnosis is what comes back, because the rebuild has read +// every object header in the file where the tables only failed to be read. The +// tables' error is kept alongside it, since it says why there was a rebuild. +func TestOpenReportsWhatTheRebuildFound(t *testing.T) { + cases := []struct { + name, file, wantRebuild, wantTables string + }{ + { + // PostScript, or a PDF truncated before its body: no objects. + name: "no objects at all", + file: "%!PS-Adobe-2.0\nnothing a reader can use\n", + wantRebuild: "the file holds no indirect objects", + wantTables: "no startxref", + }, + { + // Objects, but nothing that leads to a page. + name: "objects but no catalogue", + file: "%PDF-1.7\n1 0 obj\n<< /Type /Font >>\nendobj\n", + wantRebuild: "no document catalogue found", + wantTables: "no startxref", + }, + } + for _, c := range cases { + _, err := Open([]byte(c.file)) + if err == nil { + t.Errorf("%s: want an error", c.name) + continue + } + if !strings.Contains(err.Error(), c.wantRebuild) { + t.Errorf("%s: %q does not say %q", c.name, err, c.wantRebuild) + } + if !strings.Contains(err.Error(), c.wantTables) { + t.Errorf("%s: %q loses the tables' error %q", c.name, err, c.wantTables) + } + } +} + +// Tables that read but lead nowhere keep what the catalogue lookup said, which +// is not the same thing twice: "/Root is a null" and "no page tree" send a +// reader to different places. +func TestOpenKeepsTheCatalogueError(t *testing.T) { + // The catalogue has no page tree, and nothing else in the file calls + // itself a page either, so the rebuild has nothing to fall back on. + // The replacements keep the byte length, so the tables still read: it is + // the catalogue lookup that has to be what fails. + b := replaceAll(onePage(), "/Type /Catalog /Pages 2 0 R", "/Type /Catalog /Pagez 2 0 R") + b = replaceAll(b, "/Type /Page /Parent", "/Type /Pige /Parent") + _, err := Open(b) + if err == nil { + t.Fatal("want an error") + } + if !strings.Contains(err.Error(), "no page tree") { + t.Errorf("%q does not say what the catalogue lookup found", err) + } +} + +// The bytes "/Encrypt" can appear in a file that is not encrypted at all — in +// a content stream, say — and the rebuild must not conclude anything from that. +func TestRepairIgnoresAStrayEncryptMention(t *testing.T) { + b := newBuilder() + b.obj(1, "<< /Type /Catalog /Pages 2 0 R >>") + b.obj(2, "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 1 1] >>") + b.obj(3, "<< /Type /Page /Parent 2 0 R /Contents 4 0 R >>") + b.streamObj(4, "", []byte("BT (/Encrypt) Tj ET")) + d, err := Open(withoutStartxref(b.table("/Root 1 0 R"))) + if err != nil { + t.Fatal(err) + } + if d.Encrypted() { + t.Error("the file reports itself encrypted") + } + if got, err := d.PageContent(1); err != nil || string(got) != "BT (/Encrypt) Tj ET" { + t.Errorf("content %q, %v", got, err) + } +} + +// A rebuild that fails on the key says so, and stays matchable. +func TestRepairReportsAWrongPassword(t *testing.T) { + b := withoutStartxref(protectedFile(t, true, Encryption{UserPassword: "right"})) + _, err := OpenWithPassword(b, "wrong") + if !errors.Is(err, ErrWrongPassword) { + t.Fatalf("got %v", err) + } +} diff --git a/repair.go b/repair.go index 5ee32f4..1200f87 100644 --- a/repair.go +++ b/repair.go @@ -26,6 +26,15 @@ func (d *Document) repair() error { if len(d.xref) == 0 { return fmt.Errorf("reader: the file holds no indirect objects") } + // A rebuild reads object streams, and in an encrypted file it cannot read + // one until the file key is known. The trailers a scan can see are where + // /Encrypt is named, so the key is established before the streams are + // indexed rather than after: otherwise a file whose catalogue lives in an + // encrypted object stream is rebuilt from bytes nobody can read, and the + // rebuild fails for a reason that has nothing to do with the file. + if err := d.establishDecryption(); err != nil { + return err + } // Objects held in object streams are invisible to a header scan; take them // from every object stream the scan did find. d.indexObjectStreams() @@ -36,6 +45,70 @@ func (d *Document) repair() error { return nil } +// establishDecryption derives the file key from a trailer the scan can see, for +// a rebuild that runs before the cross-reference tables could name /Encrypt. +// A file that is not encrypted, or whose key is already known, needs nothing. +func (d *Document) establishDecryption() error { + if d.decrypt != nil || !bytes.Contains(d.buf, []byte("/Encrypt")) { + // /Encrypt can only be named by a trailer, and no trailer is + // compressed, so a file that does not hold those bytes anywhere is not + // encrypted. Saying so cheaply keeps the scan below off the common path. + return nil + } + previous := d.trailer + for _, tr := range d.trailerCandidates() { + if tr == nil || tr.Get("Encrypt").Kind() == KindNull { + continue + } + d.trailer = tr + err := d.setUpDecryption(d.password) + d.trailer = previous + if err != nil { + return err + } + // Whatever was read while the key was unknown was read wrong. + d.cache = map[int]Object{} + return nil + } + return nil +} + +// trailerCandidates lists every dictionary that could carry trailer entries: +// what the tables managed to read, then the file's trailer keywords, then the +// dictionary of each cross-reference stream. +// +// The last of those is not optional. A file written with cross-reference +// streams has no trailer keyword anywhere in it, and /Encrypt then exists only +// in a /Type /XRef stream's dictionary — which is the one stream a PDF never +// encrypts, precisely so that it can be read before the key is known. +// +// The cross-reference streams come highest object number first, an incremental +// update having appended both its new objects and its new table to the file. +func (d *Document) trailerCandidates() []Dict { + out := append([]Dict{d.trailer}, scanTrailers(d.buf)...) + + nums := make([]int, 0, len(d.xref)) + for num := range d.xref { + nums = append(nums, num) + } + slices.Sort(nums) + var streams []Dict + for _, num := range nums { + // An error here leaves a nil object, which is not a stream; the + // rebuild has already been told about anything unreadable. + o, _ := d.Get(Ref{Num: num}) + s, ok := ToStream(o) + if !ok { + continue + } + if t, ok := ToName(s.Dict.Get("Type")); ok && t == "XRef" { + streams = append(streams, s.Dict) + } + } + slices.Reverse(streams) + return append(out, streams...) +} + // loadRepairedTrailer finds a trailer that leads to a catalogue: the file's own // trailer dictionaries first, newest first; then any object that calls itself a // catalogue; and failing both, a catalogue built over whatever pages survive. From 4f2914f81c50b455a5efb5360a30f33f97fd6298 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 27 Aug 2026 15:10:08 +0200 Subject: [PATCH 5/8] Open a file whose crypt filters say nothing in it is encrypted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /StmF and /StrF name the crypt filters that protect a file's streams and strings, and both default to /Identity, which leaves bytes alone. A file where both are /Identity — or name a filter its own /CF does not define, which comes to the same thing — has nothing encrypted in its body. /Encrypt is there for /EFF, the embedded files, and for the permission bits. This reader demanded a valid password for such a file anyway, and refused it. Nothing was gained by refusing: the key it was asking for would have been used to decrypt nothing, because both methods resolve to cryptNone either way. The file's every byte is already plain text. Measured over 12 172 files — the 1633 real forms, the 635 vendor fixtures and 9904 arXiv PDFs — 495 declare /Encrypt with readable tables, and their crypt filters divide up like this: /StmF and /StrF both /StdCF 441 both absent, and /V below 4 (no /StmF) 47 both /DefaultCryptFilter 4 both /Identity, /V 5 3 Only the last three are affected, so the blast radius is exactly three files, all openpdf fixtures, all /V 5 /R 6 with /EFF /StdCF and no user password this reader can guess. Before: "reader: the password does not open this file". Now they open, with 30, 1 and 30 pages — which is what poppler's pdfinfo reports for the same three files, and it calls them "Encrypted: no" for this reason. The 47 files with no /StmF are untouched: below /V 4 there are no crypt filters and RC4 protects everything, which readMethods already knows. Nothing is authenticated on this path, and the reader says so rather than implying otherwise: Encrypted() still reports true, Protection() reports the revision and the /P permissions, Method is "none" — the method that really applies to the content — and Owner is false, because no password was checked. This unblocks the fixture that made the case for dbb2558. openpdf's issue375_unfilterable-with-crypt.pdf carries a stream whose /Filter is [/Crypt /ZlateDecode], a filter name invented to be unimplementable. It could not be reached at all before, because the file could not be opened; it opens now, all 30 pages, and the invented filter is salvage rather than a lost page. --- crypt.go | 12 ++++++++++++ crypt_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/crypt.go b/crypt.go index 9b2ac46..264288e 100644 --- a/crypt.go +++ b/crypt.go @@ -169,6 +169,18 @@ func newDecryptor(enc Dict, id []byte, password string, r Resolver) (*decryptor, } dec.perm = Permissions(uint32(perm)) & AllPermissions + if dec.streams == cryptNone && dec.strings == cryptNone { + // /StmF and /StrF both name the identity crypt filter, so nothing in + // the document body is encrypted and no key is needed to read it. A + // file like that is opened without validating the password: refusing + // it would refuse a document whose every byte is already plain, which + // is what every other reader shows. Only /EFF — the embedded files — + // would need the key, and this reader does not hand those out. + // + // Nothing was authenticated, so dec.owner stays false and Protection + // reports the method as "none": a caller can see exactly what it got. + return dec, nil + } if rev >= 5 { key, asOwner, err := deriveKeyR5(enc, password, r) if err != nil { diff --git a/crypt_test.go b/crypt_test.go index 3e0ae9f..848c1ed 100644 --- a/crypt_test.go +++ b/crypt_test.go @@ -125,6 +125,38 @@ func TestEncryptDictionaryDefaults(t *testing.T) { } } +// A file whose /StmF and /StrF both name the identity crypt filter has nothing +// encrypted in its body, so it opens with no password at all — including when +// its /Encrypt dictionary is in no state to derive a key from. /Encrypt is +// still reported, with the method it really applies to the content: none. +func TestIdentityCryptFiltersNeedNoPassword(t *testing.T) { + for _, enc := range []string{ + // No /StmF or /StrF: both default to /Identity. + "/Encrypt << /Filter /Standard /V 5 /R 6 /U <00> /EFF /StdCF >>", + // Named outright. + "/Encrypt << /Filter /Standard /V 4 /R 4 /StmF /Identity /StrF /Identity >>", + // Naming a crypt filter that /CF does not define comes to the same. + "/Encrypt << /Filter /Standard /V 4 /R 4 /StmF /Missing /StrF /Missing >>", + } { + b := replaceAll(onePage(), "/Root 1 0 R", "/Root 1 0 R "+enc) + d, err := Open(b) + if err != nil { + t.Errorf("%s: %v", enc, err) + continue + } + if !d.Encrypted() { + t.Errorf("%s: /Encrypt not reported", enc) + } + p, ok := d.Protection() + if !ok || p.Method != "none" || p.Owner { + t.Errorf("%s: protection %+v, %v", enc, p, ok) + } + if got, err := d.PageContent(1); err != nil || len(got) == 0 { + t.Errorf("%s: content %q, %v", enc, got, err) + } + } +} + // A /StmF or /StrF that names a crypt filter /CF does not define means no // encryption for that class of data: an absent entry is the identity filter, // and naming one that is not there comes to the same thing. From 062161b898a159bb9d476e3d8e17b88134db7a8e Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 27 Aug 2026 15:16:33 +0200 Subject: [PATCH 6/8] Let the later object stream win, as the header scan already does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 19dc306 made the rebuild deterministic by giving a contested object number to the lowest-numbered object stream. Sorting by object number was the wrong tie-break, and the corpus said so. Comparing 1184 real files cut to 92% of their length against v0.4.2, 120 files extracted more page content and one extracted less. That one is worth spelling out, because "got worse" is not quite what happened: on v0.4.2 the file has no single answer at all. Six runs of the same binary on the same bytes: 89170 / 85910 / 88937 / 99927 / 99961 / 102097 bytes 19dc306 gave it one answer, 84229, which is below all six samples. So it was not a regression against a baseline — there was no baseline — but it was still a worse choice than the file could support. The rule now matches the one the header scan a few lines above already applies to objects written directly: a later definition wins, an incremental update having appended it. Object streams are walked latest in the file first, so the newer of two streams claiming the same object defines it. 1184 truncated files v0.4.2 branch pages returning nothing 591 0 pages 3780 3780 content bytes 126452734 137250792 (+8.54%) streams the strict Decode refuses 12371 22 open failures 136 136 files extracting more — 120 files extracting less — 0 Deterministic across three runs: 137250792 bytes, hash abdff1d485f8dd38, every time. Of those bytes, 137209833 are clean decodes and 36185 across 4 pages are flagged salvage — so the gain is decoded content, not compressed bytes counted as if they were content. The test now writes the higher-numbered stream first, which a rule going by object number would fail. --- repair.go | 27 +++++++++++++++++++++------ repair_test.go | 5 +++-- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/repair.go b/repair.go index 1200f87..4a628d8 100644 --- a/repair.go +++ b/repair.go @@ -2,6 +2,7 @@ package reader import ( "bytes" + "cmp" "fmt" "slices" ) @@ -193,16 +194,25 @@ func (d *Document) synthesiseCatalogue() { // indexObjectStreams adds the objects held inside every object stream the scan // found, without overwriting an object written directly in the file. func (d *Document) indexObjectStreams() { - // The object numbers are sorted before they are walked. Two object streams - // in a damaged file may both claim the same object number, and the first - // one walked wins; taking them in map order makes the same file read - // differently from one run to the next. + // Two object streams in a damaged file may both claim the same object + // number — a partial rewrite leaves the old one in place beside the new — + // and the loop below lets the first stream walked define it. Walking + // d.xref in map order therefore made the same file read differently from + // one run to the next. + // + // They are walked latest in the file first, which is the rule the header + // scan above already applies to objects written directly: a later + // definition wins, an incremental update having appended it. nums := make([]int, 0, len(d.xref)) for num := range d.xref { nums = append(nums, num) } slices.Sort(nums) - var streams []int + type located struct { + offset int64 + num int + } + var found []located for _, num := range nums { o, err := d.Get(Ref{Num: num}) if err != nil { @@ -213,9 +223,14 @@ func (d *Document) indexObjectStreams() { continue } if t, ok := ToName(s.Dict.Get("Type")); ok && t == "ObjStm" { - streams = append(streams, num) + found = append(found, located{d.xref[num].offset, num}) } } + slices.SortFunc(found, func(a, b located) int { return cmp.Compare(b.offset, a.offset) }) + streams := make([]int, 0, len(found)) + for _, f := range found { + streams = append(streams, f.num) + } for _, num := range streams { // The stream object is already cached by the pass above, so this // cannot fail; an empty result simply contributes nothing. diff --git a/repair_test.go b/repair_test.go index e94bd45..8560ad6 100644 --- a/repair_test.go +++ b/repair_test.go @@ -560,8 +560,9 @@ func TestIndexObjectStreamsIsDeterministic(t *testing.T) { b.obj(1, "<< /Type /Catalog /Pages 2 0 R >>") b.obj(2, "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 1 1] >>") b.obj(3, "<< /Type /Page /Parent 2 0 R >>") - // The higher-numbered stream is written first, so agreeing with the file's - // own order would not be enough to pass. + // The later stream in the file is the one that must win, so the + // higher-numbered one is written first: a rule that went by object number + // would pick the other and fail here. b.streamObj(objStm(20, "fromTwenty")) b.streamObj(objStm(10, "fromTen")) // No startxref, so the tables cannot be read and the rebuild runs. From fed7c59bc0977c9c4f9bd3fb5ad911953560df0b Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 27 Aug 2026 15:18:13 +0200 Subject: [PATCH 7/8] Describe the salvage and the rebuild as they now behave The filters list gains /Crypt, which the encryption paragraph below already claimed. Two paragraphs say what a reader has to be told: which of Decode and DecodeRecovering it wants, and that a rebuild is now deterministic, can reach an encrypted object stream, and reports what it found. --- README.md | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index ed07f6a..26cf9ca 100644 --- a/README.md +++ b/README.md @@ -29,16 +29,36 @@ the **document structure**: resolved the way the specification requires. - **Filters** — `FlateDecode`, `LZWDecode` (with PDF's `EarlyChange`, which the standard library's LZW does not implement), `ASCIIHexDecode`, - `ASCII85Decode`, `RunLengthDecode`, plus the PNG and TIFF predictors. - Image filters (`DCTDecode`, `JPXDecode`, `CCITTFaxDecode`, `JBIG2Decode`) are - reported rather than applied, so an image consumer gets the encoded bytes. + `ASCII85Decode`, `RunLengthDecode`, `Crypt`, plus the PNG and TIFF + predictors. Image filters (`DCTDecode`, `JPXDecode`, `CCITTFaxDecode`, + `JBIG2Decode`) are reported rather than applied, so an image consumer gets + the encoded bytes. +- **Filters that fail** — a chain that cannot be run to the end is not the end + of the stream. `DecodeRecovering` returns what the filters before the failure + produced — the prefix a damaged Flate stream did inflate, not its compressed + bytes — and says so through `Decoded.Recovered` and `Decoded.Cause`. + `Decode` is the strict reading and refuses such a stream outright, so a + caller that must not act on damaged data says which it wants by which one it + calls. Page content takes the lenient reading, because a page whose content + will not decode is still a page; `PageContentDecoded` reports whether any + salvaging happened. Cross-reference streams take the strict one: a table that + has to be guessed at is worse than no table, and the repair below is the + answer to one. - **Cross-references** — classic tables, cross-reference **streams**, **object streams**, `/Prev` chains, and the `/XRefStm` of a hybrid file, newest definition winning. - **Repair** — a file whose tables are missing, truncated or simply wrong is rebuilt by scanning it for object headers, trailers and, failing those, for a catalogue; a file that kept its pages but lost its catalogue gets one. - This is not an exceptional path: it is what makes a reader usable. + This is not an exceptional path: it is what makes a reader usable. The + rebuild establishes the file key before it reads object streams, so a file + whose catalogue lives in an encrypted one can be rebuilt at all; it takes + `/Encrypt` from a cross-reference stream's dictionary when the file has no + trailer keyword to name it in. Where two object streams claim the same + object, the later one in the file wins, the same rule the header scan + applies to objects written directly — so the same damaged file reads the + same way twice. When a rebuild fails, the error says what the rebuild found + and not merely that the tables could not be read. - **Documents** — `Open`, object resolution with cycle and recursion guards, the trailer, the catalogue, and the page tree with the four attributes a page inherits from its ancestors. From c89f94c8175112412df5a031444ac48ac3368283 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Thu, 27 Aug 2026 15:31:12 +0200 Subject: [PATCH 8/8] Keep undecoded bytes out of the field a caller paints from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A flag is not a guarantee. dbb2558 returned the bytes a failed chain could not get past in Decoded.Data with Recovered set beside them, which asks every caller to check a field before believing another one. render has just been found painting 273 encoded image masks as one-bit samples on 51 first pages of real forms — noise, unnoticed, because a page with noise on it looks like a page with something on it. That is this shape of bug, and a fallback that can feed it is not finished. So the two kinds of bytes go in different fields: Decoded.Data what the chain decoded. Fewer bytes than the stream meant to carry when Recovered is set, but bytes of that kind. Decoded.Undecoded what the chain could not get past, still in the encoding Filter names. Set instead of Data, never beside it. A caller reading Data cannot be handed a compressed stream by accident, and one that wants the stream as it lies asks for it by a name that says what it is. Two holes showed up the moment the guarantee was written down as a test, both of them the exact failure described above: - cryptFilter returned the bytes it could not decrypt alongside its error, so ciphertext arrived as Data. It returns nothing now. - v0.5.0 decodes /CCITTFaxDecode in applyFilter rather than stopping at it, so a fax whose /Columns is absurd, or whose pixel count runs past the limit, now fails inside the chain. Its still-encoded bytes were landing in Data — a fax handed to a stencil path as though it were samples. TestUndecodedBytesNeverArriveAsData asserts it over six shapes of failure (unimplemented filter, Flate that is not Flate, two bad faxes, an unreadable /Filter, an unapplicable crypt filter), and FuzzDecodeRecovering asserts that Data and Undecoded are never both set and that Undecoded never appears on a clean decode. Page content follows: a page whose content stream cannot be decoded at all now yields no bytes rather than compressed ones, and PageContentDecoded says why. Object streams likewise parse only Data, so a rebuild cannot take compressed bytes for objects. Measured, v0.5.0 vs this branch: 1633 real forms pages 9362 -> 9362, content bytes 224709710 unchanged, every per-file content hash identical, open failures 0. Streams the strict Decode refuses 263 -> 87. 635 vendor fx open failures 31 -> 28, pages 1236 -> 1297, 3 files change. 1184 truncated open failures 136 -> 136, pages 3780 -> 3780, content 126466671 -> 137214633 bytes (+8.50%), pages returning nothing 591 -> 0, strict refusals 12373 -> 22. Of the branch's 137 214 633 bytes, 137 214 607 are clean decodes and 26 across 4 pages are flagged salvage; a further 29 320 bytes sit in Undecoded on 95 pages and are counted nowhere near the content total, which is the point. The byte figure went down by 36 159 against the pre-split design and is worth more for it: what it counts is now decoded content only. One of the 1184 extracts fewer bytes than the v0.5.0 sample in that table (1227119 -> 1213608). v0.5.0 has no single answer for that file: six runs give 1213608, 1218311, 1221792, 1223638 twice, and 1227119. The branch's answer is one of them, arrived at the same way every time. The two openpdf fixtures, re-measured after the rebase: unfilterable-with-crypt.pdf opens, 30 pages — poppler's number. 38 of its 40 streams decode cleanly; the 2 whose /Filter is [/Crypt /ZlateDecode] are Undecoded, so the invented filter cannot masquerade as anything. Brotli-Prototype-FileA.pdf still refused, and rightly: its cross-reference stream is Brotli, there is no Brotli in the standard library, and a table that has to be guessed at is the one place salvage is wrong. --- README.md | 5 +++- content.go | 5 ++++ content_test.go | 9 +++++-- filter.go | 54 +++++++++++++++++++++++++++++--------- recover_test.go | 69 +++++++++++++++++++++++++++++++++++++++++++------ 5 files changed, 119 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 26cf9ca..c10abea 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,10 @@ the **document structure**: - **Filters that fail** — a chain that cannot be run to the end is not the end of the stream. `DecodeRecovering` returns what the filters before the failure produced — the prefix a damaged Flate stream did inflate, not its compressed - bytes — and says so through `Decoded.Recovered` and `Decoded.Cause`. + bytes — and says so through `Decoded.Recovered` and `Decoded.Cause`. Bytes no + filter decoded arrive in `Decoded.Undecoded`, never in `Decoded.Data`, so a + compressed stream cannot be painted as samples or tokenised as content by a + caller that forgot to check a flag. `Decode` is the strict reading and refuses such a stream outright, so a caller that must not act on damaged data says which it wants by which one it calls. Page content takes the lenient reading, because a page whose content diff --git a/content.go b/content.go index 383f356..5b8b5e4 100644 --- a/content.go +++ b/content.go @@ -297,6 +297,10 @@ func (d *Document) contentOf(page Dict) (Decoded, error) { part := d.decodedContent(s) if part.Recovered && !out.Recovered { out.Recovered, out.Cause, out.Filter = true, part.Cause, part.Filter + out.Undecoded = part.Undecoded + } + if len(part.Data) == 0 { + continue } if len(out.Data) > 0 { out.Data = append(out.Data, '\n') @@ -315,6 +319,7 @@ func (d *Document) decodedContent(s *Stream) Decoded { dec := d.DecodeStreamRecovering(s) if dec.Image != "" { return Decoded{ + Undecoded: dec.Data, Recovered: true, Filter: dec.Image, Cause: fmt.Errorf("reader: a content stream is filtered as an image (/%s)", dec.Image), diff --git a/content_test.go b/content_test.go index f9b6e65..4ddf42e 100644 --- a/content_test.go +++ b/content_test.go @@ -275,8 +275,13 @@ func TestPageContentUndecodable(t *testing.T) { if !dec.Recovered || dec.Cause == nil || dec.Filter != "FlateDecode" { t.Errorf("%s: got %+v", contents, dec) } - if string(dec.Data) != "not deflate data" { - t.Errorf("%s: got %q, want the raw bytes", contents, dec.Data) + // The bytes come back, but not as content: a scanner must not be + // handed a compressed stream to tokenise. + if len(dec.Data) != 0 { + t.Errorf("%s: undecoded bytes arrived as content: %q", contents, dec.Data) + } + if string(dec.Undecoded) != "not deflate data" { + t.Errorf("%s: got %q, want the raw bytes", contents, dec.Undecoded) } } } diff --git a/filter.go b/filter.go index 46add2a..c26248b 100644 --- a/filter.go +++ b/filter.go @@ -33,18 +33,45 @@ func ImageFilter(n Name) bool { // A Decoded is the outcome of applying a stream's filter chain, including the // outcome of a chain that could not be finished. +// +// The point of the type is that a caller can tell the three outcomes apart +// without having to be careful. Bytes a filter decoded and bytes no filter +// decoded arrive in different fields, so the second kind cannot be painted as +// samples or tokenised as content by a caller that forgot to check a flag. type Decoded struct { - // Data is what the chain produced. + // Data is what the chain decoded. A recovered decode leaves fewer bytes + // here than the stream meant to carry, but they are bytes of the kind it + // meant to carry: Data never holds bytes that no filter has decoded. + // + // The one case where Data is still encoded is the deliberate one: a chain + // that stopped at an image filter, which Image names and Recovered does + // not flag, because stopping there is the contract. Data []byte + + // Undecoded holds the bytes the chain could not get past, still in the + // encoding Filter names. It is set instead of Data — never beside it — + // when the filter that failed produced nothing at all. + // + // This is where a compressed content stream, a truncated fax, or a filter + // nobody implements ends up. A caller that wants the stream as it lies has + // to ask for it by a name that says what it is; a caller that reads Data + // cannot be handed it by accident. Painting undecoded bytes as a one-bit + // image looks like a page with something on it, which is why nothing about + // this is left to a flag. + Undecoded []byte + // Image names the image filter the chain stopped at, when it stopped at - // one; Data is then still encoded in that filter. + // one; Data is then the bytes still encoded in it. Image Name - // Recovered says Data is what could be salvaged from a chain that could - // not be run to the end, not a clean decode. A caller that must not act on - // damaged data stops here — or calls [Decode], which refuses outright. + + // Recovered says the chain could not be run to the end. A caller that must + // not act on damaged data stops here — or calls [Decode], which refuses + // outright. Recovered bool - // Cause says why the chain stopped, and is set only when Recovered is. + + // Cause says why the chain stopped, and is set exactly when Recovered is. Cause error + // Filter names the filter that could not be applied, when one is to blame: // a chain whose /Filter entry itself is unreadable blames nothing. Filter Name @@ -59,12 +86,13 @@ type Decoded struct { // // The salvage is always as far down the chain as the filters got, never the // bytes as they arrived: a damaged Flate stream yields the prefix it did -// inflate, not the compressed bytes. Only a filter that produced nothing at -// all falls back to what went into it. +// inflate, in [Decoded.Data]. A filter that produced nothing at all leaves the +// bytes in [Decoded.Undecoded] instead, still in its encoding, because that is +// what they are. func DecodeRecovering(d Dict, raw []byte, resolve Resolver) Decoded { filters, parms, err := filterChain(d, resolve) if err != nil { - return Decoded{Data: raw, Recovered: true, Cause: err} + return Decoded{Undecoded: raw, Recovered: true, Cause: err} } data := raw for i, f := range filters { @@ -74,7 +102,7 @@ func DecodeRecovering(d Dict, raw []byte, resolve Resolver) Decoded { out, err := applyFilter(f, data, parms[i], resolve) if err != nil { if len(out) == 0 { - out = data + return Decoded{Undecoded: data, Recovered: true, Cause: err, Filter: f} } return Decoded{Data: out, Recovered: true, Cause: err, Filter: f} } @@ -209,10 +237,12 @@ func applyFilter(f Name, data []byte, parm Dict, r Resolver) ([]byte, error) { func cryptFilter(data []byte, parm Dict, r Resolver) ([]byte, error) { o, err := Resolve(parm.Get("Name"), r) if err != nil { - return data, fmt.Errorf("reader: /Crypt filter: %w", err) + return nil, fmt.Errorf("reader: /Crypt filter: %w", err) } if n, ok := ToName(o); ok && n != "Identity" { - return data, fmt.Errorf("reader: /Crypt filter names /%s, which this reader cannot apply", n) + // No bytes come back: they are ciphertext, and handing them over as + // though the filter had run is how ciphertext gets painted. + return nil, fmt.Errorf("reader: /Crypt filter names /%s, which this reader cannot apply", n) } return data, nil } diff --git a/recover_test.go b/recover_test.go index b10b94f..a5c6b22 100644 --- a/recover_test.go +++ b/recover_test.go @@ -29,15 +29,19 @@ func TestDecodeRecoveringImageFilter(t *testing.T) { } // A filter nobody implements ends the chain, and the bytes as they stand come -// back flagged rather than not at all. +// back flagged rather than not at all — as Undecoded, since nothing decoded +// them. func TestDecodeRecoveringUnknownFilter(t *testing.T) { dec := DecodeRecovering(Dict{"Filter": Name("BrotliDecode")}, []byte("brotli bytes"), nil) if !dec.Recovered || dec.Cause == nil { t.Fatalf("got %+v", dec) } - if dec.Filter != "BrotliDecode" || string(dec.Data) != "brotli bytes" { + if dec.Filter != "BrotliDecode" || string(dec.Undecoded) != "brotli bytes" { t.Errorf("got %+v", dec) } + if len(dec.Data) != 0 { + t.Errorf("undecoded bytes arrived as Data: %q", dec.Data) + } // The strict reading refuses the same stream outright. if _, _, err := Decode(Dict{"Filter": Name("BrotliDecode")}, []byte("brotli bytes"), nil); err == nil { t.Error("Decode: want an error") @@ -53,8 +57,13 @@ func TestDecodeRecoveringSalvagesDownTheChain(t *testing.T) { if !dec.Recovered || dec.Filter != "Nope" { t.Fatalf("got %+v", dec) } - if string(dec.Data) != "inflated already" { - t.Errorf("got %q, want the inflated prefix", dec.Data) + // The bytes are inflated, but they are still whatever /Nope encodes, so + // they are Undecoded and not content. + if string(dec.Undecoded) != "inflated already" { + t.Errorf("got %q, want the inflated bytes", dec.Undecoded) + } + if len(dec.Data) != 0 { + t.Errorf("bytes still in a filter's encoding arrived as Data: %q", dec.Data) } } @@ -64,8 +73,8 @@ func TestDecodeRecoveringUnreadableFilterEntry(t *testing.T) { if !dec.Recovered || dec.Cause == nil || dec.Filter != "" { t.Fatalf("got %+v", dec) } - if string(dec.Data) != "as it lies" { - t.Errorf("got %q", dec.Data) + if string(dec.Undecoded) != "as it lies" || len(dec.Data) != 0 { + t.Errorf("got %+v", dec) } } @@ -156,14 +165,14 @@ func TestLZWKeepsItsPrefix(t *testing.T) { // The stream helpers agree with the dictionary ones. func TestDecodeStreamRecovering(t *testing.T) { s := &Stream{Dict: Dict{"Filter": Name("Nope")}, Raw: []byte("raw")} - if dec := DecodeStreamRecovering(s, nil); !dec.Recovered || string(dec.Data) != "raw" { + if dec := DecodeStreamRecovering(s, nil); !dec.Recovered || string(dec.Undecoded) != "raw" { t.Errorf("package: got %+v", dec) } d, err := Open(onePage()) if err != nil { t.Fatal(err) } - if dec := d.DecodeStreamRecovering(s); !dec.Recovered || string(dec.Data) != "raw" { + if dec := d.DecodeStreamRecovering(s); !dec.Recovered || string(dec.Undecoded) != "raw" { t.Errorf("document: got %+v", dec) } } @@ -209,6 +218,44 @@ func TestObjectStreamSalvagesItsPrefix(t *testing.T) { } } +// The guarantee the split exists for: bytes no filter decoded never arrive in +// Data, whatever the filter and whatever the shape of the failure. A stencil +// path reading Data cannot paint a compressed stream as a one-bit image, and a +// content scanner reading Data cannot tokenise one. +func TestUndecodedBytesNeverArriveAsData(t *testing.T) { + for _, c := range []struct { + what string + d Dict + raw []byte + }{ + {"a filter nobody implements", Dict{"Filter": Name("BrotliDecode")}, []byte("\x1b\x2e\x00")}, + {"Flate that is not Flate", Dict{"Filter": Name("FlateDecode")}, []byte("not compressed at all")}, + {"a fax with no columns", Dict{"Filter": Name("CCITTFaxDecode"), + "DecodeParms": Dict{"Columns": Integer(0)}}, []byte("\x00\xff\x00\xff")}, + {"a fax past the pixel limit", Dict{"Filter": Name("CCITTFaxDecode"), + "DecodeParms": Dict{"Columns": Integer(1 << 20), "Rows": Integer(1 << 20)}}, []byte("\x26\xa0")}, + {"an unreadable /Filter", Dict{"Filter": Real(2.5)}, []byte("who knows")}, + {"a crypt filter that cannot be applied", Dict{"Filter": Name("Crypt"), + "DecodeParms": Dict{"Name": Name("StdCF")}}, []byte("ciphertext")}, + } { + dec := DecodeRecovering(c.d, c.raw, nil) + if !dec.Recovered || dec.Cause == nil { + t.Errorf("%s: not reported as recovered: %+v", c.what, dec) + continue + } + if len(dec.Data) != 0 { + t.Errorf("%s: %d undecoded bytes arrived as Data", c.what, len(dec.Data)) + } + if !bytes.Equal(dec.Undecoded, c.raw) { + t.Errorf("%s: Undecoded = %q, want the bytes as they lie", c.what, dec.Undecoded) + } + // And the strict reading gives nothing at all. + if got, _, err := Decode(c.d, c.raw, nil); err == nil || got != nil { + t.Errorf("%s: Decode returned %q, %v", c.what, got, err) + } + } +} + // FuzzDecodeRecovering asserts the contract the salvage rests on: it never // panics and never reports a clean decode it did not make. func FuzzDecodeRecovering(f *testing.F) { @@ -227,6 +274,12 @@ func FuzzDecodeRecovering(f *testing.F) { if dec.Recovered != (dec.Cause != nil) { t.Fatalf("Recovered and Cause disagree: %+v", dec) } + if len(dec.Undecoded) > 0 && len(dec.Data) > 0 { + t.Fatalf("Data and Undecoded both set: %+v", dec) + } + if len(dec.Undecoded) > 0 && !dec.Recovered { + t.Fatalf("Undecoded set on a clean decode: %+v", dec) + } data, img, err := Decode(d, raw, nil) if (err != nil) != dec.Recovered { t.Fatalf("Decode and DecodeRecovering disagree: %v vs %+v", err, dec)