Repository navigation
linearize: check the parameters against the document, not only against themselves - #41
Merged
Merged
Conversation
…t themselves
⛔⛔ Linearized() IS A WEAKER TEST THAN IT LOOKS, AND A WRITER DEVELOPED
AGAINST IT WOULD PASS WITHOUT LINEARIZING ANYTHING. Of Table F.1's claims it
checks exactly one against the file — /L against the byte count. /O, /E, /N
and /T are read for type and never consulted. So a file carrying a correct /L
and four invented integers is ACCEPTED, which is precisely what a first
linearizer emits.
That is not a defect in Linearized(): it is deliberately cheap, because the
reader it serves has fetched 1024 bytes and has no cross-reference table to
resolve anything with. It is the wrong judge for a writer, and reader#40 opens
the writer, so the judge comes first and separately — developed against a
check that was not written to let it through.
CheckLinearized(b) answers the other question: not "may a reader trust the
front of this file" but "is this file actually what the front says". It opens
the document and establishes that every parameter POINTS AT WHAT IT CLAIMS TO:
/N the real page count
/O the real first page, and an object whose /Type is /Page — a number that
happens to be right while the object is something else is a file whose
page tree was rebuilt under it
/H an indirect object that is a STREAM, at the offset Table F.1 says is
"the beginning of the stream object, not the beginning of the stream
data"
/T the main cross-reference — ⛔ IN EITHER SHAPE. Table F.1 gives it two
meanings and both are real: the white space before the object-zero entry
of a traditional table, or a cross-reference STREAM object. Accepting
only one would call every file of the other kind stale.
/E inside the file. Nothing here knows which objects the first page owns —
that is the hint tables' job — so this is the one claim that can be
checked without them.
⛔ THE FOUR GATES ARE FILES Linearized() ACCEPTS AND THIS REFUSES. Each is the
committed qpdf fixture with ONE integer replaced by one of THE SAME WIDTH, so
/L still equals the byte count and nothing at the front of the file is wrong —
what is wrong is that it describes a different document. The helper ASSERTS
the width: an edit one byte longer would be refused for the length instead,
the deep check would never run, and the test would pass measuring nothing.
⛔ And the negative control: an ordinary file written by this package is
refused by BOTH checks. A suite in which every input passes measures nothing.
Each of the four checks was mutated to always agree, and the verdict was taken
from cmd/failset rather than from a pass/fail count — every mutation red on
its own subtest and no other:
checkFirstPage -> …/a_first_page_that_is_not_the_first_page
the /N comparison -> …/a_page_count_the_document_contradicts
checkHintStream -> …/a_hint_stream_that_starts_one_byte_late
checkMainXref -> …/a_main_cross-reference_that_is_not_there
Still not checked, by either: the hint tables (F.4). qpdf --check remains the
foreign judge, and CheckLinearized agrees with qpdf's output at 1, 2 and 5
pages.
Refs #40.
gofmt clean, go vet clean, 99.4% of statements.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI was red on the exact-100% coverage gate: 14 uncovered blocks, every one of them an error path I had added. The gate was right to refuse, and two of the 14 were not missing tests — they were branches that could not be reached from any file at all. ⛔⛔ THE /O CHECK WAS ASKING A STRICTER QUESTION THAN THE READER DOES, AND THAT WAS THE DEFECT THE GATE FOUND. It resolved the object /O names and refused the file unless its /Type was /Page. But Document.PageRef is what the document layer CALLS the first page, and it does not require /Type: walkPages appends any resolvable dictionary with no /Kids. So the checker had a second, stricter definition of "first page" than the reader that will actually open the file — a judge and a subject that do not count alike — and a file the reader is perfectly happy with would have been reported as stale. What /O must equal is what a reader will treat as page one, and nothing else. The Get-error and not-a-Dict branches went with it; no file could reach them, which is what dead code dressed as thoroughness looks like. ⛔ /N < 1 IS NOW ITS OWN ANSWER, AND IT HAS TO COME FIRST. Table F.1 calls /N "the number of pages in the document"; a linearized document with none is not a document this format describes. Checked before the comparison, because a /N of 0 reported as "the document has 2" sends the reader to a page tree that is not the problem. It also makes the "no first page" branch unreachable — the count is lin.PageCount and that is now at least 1 — so that branch is gone rather than tested. ⛔⛔ AND TABLE F.1 GIVES /T TWO MEANINGS WHILE THE FIXTURE ONLY HAD ONE. Every test ran against testdata/linearized.pdf, a traditional cross-reference table, so the cross-reference STREAM arm had NEVER RUN. Until the gate said so, "both shapes are handled" was a comment and nothing more. testdata/linearized-objstm.pdf is the same document through `qpdf --linearize --object-streams=generate` — still an independent implementation's output, and its /T is an /XRef stream. Seven more same-width edits reach the bounds checks: /E at 0, /H at offset 0, /H running past the end, /H pointing at a dictionary rather than a stream, /T at 0, and /T one byte from the end — that last one so the free-entry test has to notice the buffer is shorter than an entry rather than read past it. And one case that is not an integer edit: a front that still says "linearized" over a body that is gone. The first 1024 bytes are untouched and /L still equals the length, so the cheap check passes; everything after the parameter dictionary is blanked, so there is no cross-reference table to open with. That is a truncated download padded back to length. ⛔ MEASURED WITHOUT QPDF, WHICH IS CI'S CONDITION. `go test -c -cover` and the binary run under a PATH with no qpdf: 0 uncovered blocks, so the committed fixtures carry the gate and the skipping tests are not propping it up. Said out loud because reader's CI installs no qpdf at all — see #40. ⛔ covblocks, not `go tool cover -func`: the latter rounded this to "100.0%" while one block was still uncovered. checkFirstPage disabled as a mutation turns exactly its three subtests red — /N too small, /N contradicted, /O wrong — judged by cmd/failset on the failing NAMES rather than on a count. gofmt clean, go vet clean, 0 uncovered blocks of 1743. Refs #40. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… now
⛔⛔ A SECURITY AUDIT OF MY OWN NEW CODE, AND IT FOUND ONE. checkHintStream
bounded the hint stream with
lin.HintOffset+lin.HintLength > len(b)
Both operands come straight out of the first object of the file, so an
attacker chooses them. On a 32-bit build, int is 32 bits: a /H length near
MaxInt32 makes that sum OVERFLOW to a negative number, the comparison is
false, and the check admits exactly the file it exists to refuse. It is now a
subtraction — the offset is already known to be inside the file, so
len(b)-lin.HintOffset is positive and cannot wrap.
Nothing could read past the end as a result (the slice is taken at the offset,
which is bounds-checked separately), so this was a wrong VERDICT rather than a
wrong ACCESS. It is still the defect class CLAUDE.md names, in code I wrote an
hour ago, and it was invisible on this 64-bit workstation.
⛔ AND THE CHECK IS FUZZED NOW, because it is a parser of numbers an attacker
chose: /H, /O, /E and /T are used as offsets and lengths into the file, which
is where a bad bound stops being a wrong answer and becomes a read past the
end. The cheap Linearized() only type-checks them; this one slices at them.
FuzzCheckLinearized seeds with the two committed fixtures — byte substitutions
in those keep the LENGTH, so /L still matches and the mutator reaches the deep
checks instead of stopping at the front — plus three fronts whose integers are
absurd on purpose, including MaxInt32 in both halves of /H, which is the
regression seed for the overflow above.
⛔ It asserts an INVARIANT rather than merely "it did not panic": the deep
check is strictly stronger than the cheap one, so anything it accepts the
cheap one must accept too. A disagreement would mean the two read the front of
a file differently — which is how a reader and a writer come to disagree about
the same bytes. It also asserts that every offset accepted is inside the file
accepted, which is the property the overflow broke.
Measured: 2 579 861 executions, 217 interesting inputs, no crasher and no
invariant violation.
gofmt clean, go vet clean, 0 uncovered blocks of 1743.
Refs #40.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ed about
⛔⛔ AN INDEPENDENT REVIEW BUILT FILES AND ASKED BOTH JUDGES. Five of them were
accepted here and refused by qpdf 12.4.2 — the judge this whole issue says the
writer must be built against. A checker that is weaker than its own foreign
judge is a checker a writer can satisfy while producing something qpdf calls
not linearized.
- /Linearized 9 WAS ACCEPTED. The version was read for its TYPE and its
value never looked at. It is the one entry that says which format the rest
of the dictionary is written in, so a version this has never seen makes
every other entry a guess. Refused in the CHEAP check, as MALFORMED: a
reader deciding whether to stream a file consults exactly this. 1 and 1.0
are both accepted, because the specification's Example 2 writes one and
qpdf writes the other.
- /H [ 550 100 ] FOR A 127-BYTE OBJECT WAS ACCEPTED. The pair was bounded
against the FILE and never compared with the stream it describes. It is
compared with the parse's own extent now.
⛔ NOT WITH EQUALITY, AND MEASUREMENT IS WHY: requiring n == declared
refused every file qpdf produces, because the parse ends at `endobj` while
Table F.1's length counts the terminating end-of-line. Measured on qpdf
output at 1, 2 and 5 pages and on both committed fixtures: declared was
n+1 in every case. Two bytes are allowed for CRLF. The rule is asymmetric
on purpose — a declared length SHORTER than the object is always wrong.
- THE OVERFLOW PAIR OF /H WAS READ AND NEVER EXAMINED. A file carrying
`/H [ 550 127 9 9 ]` was accepted while qpdf said the overflow hint table
is not a stream. An entry present and unchecked is worse than one absent.
- /O NAMING THE HINT STREAM WAS ACCEPTED, because walkPages treats any
resolvable object with no /Kids as a page leaf. ⛔ This is NOT the /Type
check deleted in the previous commit returning: requiring /Type /Page
would impose a stricter notion of a page than the reader that opens the
file; requiring a DICTIONARY imposes nothing, since 7.7.3.3 says a page
object is one. The page walker's leniency is its own question.
⛔ THE 32-BIT BOUND WAS CORRECT AND UNTESTED, which review proved by restoring
the addition form and watching the whole suite stay green. No test carried a
length anywhere near the limit. Two do now, at MaxInt64 and MaxUint32, and
they fail on every word size with the addition form.
⛔ AND THE FUZZ SEEDS NEVER REACHED THE CODE THEY WERE WRITTEN FOR: every one
said `/L 1` for a file hundreds of bytes long, so Linearized refused them on
the length and checkHintStream never ran. The placeholder is ten digits now,
so filling it in cannot change the length it measures. 2 579 861 executions
before, 12 733 408 after, 345 interesting inputs, no crasher.
⛔ A TAUTOLOGY WAS REMOVED FROM THE FUZZ INVARIANTS: "anything CheckLinearized
accepts, Linearized accepts" cannot fail, because the first calls the second.
The bound assertions stay, with the reason they are NOT restatements — they
re-derive the property from len(b) after the function has committed, which is
what catches an arithmetic fault rather than a missing branch.
⛔ THE DIRECTION THAT MATTERS MOST — not refusing what qpdf accepts — is now
judged across SHAPES as well as page counts: as qpdf writes it, with
--object-streams=generate, and with --compress-streams=n, at 1, 2, 5 and 25
pages. Twelve comparisons, all agreeing.
One branch went the other way: `d.Get` after PageRef cannot fail, since
walkPages resolved that reference to produce it, and an object that did not
read is nil — which the dictionary check already reports. Dead code dressed as
thoroughness, and the coverage gate is what said so.
gofmt clean, go vet clean, 0 uncovered blocks of 1766.
Refs #40.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… claim I published ⛔⛔ THE PREVIOUS COMMIT MESSAGE SAID SOMETHING FALSE, and review measured it. It claimed the two huge-/H tests "fail on every word size with the addition form". With the addition form restored the suite stayed GREEN: `spans` refused those files a few lines later for a different reason, so the tests never reached the bound they are named for. I asserted that from reading the code instead of running the mutation — with cmd/failset, the tool built this session FOR exactly that, already used twice that hour. Third time in one session that a published claim about a mutation was wrong, and the first two were caught by reviewers too. The test names the BOUND'S OWN SENTENCE now, so only the length check can satisfy it. Re-measured: with the addition form, MaxInt64 turns it red and MaxUint32 does NOT — the latter overflows only where int is 32 bits, which is the build the bound exists for and is not the one the tests run on. That is in the test, because a reader would otherwise assume both cases catch it. ⛔⛔ AND `spans` WAS A GUESS WHERE IT COULD BE A READING. It allowed the declared length to exceed the parsed extent by 0..2 bytes, on the reasoning that a writer might use CRLF. Review measured what that costs: /H [ 550 126 ] and /H [ 550 128 ] for a 126-byte object are both ACCEPTED here and both refused by qpdf — "first page object offset mismatch" and "hint table length mismatch". A tolerance wide enough to cover every line ending is wide enough to hide a wrong length, and this file exists to be no weaker than the judge it will be measured against. The end-of-line is READ now (eolAfter), and the declared length must be exactly the object plus it. Both neighbours of the right answer are tests. ⛔ Every spelling of a line ending has its own case — LF, lone CR, CRLF, none at all, past the end of the buffer — because qpdf writes LF and the fixtures would otherwise exercise one branch behind a suite that looks thorough, which is how the window got there to begin with. ⛔ /E IS NOT GATED, AND THAT IS NOW SAID OUT LOUD in the doc comment and the README rather than left to be assumed. qpdf COMPUTES it (on the fixture: "computed = 901..902") and refuses a wrong value; this only bounds it between 1 and the file length, so a writer that miscomputes /E passes here and fails qpdf. Computing it needs the first-page object set — the same partition the hint tables describe — so it waits for that work rather than being approximated. A pass here is not evidence about /E. The twelve qpdf comparisons (1/2/5/25 pages x plain, object-streams, uncompressed-streams) still agree under the exact rule, which is the stronger statement: declared == object + its own end-of-line holds for every file qpdf writes, where the window was only approximating it. gofmt clean, go vet clean, 0 uncovered blocks of 1775. Refs #40. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ating it ⛔⛔ CI'S 386 AND arm LANES WENT RED, AND THE TEST WAS THE SMALL HALF OF IT. Both 9223372036854775807 and 4294967295 arrive as -1 on a 32-bit build, because `int(Integer)` discards the high half in silence. The assertion written for a 64-bit machine failed on exactly the two lanes that exist to catch this. ⛔ THE REAL HAZARD IS NOT THE VALUE THAT BECOMES -1. A /H length of 2^32+127 truncates to 127 — the fixture's own hint length, a perfectly plausible value that every later bound then accepts. The same bytes would be judged SOUND on a 32-bit build and refused on a 64-bit one, and nothing would say so. The wild values were caught only because they happened to land where a bound objected. Review named this hazard; CI reached it first by another route. Every integer the dictionary offers now goes through `addressable`, which refuses what this build cannot represent rather than letting it wrap — /H included, since both halves are used as a byte range. ⛔ intMax IS A VARIABLE SO THE BRANCH HAS A TEST HERE. On a 64-bit build the refusal is unreachable from any file, so it would be a block no test could enter — reported by the coverage gate and otherwise exercised only in CI, i.e. after pushing. Shrinking it to a 32-bit machine's largest int reproduces that machine's arithmetic on this one, and the 2^32+127 case is the test. ⛔ AND THE WORD SIZE DECIDES WHICH REFUSAL IS CORRECT, which the tests now say instead of assuming. On 32 bits the conversion refuses before any bound is consulted, so asserting that the cheap check ACCEPTS the file — the right precondition on 64 bits — is what turned those lanes red. Both paths are asserted, each where it applies. `GOARCH=386 go vet` is clean; the 32-bit behaviour itself is CI's to confirm, and this workstation cannot run it. gofmt clean, go vet clean, 0 uncovered blocks of 1780. Refs #40. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third review. CI is green at 20c31e2 on all ten lanes, so nothing here is a fix for a red build — all four findings are tests that could not have failed, which is the harder kind to notice and the reason the review was asked for. ⛔⛔ addressable WAS REACHED ONLY THROUGH /H. Review measured `int(int32(n))` in directInteger — the conversion for /L, /O, /E, /N and /T — and the suite stayed GREEN. The seam test shrinks intMax and then edits /H, so every other entry's refusal was written and never exercised. /H is where a hostile value is most likely to arrive, which is why it was written first; that is not a reason for it to be the only one a test can see. ⛔ THE LOWER HALF OF THAT REFUSAL HAD NEVER BEEN REACHED BY ANY FILE. `v < -intMax-1` was written from symmetry with the upper bound, not from a case, and deleting it left the suite green. ⛔⛔ THE OVERFLOW PAIR'S LENGTH BOUND WAS HELD BY NOTHING — the round-2 vacuity, surviving in the half the fix was never copied into. Rewriting it as `off+len > len(b)` left the suite green, because the file is still refused one step later by spans, for a different reason. A bound subsumed by a later check is a bound no test is holding, and a test that asserts only "an error" cannot tell the two refusals apart. The new test asserts the SENTENCE. ⛔ AND /P WAS THE ONE ENTRY STILL CONVERTING DIRECTLY. It is never used as an offset, which is why it was missed and is not a reason to leave it: it is a public field, so a 32-bit build would hand a caller 0 for 2^32 from the same bytes a 64-bit build reads correctly, with nothing to say which it got. Mutations re-run for this commit, judged by cmd/failset on the failing test NAMES rather than on a count, each red on its own test and no other: ovf bound -> addition form -> TestAnOverflowHintLengthNearTheIntegerLimit… directInteger int(int32(n)) -> TestAnEntryThatWrapsOnA32BitBuildIsRefused…, TestAValueBelowTheSmallestIntThisBuildHas… drop `v < -intMax-1` -> TestAValueBelowTheSmallestIntThisBuildHas… /P back to int(n) -> TestAFirstPageNumberThatWrapsOnA32Bit… Review also confirmed the seam is safe: t.Cleanup is registered before the shrink, so the restore is unconditional, and nothing in the package calls t.Parallel. /E stays documented as NOT gated. Review found it is now cheap to close for table files — obj 9 at 871 gives 871+n+eolAfter == 902, which equals /E — but the xref-stream shape needs more, and that is its own change rather than a rider on this one. gofmt clean, go vet clean, GOARCH=386 go vet clean, 0 uncovered blocks of 1782. Refs #40. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First half of #40: the judge, before the writer it will judge.
⛔⛔
Linearizedis a weaker test than it looksOf Table F.1's claims it checks exactly one against the file —
/Lagainst the byte count./O,/E,/Nand/Tare read for type and never consulted. So a file carrying a correct/Land four invented integers is accepted — which is precisely what a first linearizer emits.That is not a defect in it. It is deliberately cheap, because the reader it serves has fetched 1024 bytes and has no cross-reference table to resolve anything with. It is simply the wrong judge for a writer, and #40 opens the writer — so the judge comes first, and separately, so that the writer is developed against a check that was not written to let it through.
CheckLinearized(b)Not "may a reader trust the front of this file" but "is this file actually what the front says". It opens the document and establishes that every parameter points at what it claims to:
/N/O/Typeis/Page— a number that happens to be right while the object is something else is a file whose page tree was rebuilt under it/H/T/EThe gates are files the cheap check accepts
Each of the four is the committed qpdf fixture with one integer replaced by one of the same width, so
/Lstill equals the byte count and nothing at the front of the file is wrong — what is wrong is that it describes a different document.⛔ The helper asserts that width. An edit one byte longer would be refused for the length instead, the deep check would never run, and the test would pass while measuring nothing.
⛔ And the negative control: an ordinary file written by this package is refused by both checks. A suite in which every input passes measures nothing.
Mutations, judged by name rather than by count
Each of the four checks was made to always agree. The verdict came from
go-fleettools/fleettoolscmd/failsetrather than from a pass/fail count — every mutation red on its own subtest and no other:Still not checked
The hint tables (F.4), by either function.
qpdf --checkremains the foreign judge, andCheckLinearizedagrees with qpdf's output at 1, 2 and 5 pages.gofmtclean,go vetclean, 99.4% of statements.Refs #40.
🤖 Generated with Claude Code