From 76721e35579117060fce93f676c81ad159be13b2 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sat, 14 Nov 2026 22:48:51 +0100 Subject: [PATCH] =?UTF-8?q?linearize:=20bound=20/S=20above=20where=20it=20?= =?UTF-8?q?costs=20nothing=20=E2=80=94=20an=20unfiltered=20hint=20stream?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⭐ THE OPEN LIMIT WAS SMALLER THAN I SAID. /S indexes the DECODED data, so bounding it above normally means decoding an attacker's bytes in the cheap path — filter.go caps expansion at 1 GiB, which refuses a bomb but is three orders of magnitude past anything a hint stream needs. That is still true and still wants a size bound derived from /N and the object count (#40). But a stream with NO FILTER needs no decoding at all: its raw bytes ARE its data. That case is free, and it is the one that matters most while a writer is being developed, because an uncompressed hint stream is what somebody debugging one emits. Ghostscript already writes one unfiltered, and qpdf does under --compress-streams=n. ⛔⛔ AND THE FILTER CONDITION IS LOAD-BEARING, WHICH THE CORPUS PROVED. Applying the bound to filtered streams as well turns the generated sweep's TWENTY-FIVE page cases red — `as qpdf writes it, 25_pages` and `with object streams, 25_pages`. For a document that size the hint table compresses well below /S, so /S is past the RAW length while sitting comfortably inside the DECODED one. The 1, 2 and 5 page cases cannot see it: their tables are too small to compress under 43. So this is not defensive coding, it is required, and only a corpus with a large file could say so. ⛔ ASKED THROUGH filterChain, NOT BY LOOKING FOR THE KEY. /Filter may be present and Null, or an empty array, both of which mean no filtering; `_, ok := s.Dict["Filter"]` would call those filtered and skip the one check they are eligible for. ⛔⛔ AND THE FIRST VERSION OF THIS CHANGE WAS GREEN, MEASURED, AND UNPINNED AT EVERY EDGE — WHICH REVIEW CAUGHT AND MY OWN MUTATIONS DID NOT. I mutated the two things I had been thinking about (the filter condition, and the bound's existence) and both died, so I called the work done. Review mutated the bound's COMPARISON instead and found three survivors: `>`, `>= len-1` and `>= len+1` all left the whole suite GREEN. The cause is that the only values the suite exercised were /S 99 against 67 bytes and /S 43 against 64 — both far from the edge, so any comparison in the neighbourhood passes. A fourth survivor was the key lookup above: no fixture had `/Filter null` or `/Filter [ ]`, so the stated REASON for calling filterChain was the one thing nothing held. ⛔ A MUTATION ONLY MEASURES THE QUESTION IT ASKS. Mine asked "is this condition load-bearing?" and got a true answer. It could not ask "is this boundary the right one?", because I chose the mutations from what I had just been thinking about — which is exactly where I am least likely to be wrong. The lesson is not "mutate more", it is that the mutation set must come from the SHAPE of the code (a comparison has three neighbours; a documented reason needs a case) and not from the author's recollection of the risk. So four tests land with it, each pinning one survivor: /S == len-1 accepted kills `>= len-1` /S == len refused kills `>` and `>= len+1` /Filter null, /S past kills the key lookup /Filter [ ], /S past kills it the other spelling /Filter /FlateDecode kills dropping the condition, LOCALLY — it was only the 25-page sweep that held this before, which is a slow and indirect witness for one line ⛔ AND /S ABOVE THE END IS DELIBERATELY ABSENT, because it kills nothing the `== len` case does not already kill. An extra case that cannot fail reports its own index rather than a measurement. ⛔ hintFixtureData IS A CONSTANT AND THE CASES COUNT WITH IT. `/S 64` written beside a fixture that happens to hold 64 bytes stops testing the boundary the moment either number is edited alone, and the suite stays green — the shape this package has already paid for under "judge and subject must count alike". ⛔ AND GHOSTSCRIPT SPACE-PADS ITS INTEGERS, which the first version of the new test got wrong: `<>`. Anchoring on "/S 38" matched nothing and the helper's uniqueness check said so rather than letting a silent zero-replacement pass. Worth knowing beyond this test: that padding is the FIXED-WIDTH PLACEHOLDER technique a two-pass linearizing writer needs for /L, /H, /E and /T, so the corpus already holds a worked example of the fixpoint trick #40 describes. ⛔⛔ AND A CONTROL WRITTEN ONE CHANGE AGO CAUGHT THE FIXTURE, NOT THE CODE. TestEveryNonIntegerSharedTablePositionIsRefused's "an integer is accepted" subtest went red the moment this bound existed: its hand-built stream carried `/Length 0`, and /S 43 does point outside zero bytes. The bound was right and the fixture was unrealistic. Without that control the seven kind cases above it would have gone on passing for the WRONG REASON — refused by the length check rather than the kind check, which is the "refused for a reason that is not the reason" shape this file has already paid for twice. The fixture now carries 64 bytes, through a helper that says why. ⛔⛔ AND THE SECOND REVIEW FOUND A SIXTH SURVIVOR, WHOSE SUGGESTED TEST WOULD HAVE FAILED. Mutating the condition to `(ferr == nil && len(names) == 0) || ferr != nil` — applying the bound to a chain it could not read — was green: the same shape again, a documented reason with no case. But review's proposed fixture was `/Filter 9 0 R`, on the stated ground that an indirect filter makes filterChain error under a nil resolver. It does not. Resolve returns `Null{}, nil` for a Ref when the resolver is nil (object.go:162), so an indirect /Filter takes filterChain's `case Null:`, names stays empty, the stream is judged UNFILTERED, and the file is REFUSED — the opposite of the acceptance the suggested test asserts. Reading object.go rather than taking the mechanism on trust is the whole of the difference. ⛔ THE TEST FOR IT ASSERTS ITS OWN PREMISE. `/Filter 5` reaches filterChain's `default:` and is the spelling that actually makes it error, so the test calls filterChain itself and fails loudly if that ever stops erroring — the day it reads as a valid chain is the day the test stops exercising the branch it names while staying green. ⛔⛔⛔ AND THEN THE THIRD ROUND FOUND THE REAL DEFECT, WHICH WAS MINE AND WHICH I HAD WRITTEN A TEST TO BLESS. Having learned that an indirect /Filter reads as "no filter", I pinned THAT as intended behaviour and argued for it: the reader this format exists for cannot resolve anything, so treating a reference as "no filter it can use" is what it would do. Review supplied the input that kills the argument. If object 9 holds /FlateDecode, the stream really IS filtered, /S really does index decoded data longer than the raw bytes, and the refusal is for a reason the check cannot evaluate — which is WORD FOR WORD the argument I had just used, one test higher in the same file, to ACCEPT `/Filter 5`. Two cases that cannot be told apart, decided opposite ways, with a test holding each in place. ⛔⛔ A TEST THAT PINS A WRONG BEHAVIOUR IS WORSE THAN NO TEST, because it converts an oversight into a specification. The mutation set cannot find this class at all: every mutation of the old condition was correctly killed by TestAnIndirectFilterIsJudgedAsNoFilter, which is exactly the problem — the tests agreed with the code, and both were wrong. Only a reader asking "what does this REFUSE, and can the check know it?" gets there. So the predicate is now rawIsTheData, and it answers a THREE-way question as such: "filtered", "not filtered" and "cannot tell" are distinct, and only the second licenses the bound. An indirect /Filter is the third, so the bound is left alone and the file is accepted HERE. It is still non-conforming — F.3.6 requires every hint-dictionary entry beyond the stream Length to be direct — but that refusal is a NAMED property tabled on #40, and #40 should make it by name rather than this bound making it as a side effect, with a message about the shared object table that would send the next reader to the wrong place. ⛔ `/Filter [ 9 0 R ]` needs no case of its own: filterChain resolves the element to Null, ToName fails, and the error is the same middle answer. It is in the predicate's own table for the record, not as a separate claim. ⛔⛔ AND LOOKING FOR A SECOND INSTANCE OF THE CLASS FOUND ONE: /F. Rather than re-checking the case just fixed, the fourth round asked whether anything else refuses on something it cannot evaluate, and 7.3.8.2 Table 5 answers — when a stream dictionary carries /F its data lives in an EXTERNAL FILE, and the bytes between `stream` and `endstream` are not it. The parse slices them into Raw anyway, so len(Raw) measures a placeholder and bounding a decoded position against it refuses on a length belonging to different bytes. No /Filter need be involved at all, which is what makes it a case and not a variant. ⛔ The predicate is named rawIsTheData, and with /F present that name would be false. A predicate that is wrong in a case it has been told about is worth one line more than an issue describing it. It DECLINES TO BOUND rather than refusing: a hint stream with /F is wrong, and saying so is #40's job. ⛔ AND THE BARE PRESENCE TEST IS ONLY ADEQUATE FOR A STREAM DICTIONARY, which is written into the comment because it is a constraint on REUSE and not a property of the format. /F is a file specification in Table 5, but the same name is an unrelated flag field elsewhere — an annotation's /F is an integer of bit flags. Neither can reach a hint stream, so presence is enough here; a future caller passing a dictionary of another kind would need the key read for what it means there rather than merely counted. ⛔ AND THE PREDICATE IS TESTED AS A PREDICATE. Reaching it only through fixtures cannot distinguish "answered false" from "never asked", so TestRawIsTheDataAnswersOnlyWhenItKnows states all eight answers directly — absent, Null, empty array, a name, a chain, a reference, an array holding a reference, and an impossible kind. A refactor that keeps checkHintStream green while changing what the predicate MEANS now goes red. ⛔ AND A SECOND /F ROW WAS WRITTEN AND THEN DELETED, which is the same rule applied to myself. `{/F, /Filter null}` reddens, but it kills nothing the /F row alone does not — and /S above the end was excluded a few paragraphs up on exactly that ground. Keeping it because it looked thorough while excluding the other because the rule said so is the incoherence this commit was corrected for, one level up. Mutations, judged by cmd/failset on the failing NAMES, nine of nine killed. The boundary pair and the gate were re-run AFTER the refactor, because the condition moved and a mutation proved against the old shape says nothing about the new one: `>` instead of `>=` -> …MustLeaveRoomForItsOwnFirstByte/exactly_at_the_end `>= len-1` -> …MustLeaveRoomForItsOwnFirstByte/one_byte_inside `>= len+1` -> …MustLeaveRoomForItsOwnFirstByte/exactly_at_the_end the `s.Dict["Filter"]` key lookup -> …AFilterThatFiltersNothingIsStillBounded, both cases drop the condition -> …ARealFilterLeavesTheSharedTablePositionUnbounded AND the two 25_pages sweep cases bound an UNREADABLE chain as well -> …AFilterThatCannotBeReadLeavesTheBoundAlone, and no other drop rawIsTheData's Ref guard -> …AnIndirectFilterIsNotSomethingThisCheckCanRead and …AnswersOnlyWhenItKnows/an_indirect_/Filter, and no other drop the /F guard -> …AnswersOnlyWhenItKnows/an_external_file_stream, and no other invert the predicate (`len(names) != 0`) -> 16 tests, five of the predicate's own answers among them Note that the first two mutations redden DIFFERENT subtests, so the two cases hold opposite sides of the boundary rather than one of them carrying both. /S == len is refused on the claim that the shared object hint table opens with a Table F.5 header and so occupies at least one byte: a table starting AT the end of the data has nowhere to put it, which makes /S == len unrepresentable in a conforming file rather than merely unusual. ⛔ FOUR REVIEW ROUNDS, EACH FINDING SOMETHING THE PREVIOUS ONE COULD NOT. Round one: the boundary was unpinned. Round two: an unreadable chain was unpinned, and the fixture suggested for it would have failed. Round three: the behaviour I had just pinned was itself wrong. Round four, asked to look for a SECOND instance of that class rather than re-check the first: /F. The first two are test gaps a mutation finds; the third is a wrong SPECIFICATION, which no mutation can find because the tests and the code agreed; the fourth came from naming the class and hunting it. gofmt clean, go vet clean, 100.0% of statements, 0 uncovered blocks of 1820 under REQUIRE_QPDF=1 (CI's own command, replayed rather than approximated). Refs #40. Co-Authored-By: Claude Opus 5 --- linearized.go | 90 ++++++++++++- linearized_test.go | 328 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 392 insertions(+), 26 deletions(-) diff --git a/linearized.go b/linearized.go index 97f30c5..a34841b 100644 --- a/linearized.go +++ b/linearized.go @@ -544,12 +544,6 @@ func checkHintStream(b []byte, lin *Linearization) error { // stream", a symptom of seeking outside the data rather than a statement // about /S. // - // ⚠ THE UPPER BOUND IS STILL NOT CHECKED, and that is an open limit with - // a reason: /S indexes the DECODED data, so bounding it above means - // decoding an attacker's bytes in the cheap path — capped at 1 GiB by - // filter.go, which refuses a bomb but is three orders of magnitude more - // than any hint stream needs. That wants a size bound derived from /N and - // the object count, which wants F.4's layouts. #40 carries the ordering. const pageOffsetHeaderBytes = 36 if pos < pageOffsetHeaderBytes { return fmt.Errorf("%w: the primary hint stream's /S is %d, and the shared object table "+ @@ -557,6 +551,30 @@ func checkHintStream(b []byte, lin *Linearization) error { "is %d bytes (Table F.3, thirteen items of 288 bits)", ErrLinearizationStale, int64(pos), pageOffsetHeaderBytes, pageOffsetHeaderBytes) } + // ⭐ AND THE UPPER BOUND, FOR AN UNFILTERED STREAM, WHICH COSTS NOTHING. + // /S indexes the DECODED data, and decoding an attacker's bytes in the + // cheap path is what kept this open: filter.go caps expansion at 1 GiB, + // which refuses a bomb but is three orders of magnitude past anything a + // hint stream needs, so the bound wants a size limit derived from /N and + // the object count — F.4 work, ordered on #40. + // + // ⭐ But a stream with NO FILTER needs no decoding at all: its raw bytes + // ARE its data. That case is free, and it is the one that matters most + // while a writer is being developed, because an uncompressed hint stream + // is what somebody debugging one emits. Ghostscript writes one unfiltered + // already, and qpdf does under --compress-streams=n. + // + // ⛔ ASKED THROUGH rawIsTheData, WHICH ANSWERS ONLY WHEN IT KNOWS. The + // predicate is "the dictionary DIRECTLY says this stream is not filtered", + // and every other answer — including "I cannot tell" — leaves the bound + // alone. + if rawIsTheData(s.Dict) { + if int64(pos) >= int64(len(s.Raw)) { + return fmt.Errorf("%w: the primary hint stream's /S is %d and its data is %d bytes, "+ + "so the shared object table starts outside the stream", + ErrLinearizationStale, int64(pos), len(s.Raw)) + } + } // ⛔ AND THE LENGTH IS COMPARED WITH THE OBJECT THAT IS ACTUALLY THERE, // which review found it was not: the pair was bounds-checked against the // file and never against the stream it describes, so `/H [ 550 100 ]` for @@ -570,6 +588,66 @@ func checkHintStream(b []byte, lin *Linearization) error { return nil } +// rawIsTheData says whether a stream dictionary DIRECTLY states that its +// stream is not filtered, so that the raw bytes are the data and a position +// counted in decoded bytes can be bounded by len(Raw) without decoding +// anything. +// +// ⛔⛔ IT IS A THREE-WAY QUESTION ANSWERED AS TWO, AND THE MIDDLE ANSWER IS THE +// POINT. "Filtered", "not filtered" and "cannot tell" are distinct, and only +// the second licenses the bound. Review found the first version of this +// incoherent: it accepted `/Filter 5` on the ground that an unreadable chain +// is something this check cannot evaluate, while REFUSING `/Filter 9 0 R` — +// which it also cannot evaluate — because filterChain erases the difference. +// +// ⛔ Resolve returns `Null{}, nil` for a Ref when the resolver is nil +// (object.go), so filterChain reports an indirect /Filter as a chain of ZERO +// filters, indistinguishable from an absent one. If object 9 holds +// /FlateDecode then the stream really is filtered, /S really does index +// decoded data longer than the raw bytes, and bounding it refuses a +// conforming-in-this-respect position for a reason the check does not have. +// That is the exact failure the unreadable-chain case was written to avoid. +// +// ⛔ So an indirect /Filter is "cannot tell" and not "no filter". The file is +// still wrong — F.3.6 requires every hint-dictionary entry beyond the stream +// Length to be direct — but refusing it is a NAMED property tabled on #40, and +// #40 should refuse it as that, rather than this bound refusing it as a side +// effect with a message about the shared object table. +// +// ⛔ An array holding a reference, `/Filter [ 9 0 R ]`, already lands in +// "cannot tell" without a case of its own: filterChain resolves the element to +// Null, ToName fails, and the error is the same middle answer. +// ⛔⛔ AND /F MAKES THE RAW BYTES NOT THE DATA AT ALL, which is the same class +// again and was found by looking for a SECOND instance rather than re-checking +// the first. 7.3.8.2, Table 5: when a stream dictionary has /F its data lives +// in an external file, and the bytes between `stream` and `endstream` are not +// it — the parse still slices them into Raw, so len(Raw) measures the +// placeholder and nothing else. Bounding a decoded position against that +// refuses on a length belonging to different bytes. +// +// ⛔ No hint stream should carry /F, and this is not the check that says so — +// but the predicate is named rawIsTheData, and with /F present that name would +// be false. A predicate that is wrong in a case it was told about is worth one +// line more than an issue describing it. It DECLINES TO BOUND rather than +// refusing: a hint stream with /F is wrong, and saying so is #40's job. +// +// ⛔ AND THE BARE PRESENCE TEST IS ONLY ADEQUATE FOR A STREAM DICTIONARY. /F +// means a file specification in Table 5, but the same name is an unrelated +// flag field elsewhere in the format — an annotation's /F is an integer of bit +// flags. Neither can appear in a hint stream, so presence is enough HERE; +// reusing this predicate on dictionaries of another kind would need the key +// read for what it means there, not merely counted. +func rawIsTheData(d Dict) bool { + if _, external := d["F"]; external { + return false + } + if _, indirect := d["Filter"].(Ref); indirect { + return false + } + names, _, err := filterChain(d, nil) + return err == nil && len(names) == 0 +} + // spans says whether a declared length describes the object at off, which the // parse measured as n bytes. // diff --git a/linearized_test.go b/linearized_test.go index f148d55..8de1b31 100644 --- a/linearized_test.go +++ b/linearized_test.go @@ -938,6 +938,261 @@ func TestASharedTablePositionInsideThePageOffsetHeaderIsRefused(t *testing.T) { }) } +// editedGS is editedFixture for the Ghostscript file. No padding arithmetic: +// every edit here is same-width by construction, which the helper asserts, so +// nothing in the file moves. +func editedGS(t *testing.T, from, to string) []byte { + t.Helper() + if len(from) != len(to) { + t.Fatalf("%q and %q are not the same width, so this edit would move the file", from, to) + } + b, err := os.ReadFile(filepath.Join("testdata", "linearized-gs.pdf")) + if err != nil { + t.Fatal(err) + } + s := string(b) + if n := strings.Count(s, from); n != 1 { + t.Fatalf("%q appears %d times, so this edit is not the one it is named for", from, n) + } + return []byte(strings.Replace(s, from, to, 1)) +} + +// ⭐ THE UPPER BOUND ON /S, WHERE IT COSTS NOTHING. /S indexes the decoded +// data, so bounding it above normally means decoding an attacker's bytes — the +// reason it stayed open. But a stream with NO FILTER needs no decoding: its +// raw bytes are its data. +// +// ⭐ GHOSTSCRIPT WRITES ONE UNFILTERED, which is why this is testable at all: +// `<< /Length 67 /S 38 >>`, no /Filter, so the data is 67 bytes and /S 38 sits +// inside it. qpdf's two fixtures are FlateDecode and are skipped by this +// check, which is exactly the asymmetry being tested. +// +// ⛔ And it is the case that matters most while a writer is being developed, +// since an uncompressed hint stream is what somebody debugging one emits. +func TestASharedTablePositionOutsideAnUnfilteredStreamIsRefused(t *testing.T) { + // 99 >= 67, so the table would start past the end of the stream's data. + // + // ⛔ The anchor is `38>>` and not `/S 38`, because Ghostscript writes its + // integers space-padded — `<>` — which + // is the fixed-width placeholder technique a two-pass linearizing writer + // needs for /L, /H, /E and /T. Guessing the spacing is how the first + // version of this test failed, and the helper's uniqueness check is what + // said so. + b := editedGS(t, "38>>", "99>>") + if _, err := Linearized(b); err != nil { + t.Fatalf("the cheap check refused it, so this is not about /S: %v", err) + } + _, err := CheckLinearized(b) + if !errors.Is(err, ErrLinearizationStale) { + t.Fatalf("error is %v, want ErrLinearizationStale", err) + } + if !strings.Contains(err.Error(), "starts outside the stream") { + t.Errorf("it was refused, but not for /S being past the data: %v", err) + } +} + +// ⛔ AND THE CONTROL THAT KEEPS THE ABOVE HONEST: the same fixture untouched +// is accepted, so the refusal is about the edit and not about this checker +// disliking Ghostscript's output — which it does not, and #43 depends on it +// not doing so. +func TestTheUnfilteredFixtureItselfIsAccepted(t *testing.T) { + b, err := os.ReadFile(filepath.Join("testdata", "linearized-gs.pdf")) + if err != nil { + t.Fatal(err) + } + if _, err := CheckLinearized(b); err != nil { + t.Fatalf("the Ghostscript fixture was refused: %v", err) + } +} + +// ⛔⛔ AND THE BOUNDARY ITSELF, WHICH THE TEST ABOVE DOES NOT HOLD. Review +// measured three mutations of `pos >= len(s.Raw)` that left the whole suite +// GREEN: `>`, `>= len-1` and `>= len+1`. The reason is that the only values +// the suite exercised were 99 against 67 bytes and 43 against 64 — both far +// from the edge, so any comparison in the neighbourhood passes. A bound that +// nothing holds at its edge is the recurring shape in this file: #43's +// `last >= len(b)` survived as `>` for exactly the same reason. +// +// ⛔ TWO CASES KILL ALL THREE, and a third would kill nothing. /S one byte +// inside must be ACCEPTED (which refutes `>= len-1`); /S exactly at the end +// must be REFUSED (which refutes both `>` and `>= len+1`). Adding /S above the +// end would only repeat the second, so it is deliberately absent — an extra +// case that cannot fail is a measurement that reports its own index. +// +// ⛔ The claim being pinned is that the table needs room for its own first +// byte: the shared object hint table opens with a Table F.5 header, so a table +// starting AT the end of the data has nowhere to put it. /S == len is +// unrepresentable in a conforming file, not merely unusual. +func TestTheSharedTableMustLeaveRoomForItsOwnFirstByte(t *testing.T) { + for _, c := range []struct { + name string + s int + refuse bool + }{ + {"one byte inside the data", hintFixtureData - 1, false}, + {"exactly at the end of the data", hintFixtureData, true}, + } { + t.Run(c.name, func(t *testing.T) { + b, lin := hintFixture(t, strconv.Itoa(c.s)) + err := checkHintStream(b, lin) + if !c.refuse { + if err != nil { + t.Fatalf("/S %d is inside %d bytes and was refused: %v", c.s, hintFixtureData, err) + } + return + } + if !errors.Is(err, ErrLinearizationStale) { + t.Fatalf("/S %d against %d bytes was accepted (or refused oddly): %v", + c.s, hintFixtureData, err) + } + if !strings.Contains(err.Error(), "starts outside the stream") { + t.Errorf("it was refused, but not for /S being past the data: %v", err) + } + }) + } +} + +// ⛔⛔ AND A /Filter THAT FILTERS NOTHING IS STILL UNFILTERED, which was the +// stated reason for asking filterChain rather than looking the key up — and +// review measured that replacing the call with `_, ok := s.Dict["Filter"]` +// left the suite GREEN, because no fixture had either spelling. The reason +// written in the comment was the one thing no test held. +// +// ⛔ Both spellings mean no filtering per 7.3.9 and §7.3.8.2: a Null /Filter +// is indistinguishable from an absent one, and an empty array is a chain of no +// filters. filterChain returns len(names) == 0 for each, so the data IS the +// raw bytes and /S is bounded by them. A key lookup would call these filtered +// and skip the one check they are eligible for. +func TestAFilterThatFiltersNothingIsStillBounded(t *testing.T) { + for _, c := range []struct{ name, extra string }{ + {"a null /Filter", " /Filter null"}, + {"an empty /Filter array", " /Filter [ ]"}, + } { + t.Run(c.name, func(t *testing.T) { + b, lin := hintFixtureWith(t, strconv.Itoa(hintFixtureData*2), c.extra) + err := checkHintStream(b, lin) + if !errors.Is(err, ErrLinearizationStale) { + t.Fatalf("%s was treated as filtered, so /S went unbounded: %v", c.name, err) + } + if !strings.Contains(err.Error(), "starts outside the stream") { + t.Errorf("it was refused, but not for /S being past the data: %v", err) + } + }) + } +} + +// ⛔ AND THE OTHER SIDE OF THE ASYMMETRY, HERE RATHER THAN ONLY AT 25 PAGES. A +// real filter means /S indexes DECODED data, which this check does not have, +// so the bound must not apply. That condition was load-bearing and measured — +// dropping it reddens the generated sweep's twenty-five page cases, where the +// hint table compresses below /S — but a sweep is a slow and indirect witness +// for a one-line condition. This states it directly: the same /S that the +// cases above refuse is ACCEPTED once a filter is named, and nothing here +// decodes anything. +func TestARealFilterLeavesTheSharedTablePositionUnbounded(t *testing.T) { + b, lin := hintFixtureWith(t, strconv.Itoa(hintFixtureData*2), " /Filter /FlateDecode") + if err := checkHintStream(b, lin); err != nil { + t.Fatalf("/S past the RAW length was refused for a filtered stream, "+ + "where it indexes the decoded data: %v", err) + } +} + +// ⛔⛔ AND A /Filter THAT CANNOT BE READ AT ALL SKIPS THE BOUND, which is the +// last thing the comment claimed and nothing held: review mutated the +// condition to `(ferr == nil && len(names) == 0) || ferr != nil` — applying +// the bound to a chain it could not read — and the suite stayed GREEN. Same +// shape as the key lookup above: a documented reason with no case. +// +// ⛔ `/Filter 5` IS THE SPELLING THAT REACHES IT, and finding that out +// corrected the review that asked for this test. Review proposed `/Filter 9 0 +// R` on the stated ground that an indirect filter makes filterChain error +// under a nil resolver. It does not: Resolve returns `Null{}, nil` for a Ref +// when the resolver is nil (object.go), so filterChain reports an indirect +// /Filter as a chain of ZERO filters. That is why rawIsTheData checks for a +// Ref itself before asking — see the test below. The error branch here is +// reached only by a /Filter whose kind can never name a filter, which is +// filterChain's `default:`. +// +// ⛔ AND THE BEHAVIOUR IS ACCEPTANCE, DELIBERATELY. An unreadable chain means +// this check cannot know whether the raw bytes are the data, and refusing on +// a bound it cannot evaluate would refuse for a reason it does not have. The +// file is still non-conforming — F.3.6 requires direct entries, which is one +// of the properties tabled on #40 — and it is #40 that should refuse it, by +// name, rather than this bound refusing it as a side effect. +func TestAFilterThatCannotBeReadLeavesTheBoundAlone(t *testing.T) { + b, lin := hintFixtureWith(t, strconv.Itoa(hintFixtureData*2), " /Filter 5") + if names, _, ferr := filterChain(Dict{"Filter": Integer(5)}, nil); ferr == nil { + t.Fatalf("the premise is gone: /Filter 5 now reads as a chain of %d filter(s), "+ + "so this test no longer exercises the unreadable-chain branch", len(names)) + } + if err := checkHintStream(b, lin); err != nil { + t.Fatalf("/S was bounded against a filter chain that cannot be read: %v", err) + } +} + +// ⛔⛔ AND AN INDIRECT /Filter IS "CANNOT TELL", NOT "NO FILTER" — WHICH IS THE +// INCOHERENCE REVIEW FOUND IN THE FIRST VERSION OF THIS CHANGE. filterChain +// erases the difference: Resolve returns `Null{}, nil` for a Ref under a nil +// resolver (object.go), so `/Filter 9 0 R` reported a chain of ZERO filters, +// indistinguishable from an absent /Filter. The bound applied and the file was +// refused. +// +// ⛔⛔ AND I PINNED THAT AS INTENDED BEHAVIOUR, which is worse than leaving it +// unexamined: a test blessing it. Review supplied the input that breaks it — +// if object 9 holds /FlateDecode then the stream really IS filtered, /S really +// does index decoded data longer than the raw bytes, and the refusal is for a +// reason the check cannot evaluate. That is precisely the argument used one +// test above to ACCEPT an unreadable /Filter 5, so the two cases contradicted +// each other and I had written the contradiction down as a feature. +// +// ⛔ The predicate is now rawIsTheData, which answers only when the dictionary +// DIRECTLY says there is no filtering. The file is still non-conforming — +// F.3.6 requires direct entries — but that refusal is a NAMED property tabled +// on #40 and belongs there, not here with a message about the shared object +// table. +func TestAnIndirectFilterIsNotSomethingThisCheckCanRead(t *testing.T) { + b, lin := hintFixtureWith(t, strconv.Itoa(hintFixtureData*2), " /Filter 9 0 R") + if err := checkHintStream(b, lin); err != nil { + t.Fatalf("/S was bounded against a filter this check cannot resolve, "+ + "so a genuinely filtered stream is refused for a reason it cannot evaluate: %v", err) + } +} + +// ⛔ AND THE PREDICATE ITSELF, ASKED DIRECTLY FOR ITS THREE ANSWERS. The two +// "cannot tell" cases above reach rawIsTheData through a fixture, which cannot +// distinguish "answered false" from "never asked"; this states each answer as +// the predicate's own, so a refactor that keeps checkHintStream passing while +// changing what the predicate MEANS still goes red. +func TestRawIsTheDataAnswersOnlyWhenItKnows(t *testing.T) { + for _, c := range []struct { + name string + d Dict + want bool + }{ + {"no /Filter at all", Dict{}, true}, + {"a null /Filter", Dict{"Filter": Null{}}, true}, + {"an empty /Filter array", Dict{"Filter": Array{}}, true}, + {"a real filter", Dict{"Filter": Name("FlateDecode")}, false}, + {"a chain of filters", Dict{"Filter": Array{Name("ASCIIHexDecode"), Name("FlateDecode")}}, false}, + {"an indirect /Filter", Dict{"Filter": Ref{Num: 9}}, false}, + {"an array holding a reference", Dict{"Filter": Array{Ref{Num: 9}}}, false}, + {"a /Filter of an impossible kind", Dict{"Filter": Integer(5)}, false}, + // ⛔ /F puts the data in an external file (7.3.8.2, Table 5), so the + // bytes between `stream` and `endstream` are a placeholder and + // len(Raw) measures nothing the position is counted in — even with no + // /Filter at all, which is what makes this a case of its own rather + // than a variant of the ones above. + {"an external file stream", Dict{"F": String("hints.dat")}, false}, + } { + t.Run(c.name, func(t *testing.T) { + if got := rawIsTheData(c.d); got != c.want { + t.Errorf("rawIsTheData = %v, want %v — the bound would be %s", + got, c.want, map[bool]string{true: "applied", false: "skipped"}[got]) + } + }) + } +} + // ⛔⛔ AND EVERY OTHER WRONG KIND, WHICH THE FILE-LEVEL TEST CANNOT REACH. // Review measured the gap: narrowing the check to "refuse only a Name" left // the suite GREEN, because `/S /x` is the single case a same-width edit of @@ -962,19 +1217,8 @@ func TestEveryNonIntegerSharedTablePositionIsRefused(t *testing.T) { {"null", "null"}, } { t.Run(c.name, func(t *testing.T) { - const head = "%PDF-1.7\n" - b := []byte(head + "1 0 obj\n<< /S " + c.s + " /Length 0 >>\nstream\n\nendstream\nendobj\n") - _, _, n, err := ParseIndirectObject(b[len(head):], nil) - if err != nil { - t.Fatalf("the fixture itself does not parse: %v", err) - } - lin := &Linearization{ - HintOffset: len(head), - // From the parse, so the length bound and spans both hold and - // cannot be what refuses this. - HintLength: n + eolAfter(b, len(head)+n), - } - err = checkHintStream(b, lin) + b, lin := hintFixture(t, c.s) + err := checkHintStream(b, lin) if !errors.Is(err, ErrLinearizationStale) { t.Fatalf("/S as %s was accepted (or refused oddly): %v", c.name, err) } @@ -987,20 +1231,64 @@ func TestEveryNonIntegerSharedTablePositionIsRefused(t *testing.T) { // ⛔ THE CONTROL: the same fixture with an integer /S is ACCEPTED, so the // cases above fail for their kind and not because this hand-built stream // is unacceptable for some other reason. + // + // ⭐ AND IT EARNED ITS KEEP IMMEDIATELY. The first version of this fixture + // carried `/Length 0` and no data, so when the unfiltered upper bound on + // /S arrived, this control went red — /S 43 does point outside zero bytes. + // The check was right and the fixture was not; without the control the + // seven cases above would have gone on passing for the wrong reason. t.Run("an integer is accepted", func(t *testing.T) { - const head = "%PDF-1.7\n" - b := []byte(head + "1 0 obj\n<< /S 43 /Length 0 >>\nstream\n\nendstream\nendobj\n") - _, _, n, err := ParseIndirectObject(b[len(head):], nil) - if err != nil { - t.Fatalf("the fixture itself does not parse: %v", err) - } - lin := &Linearization{HintOffset: len(head), HintLength: n + eolAfter(b, len(head)+n)} + b, lin := hintFixture(t, "43") if err := checkHintStream(b, lin); err != nil { t.Fatalf("an integer /S was refused, so these cases prove nothing: %v", err) } }) } +// hintFixtureData is the fixture's data length in bytes. +// +// ⛔ IT IS A CONSTANT BECAUSE THE BOUNDARY TESTS COUNT WITH IT. A test that +// wrote `/S 64` beside a fixture that happened to hold 64 bytes would stop +// testing the boundary the moment either number was edited alone, and the +// suite would stay green — the shape this package has already paid for under +// "judge and subject must count alike". The cases below spell /S as +// hintFixtureData-1 and hintFixtureData, so they cannot drift apart. +const hintFixtureData = 64 + +// hintFixture builds a minimal unfiltered hint stream whose /S is the given +// spelling, with /H's extent taken FROM THE PARSE so that nothing earlier in +// checkHintStream can be what refuses it. +// +// ⛔ The data is hintFixtureData bytes because /S 43 has to be INSIDE it. An +// empty stream made the control above fail the moment the upper bound existed, +// which is the fixture being unrealistic rather than the bound being wrong: a +// shared object table cannot start at byte 43 of nothing. +func hintFixture(t *testing.T, s string) ([]byte, *Linearization) { + t.Helper() + return hintFixtureWith(t, s, "") +} + +// hintFixtureWith is hintFixture with extra dictionary entries spliced in, +// which is how the /Filter cases say what they are about: the upper bound on +// /S is asked through filterChain, and only a dictionary can state the +// difference between "no /Filter", "/Filter null" and "/Filter [ ]". +func hintFixtureWith(t *testing.T, s, extra string) ([]byte, *Linearization) { + t.Helper() + const head = "%PDF-1.7\n" + data := strings.Repeat("h", hintFixtureData) + b := []byte(head + "1 0 obj\n<< /S " + s + extra + + " /Length " + strconv.Itoa(hintFixtureData) + " >>\nstream\n" + + data + "\nendstream\nendobj\n") + _, _, n, err := ParseIndirectObject(b[len(head):], nil) + if err != nil { + t.Fatalf("the fixture itself does not parse: %v", err) + } + return b, &Linearization{ + HintOffset: len(head), + HintLength: n + eolAfter(b, len(head)+n), + } +} + // ⛔⛔ AND THE FOREIGN JUDGE REFUSES IT TOO, which is the half the negative // control above was missing. go-pdfkit/reader#40 asks for exactly this before // a linearizing writer exists: two judges that have only ever agreed prove