From 7cb2647b1269d89b4074753b4fcaf040993e22c6 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sat, 14 Nov 2026 13:40:29 +0100 Subject: [PATCH 1/5] ci: the foreign judge had never run in CI, and nothing could have said so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⛔⛔ EVERY TEST THAT AGREES WITH qpdf HAS BEEN SKIPPING ON EVERY LANE. Measured on run 43595391186: the string "qpdf" appears ZERO times in the whole log. ⛔ AND THE TIME IS THE WITNESS, which is the only thing that could have been noticed: this job reports ok github.com/go-pdfkit/reader 0.197s (CI) ok github.com/go-pdfkit/reader 0.758s (a workstation with qpdf) The sweep generates 42 files by invoking qpdf; it cannot do that in 0.197s. So the agreement across 1, 2, 3, 5, 25 and 100 pages in five shapes, AND the negative control that an ordinary file is REFUSED by the foreign judge — the one #44 added yesterday — have never been checked by anything. ⛔ `go test` WITHOUT -v PRINTS NOTHING ABOUT A SKIP, so the log was not hiding this badly, it could not report it at all. And coverage stayed at 100.0% throughout: the same statements are reached by the tests that need no qpdf, so the coverage gate cannot see this and never could. Two gates were green about a third thing neither measures. ⛔ THIS REPOSITORY ALREADY HOLDS THE OPPOSITE STANDARD, one job down, about a missing qemu emulator: "A missing emulator FAILS this lane. A lane that reports success for an architecture it never ran is the one outcome this matrix exists to rule out." A missing foreign judge is that outcome for a different reason. go-pdfkit/ops already does it right for pdftoppm. ⛔ REQUIRE_QPDF, NOT CI, because not every lane CAN run it and a rule that demands the impossible gets deleted. Under qemu-user the test binary is emulated and an execve of the host's x86-64 qpdf is not something the emulator can carry out; the Windows lane has no apt. So ONE lane installs qpdf and sets REQUIRE_QPDF, and there a missing judge FAILS. The others skip, which is now a stated design instead of an accident nobody could see. Measured, both directions, rather than predicted: REQUIRE_QPDF=1, qpdf off PATH -> FAIL, naming the reason unset, qpdf off PATH -> skip, exit 0 ⛔ aptneed FOR THE INSTALL, because a mirror outage must not arrive as a missing judge: go-pdfkit/ops#90 was four red runs in half an hour on a third party's DNS. `--need qpdf` asks for the BINARY, since apt saying yes is not qpdf being on PATH. And `qpdf --version` is printed, so which judge agreed is answerable afterwards rather than assumed. ⚠ NOT FIXED, and deliberately: actionlint reports SC2086 on `go test $ex ./...` in the qemu job. It is pre-existing — main carries it at line 141 — and shellcheck is WRONG here. `ex` is empty on the 386 lane, which runs natively on purpose, so quoting it would hand `go test` an empty package pattern and break the one lane that must not be emulated. Reader's CI never ran actionlint, which is why it went unnoticed; silencing it properly is its own change. gofmt clean, go vet clean, 0 uncovered blocks of 1806. Refs #40. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 33 +++++++++++++++++++++++++++++++++ linearized_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 07f37fb..5f1cf49 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,10 +26,43 @@ jobs: check-latest: true cache: true + # ⛔⛔ THE FOREIGN JUDGE HAD NEVER RUN ON ANY LANE. Measured on run + # 43595391186: "qpdf" appears ZERO times in the whole log, and this job + # reported `ok 0.197s` where the same suite takes 0.758s on a machine + # that has qpdf. Every test that agrees with qpdf — the sweep over 42 + # generated files, and the control that an ordinary file is REFUSED — + # skipped, and `go test` without -v says nothing about a skip, so + # nothing in the log could have told anyone. Coverage stayed at 100% + # the whole time: the same statements are reached by the tests that + # need no qpdf, so that gate cannot see this and never could. + # + # ⛔ This repository already holds the opposite standard one job down, + # about a missing qemu emulator: "A lane that reports success for an + # architecture it never ran is the one outcome this matrix exists to + # rule out." + # + # aptneed because a mirror outage must not read as a missing judge: + # go-pdfkit/ops#90 was four red runs in half an hour on a third + # party's DNS. --need is the binary, not the package: apt saying yes is + # not qpdf being on PATH. + - name: Install qpdf, the foreign judge the linearization tests print by + run: | + GOBIN=/usr/local/bin go install github.com/go-fleettools/fleettools/cmd/aptneed@latest + sudo aptneed --need qpdf qpdf + + # Printed on purpose: the version is in the log, so "which qpdf agreed" + # is answerable after the fact rather than assumed. + - name: Say which qpdf will judge + run: qpdf --version + - name: go vet run: go vet ./... + # REQUIRE_QPDF makes a missing judge FAIL here instead of skipping. It + # is set on this lane only, because this is the lane that installs it. - name: Test with coverage + env: + REQUIRE_QPDF: "1" run: go test -covermode=set -coverprofile=coverage.out ./... - name: Assert exact 100% statement coverage diff --git a/linearized_test.go b/linearized_test.go index 960856e..c1e37c9 100644 --- a/linearized_test.go +++ b/linearized_test.go @@ -938,10 +938,43 @@ func TestCheckLinearizedAgreesWithQpdfAcrossPageCountsAndShapes(t *testing.T) { } } +// qpdfOrSkip finds the foreign judge, and REFUSES TO SKIP IT UNDER CI. +// +// ⛔⛔ IT SKIPPED EVERYWHERE, AND NOWHERE MORE QUIETLY THAN IN CI. Measured on +// run 43595391186: the word "qpdf" appears ZERO times in the whole log, and +// the suite reports `ok 0.197s` where the same suite takes 0.758s on a +// workstation that has qpdf. Every test that agrees with qpdf — the sweep +// across 42 generated files, and the negative control that an ordinary file +// is refused — has never run on any lane, and `go test` without -v prints +// nothing about a skip, so the log could not say so either. Coverage stayed +// at 100% throughout, because the same statements are reached by the tests +// that need no qpdf: that gate cannot see this and never could. +// +// ⛔ THIS REPOSITORY ALREADY HOLDS THE OPPOSITE STANDARD, one job away. Of a +// missing qemu emulator its own workflow says: "A missing emulator FAILS this +// lane. A lane that reports success for an architecture it never ran is the +// one outcome this matrix exists to rule out." A missing foreign judge is the +// same outcome for a different reason, and go-pdfkit/ops already does this +// for pdftoppm — its render tests t.Fatal when CI is set. +// +// ⛔ IT KEYS ON REQUIRE_QPDF AND NOT ON CI, because not every lane CAN run it +// and a rule that demands the impossible gets deleted. Under qemu-user the +// test binary is emulated and an `execve` of the host's x86-64 qpdf is not +// something the emulator can carry out; the Windows lane has no apt. So ONE +// lane installs qpdf and sets REQUIRE_QPDF, and on that lane a missing judge +// is a failure. The others skip, which is now a stated design rather than an +// accident nobody could see. +// +// Off CI it skips too: a contributor without qpdf should be able to run the +// suite, and that skip is visible to them because they chose it. func qpdfOrSkip(t *testing.T) string { t.Helper() path, err := exec.LookPath("qpdf") if err != nil { + if os.Getenv("REQUIRE_QPDF") != "" { + t.Fatal("REQUIRE_QPDF is set and qpdf is not installed: the foreign judge is the whole " + + "point of this test, and a lane that skips it reports success for an agreement it never checked") + } t.Skip("qpdf is not installed, so there is no independent linearizer to agree with") } return path From b25130e1aa08795b359573d00de1cce16c9dbbb7 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sat, 14 Nov 2026 21:07:37 +0100 Subject: [PATCH 2/5] ci: Go 1.27.4, because installing a fleet tool requires it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⛔⛔ SECOND OCCURRENCE IN TWO DAYS, AND I PREDICTED THIS ONE IN WRITING. The first push of the qpdf lane failed on go: …/cmd/aptneed@latest: fleettools@v0.112.0 requires go >= 1.27.4 (running go 1.27.3; GOTOOLCHAIN=local) go-pdfkit/ops hit the identical line on 2026-11-13, and after fixing it I wrote on that pull request: "Worth knowing for other repositories in the fleet that pin a toolchain and install fleettools commands in CI: the same pin blocks them the moment they try." Then I walked into it myself, one repository later, the next day. A note is applied when somebody remembers to apply it, and at the moment of adding an install step one is thinking about the install. So it is recorded as a recurrence rather than a third note. The gate that catches it is a check over `.github/workflows` for a `/cmd/@latest` install beside a `go-version` pin below that module's `go` directive — already filed as pending on go-fleettools/fleettools#103, where the `@latest` half of the same problem lives. `tagscan` cannot see either: it reads repositories, not workflows. All three jobs move to 1.27.4, not only the one that installs aptneed. The other two do not need it, but a matrix measuring on two different toolchains is a difference nobody chose, and this fleet has already been bitten by `stable` quietly selecting a minor. ⛔ GOTOOLCHAIN IS STILL LEFT ALONE. That is what made this a clean failure naming both versions instead of a runner silently fetching 1.27.4 and measuring a chain nobody picked. go.mod stays at `go 1.27.1`: that is the language version this module needs, not the toolchain a runner must have, and nothing here requires it to move. actionlint reports only the pre-existing SC2086 discussed in the previous commit. Refs #40, #44. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5f1cf49..8ff0801 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,7 +22,18 @@ jobs: - uses: actions/setup-go@v7 with: - go-version: '1.27.3' + # ⛔ 1.27.4 BECAUSE INSTALLING A FLEET TOOL REQUIRES IT. fleettools + # declares `go >= 1.27.4`, so 1.27.3 could not install aptneed at + # all: "requires go >= 1.27.4 (running go 1.27.3; + # GOTOOLCHAIN=local)". GOTOOLCHAIN is left alone on purpose — a + # runner that silently fetched a newer toolchain would measure one + # nobody chose — so the pin is what has to move. + # + # ⛔ SECOND OCCURRENCE IN TWO DAYS: go-pdfkit/ops hit the identical + # line on 2026-11-13. Any repository in this fleet pinning below + # 1.27.4 fails the moment it installs a fleettools command, and the + # failure names both versions, so it is at least legible. + go-version: '1.27.4' check-latest: true cache: true @@ -127,7 +138,7 @@ jobs: - uses: actions/checkout@v7 - uses: actions/setup-go@v7 with: - go-version: '1.27.3' + go-version: '1.27.4' # ⛔ `qemu-user-static` is a VIRTUAL package on Ubuntu 26.04, which # `ubuntu-latest` became on 2026-10-19, and apt refuses to choose between its # two providers: @@ -190,5 +201,5 @@ jobs: - uses: actions/checkout@v7 - uses: actions/setup-go@v7 with: - go-version: '1.27.3' + go-version: '1.27.4' - run: go test ./... From 02e87b548e29fd698a75919eddddd1dd831262c5 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sat, 14 Nov 2026 21:54:30 +0100 Subject: [PATCH 3/5] linearize: the shared object hint table is required, and nothing looked inside /H MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First result of reading Annex F itself, which #40 listed under "what has NOT been read yet". ⛔⛔ /S IS REQUIRED AND A WRITER THAT FORGOT IT PASSED EVERYTHING. Table F.2 lists the shared object hint table as "(Required)", and F.3.6 says the primary hint stream's dictionary "shall contain entries giving the position of the beginning of each hint table in the stream". Every check this file had about /H was about the stream's EXTENT — it starts at a real indirect object, that object is a stream, its length matches what the parse measured. Its dictionary was never read. So a primary hint stream naming no hint table at all was sound by this checker, and a reader that fetched the front of the file would have nothing to look up. This is the first thing checked INSIDE a hint stream rather than about it. Everything else is an offset or a length; /S is content. ⛔ ONLY THE PRIMARY, AND THE SPEC IS EXPLICIT ABOUT WHY. F.3.6: "the dictionary of the overflow hint stream shall not contain these entries". The same requirement in checkOverflowHint would refuse exactly the conforming file. The two halves of /H are not symmetric and this is where they differ — which is also why no test asserts the absence of the check there: with the fixture's overflow pair pointing at the primary stream, adding it would break nothing, so it would be a claim no mutation could kill. ⛔ PRESENCE, NOT POSITION. The page offset table "shall be the first table in the stream and shall start at offset 0" and has no key at all, so the positions inside the stream are F.4's question and this does not open it. Measured on both judges: - all three committed fixtures carry /S — the two qpdf shapes AND the Ghostscript one, so the positive control spans two producers; - a same-width edit of `/S 43` to `/X 43` is refused here by name; - `qpdf --check` refuses that same file too, with "ERROR: Integer operation attempted on invalid object" — it reads the missing /S as an integer. So this is NOT stricter than the reference, and the message here is the more useful of the two. ⛔ AND ONE PROPERTY OF THE FOREIGN JUDGE WORTH RECORDING: qpdf --check prints "File is linearized" from the parameter dictionary BEFORE validating the hint tables, and then errors. That line is not its approval. Anything reading qpdf as a verdict rather than a report has to read past it. ⛔ A COMMENT OF MINE WAS WRONG AND THE SPEC CORRECTED IT. checkFirstPageEnd said "Annex F puts the first-page section at part 3 and the main one at part 10". The main cross-reference table is part 11; part 10 is the OVERFLOW HINT STREAM. The code was right — it compares against /T — and the sentence explaining it named the wrong part. Mutation, judged by cmd/failset on the failing NAMES: dropping the /S check is red on TestAPrimaryHintStreamWithNoSharedObjectTableIsRefused and no other test. gofmt clean, go vet clean, 0 uncovered blocks of 1808. Refs #40. Co-Authored-By: Claude Opus 5 --- README.md | 26 ++++++++++++++++++++++++-- linearized.go | 29 +++++++++++++++++++++++++++-- linearized_test.go | 25 +++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index ffb5f2c..0087b25 100644 --- a/README.md +++ b/README.md @@ -149,8 +149,30 @@ says". It opens the document, so it costs what opening costs, and it is the judgement a writer needs on bytes it has just produced. It establishes that every parameter **points at what it claims to** — `/N` is the real page count, `/O` is the real first page *and* is a `/Page`, `/H` begins at a real stream -object, `/T` reaches the main cross-reference (either shape: Table F.1 gives -it two meanings and both are real), `/E` is where the first page really ends. +object **whose dictionary names the shared object hint table Table F.2 +requires**, `/T` reaches the main cross-reference (either shape: Table F.1 +gives it two meanings and both are real), `/E` is where the first page really +ends. + +⛔ **`/S` is the first thing checked *inside* a hint stream rather than about +it.** Everything else above is an offset or a length; `/S` is content. F.3.6 +says the primary stream's dictionary "shall contain entries giving the +position of the beginning of each hint table in the stream", and Table F.2 +marks the shared object hint table **Required** — so a primary stream naming +none leaves a reader that fetched the front of the file unable to find one. +Both committed qpdf fixtures and the Ghostscript one carry it; a same-width +edit of `/S` to `/X` is refused, and `qpdf --check` refuses that file too, +less helpfully (`ERROR: Integer operation attempted on invalid object`). + +⚠ Only the **primary** stream. F.3.6 is explicit that "the dictionary of the +overflow hint stream shall not contain these entries", so the same +requirement on the overflow half would refuse exactly the conforming file. +The two halves of `/H` are not symmetric, and this is where they differ. + +⚠ And `qpdf --check` prints `File is linearized` from the parameter +dictionary **before** it validates the hint tables, then reports errors after +it. That line is not qpdf's approval, which matters to anyone reading its +output as a judgement rather than as a report. What gates it are files `Linearized` **accepts** and `CheckLinearized` refuses: the committed qpdf fixture with one integer diff --git a/linearized.go b/linearized.go index a30a941..f7bdd8d 100644 --- a/linearized.go +++ b/linearized.go @@ -340,7 +340,9 @@ func checkFirstPageEnd(b []byte, lin *Linearization) error { // miss because a reader believes it. // // Annex F puts the first-page section at part 3 and the main one at part - // 10, so the front section precedes /T. checkMainXref has already + // 11 — part 10 is the OVERFLOW hint stream, which this comment had wrong + // until F.3.1's layout was read — so the front section precedes /T. + // checkMainXref has already // established /T against the file by the time this runs, and this says // nothing about /E, so it still holds when /E is the thing under test. if off >= int64(lin.MainXrefOffset) { @@ -483,10 +485,33 @@ func checkHintStream(b []byte, lin *Linearization) error { return fmt.Errorf("%w: /H points at byte %d, where no indirect object parses: %v", ErrLinearizationStale, lin.HintOffset, err) } - if _, ok := obj.(*Stream); !ok { + s, ok := obj.(*Stream) + if !ok { return fmt.Errorf("%w: /H points at byte %d, where the object is %T and not a stream", ErrLinearizationStale, lin.HintOffset, obj) } + // ⛔⛔ /S IS REQUIRED, AND A WRITER THAT FORGETS IT PASSED EVERYTHING ELSE. + // Table F.2 lists the shared object hint table as "(Required)", and F.3.6 + // says the primary hint stream's dictionary "shall contain entries giving + // the position of the beginning of each hint table in the stream". So a + // primary stream with no /S names no shared object hint table, and a + // reader that fetched the front of the file cannot find one. qpdf reports + // it; this checked the stream's extent and never its contents. + // + // ⛔ ONLY THE PRIMARY. F.3.6 is explicit that "the dictionary of the + // overflow hint stream shall not contain these entries", so requiring /S + // in checkOverflowHint would refuse exactly the conforming file. The two + // halves of /H are not symmetric and this is where they differ. + // + // ⛔ Presence only, not its value: the page offset table "shall be the + // first table in the stream and shall start at offset 0" and has no key at + // all, so the positions inside the stream are F.4's question, which this + // does not open. + if _, ok := s.Dict["S"]; !ok { + return fmt.Errorf("%w: the primary hint stream at byte %d has no /S, and Table F.2 "+ + "makes the shared object hint table required", + ErrLinearizationStale, lin.HintOffset) + } // ⛔ 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 diff --git a/linearized_test.go b/linearized_test.go index c1e37c9..8348b89 100644 --- a/linearized_test.go +++ b/linearized_test.go @@ -845,6 +845,31 @@ func TestAnOrdinaryFileIsRefusedByBothChecks(t *testing.T) { } } +// ⛔⛔ /S IS REQUIRED AND NOTHING LOOKED AT IT. Table F.2 lists the shared +// object hint table as "(Required)", and F.3.6 says the primary hint stream's +// dictionary "shall contain entries giving the position of the beginning of +// each hint table in the stream". A primary stream naming none passed every +// other check here: /H's extent was verified against the object that is +// there, and its dictionary never was. +// +// ⛔ THE EDIT IS SAME-WIDTH, /S -> /X, so the stream, its /Length and every +// offset after it are untouched. The only difference is that the key Table +// F.2 requires is absent — and /X is a key no part of Annex F defines, which +// is what a writer that invented its own name would produce. +func TestAPrimaryHintStreamWithNoSharedObjectTableIsRefused(t *testing.T) { + b := editedFixturePadded(t, [2]string{"/S 43", "/X 43"}) + 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(), "has no /S") { + t.Errorf("it was refused, but not for the missing /S: %v", err) + } +} + // ⛔⛔ 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 From de90c7b329ea43f0d6316a6e8d42849174122e10 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sat, 14 Nov 2026 21:57:49 +0100 Subject: [PATCH 4/5] linearize: /S has to be a direct integer, not merely present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⛔ PRESENCE IS WEAKER THAN WHAT THE SPEC SAYS, and I shipped the weaker thing in the previous commit. F.3.6: "the values of all entries in the hint streams' dictionaries shall be direct objects and may contain no indirect object references", the stream Length excepted. /S is a position counted in bytes from the start of the stream data. A name, or an indirect reference, is unusable by precisely the reader this format exists for — one that has fetched the front of the file and can resolve nothing. `/S /x` passed the presence check and names no position at all. Same-width edit again, `/S 43` -> `/S /x`, so only the value changes. gofmt clean, go vet clean, 0 uncovered blocks of 1810. Refs #40. Co-Authored-By: Claude Opus 5 --- linearized.go | 15 ++++++++++++++- linearized_test.go | 23 +++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/linearized.go b/linearized.go index f7bdd8d..5f844ed 100644 --- a/linearized.go +++ b/linearized.go @@ -507,11 +507,24 @@ func checkHintStream(b []byte, lin *Linearization) error { // first table in the stream and shall start at offset 0" and has no key at // all, so the positions inside the stream are F.4's question, which this // does not open. - if _, ok := s.Dict["S"]; !ok { + sv, ok := s.Dict["S"] + if !ok { return fmt.Errorf("%w: the primary hint stream at byte %d has no /S, and Table F.2 "+ "makes the shared object hint table required", ErrLinearizationStale, lin.HintOffset) } + // ⛔ AND IT MUST BE A DIRECT INTEGER, which presence alone does not say. + // F.3.6: "the values of all entries in the hint streams' dictionaries + // shall be direct objects and may contain no indirect object references", + // the stream Length excepted — and /S is a position counted in bytes, so + // an indirect reference or a name is unusable by the very reader this + // format exists for, which has fetched the front of the file and cannot + // resolve anything. A presence-only check accepts `/S /x`. + if _, ok := sv.(Integer); !ok { + return fmt.Errorf("%w: the primary hint stream's /S is %T, and F.3.6 requires a direct "+ + "integer position for every hint table it names", + ErrLinearizationStale, sv) + } // ⛔ 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 diff --git a/linearized_test.go b/linearized_test.go index 8348b89..880b3ac 100644 --- a/linearized_test.go +++ b/linearized_test.go @@ -870,6 +870,29 @@ func TestAPrimaryHintStreamWithNoSharedObjectTableIsRefused(t *testing.T) { } } +// ⛔⛔ AND PRESENCE IS NOT ENOUGH, which the spec says and a presence-only +// check does not. F.3.6: "the values of all entries in the hint streams' +// dictionaries shall be direct objects and may contain no indirect object +// references", the stream Length excepted. /S is a position counted in bytes, +// so a name — or an indirect reference — is unusable by the reader this +// format exists for, which has fetched the front of the file and can resolve +// nothing. +// +// `/S 43` -> `/S /x` is the same width, so again only the value changes. +func TestAPrimaryHintStreamWhoseSharedTablePositionIsNotANumberIsRefused(t *testing.T) { + b := editedFixturePadded(t, [2]string{"/S 43", "/S /x"}) + 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(), "/S is reader.Name") { + t.Errorf("it was refused, but not for /S's type: %v", err) + } +} + // ⛔⛔ 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 From c57548fab7e80a6012105864a8da091eac1e7e6c Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sat, 14 Nov 2026 22:04:20 +0100 Subject: [PATCH 5/5] linearize: the /S test was weaker than the /S check, and -1 was a position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the gap by mutating what I had not mutated, which is exactly why it was asked. ⛔⛔ THE TEST COULD ONLY EXPRESS ONE WRONG KIND. Narrowing the check from "refuse anything that is not an Integer" to "refuse only a Name" left the suite GREEN: `/S /x` is the single case a same-width edit of `/S 43` can write, and it is a Name, so the narrowed check still refused it. The code was right and the evidence for it covered one seventh of what it claims. ⛔ SAME-WIDTH CANNOT REACH THE REST, and that is why they were missing rather than forgotten. `/S 4.3` is six characters against five; an indirect reference or an array is longer still. And the hint stream sits AFTER the fixture's padding run, so growing it there moves the object /H points at and the file is refused for its hint length instead — the wrong reason, which reads as a pass for this test. ⭐ SO THE FUNCTION IS ASKED DIRECTLY. checkHintStream takes bytes and parameters; these cases need nothing else. /H's extent is computed FROM THE PARSE so that nothing earlier can be what refuses the file, and each case differs from the accepted one in exactly one thing: the kind of /S. Seven kinds — name, indirect reference, array, real, string, boolean, null — and a CONTROL asserting an integer /S is accepted, without which the seven would hold against a version that refused this hand-built stream outright. Re-measured: narrowing to Name is now red on six of the seven and the parent, and no other test. The one it does not redden is the Name case, which that mutation still refuses — as it should be. ⛔ AND -1 WAS A POSITION. Review measured `/S -1` accepted, qpdf answering "overflow reading bit stream" — a symptom of seeking outside the data, not a statement about /S. F.3.6: the positions "shall be counted in bytes relative to the beginning of the stream data", so negative names nothing. Same-width edit, so only the value changes. ⚠ THE UPPER BOUND STAYS OPEN, DELIBERATELY. /S is an offset into the DECODED stream data, so bounding it means decoding the hint stream — the first step of reading the tables themselves (F.4), which belongs with that work and not smuggled in here. qpdf reports "/S (shared object) offset is out of bounds" for a /S past the data and this does not. Stated in the README and recorded on #40 rather than left for somebody to discover. ⭐ And the review settled the question this check stands or falls on. /S is required UNCONDITIONALLY, from F.4.2: "All objects associated with the first page (part 6) shall have entries in the shared object hint table, regardless of whether they are actually shared." The first page always has at least its page object, so the table is never empty and no conforming writer may omit it. Requiring a Real is also wrong per F.3.6's "counted in bytes". gofmt clean, go vet clean, 0 uncovered blocks of 1812. Refs #40. Co-Authored-By: Claude Opus 5 --- README.md | 12 +++++++ linearized.go | 20 ++++++++++- linearized_test.go | 82 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 0087b25..6e9b538 100644 --- a/README.md +++ b/README.md @@ -169,6 +169,18 @@ overflow hint stream shall not contain these entries", so the same requirement on the overflow half would refuse exactly the conforming file. The two halves of `/H` are not symmetric, and this is where they differ. +`/S` must also be a **direct integer and not negative**: F.3.6 says these +positions "shall be counted in bytes relative to the beginning of the stream +data", so a name, an indirect reference or `-1` names no position at all. A +reader that has fetched only the front of the file can resolve nothing, which +is the whole point of the format. + +⚠ **The upper bound on `/S` is NOT checked, and that is an open limit.** It is +an offset into the *decoded* stream data, so bounding it means decoding the +hint stream — which is the first step of reading the tables themselves (F.4) +and belongs with that work. `qpdf --check` reports `/S (shared object) offset +is out of bounds` for a `/S` past the data and this does not. Tracked in #40. + ⚠ And `qpdf --check` prints `File is linearized` from the parameter dictionary **before** it validates the hint tables, then reports errors after it. That line is not qpdf's approval, which matters to anyone reading its diff --git a/linearized.go b/linearized.go index 5f844ed..8353dde 100644 --- a/linearized.go +++ b/linearized.go @@ -520,11 +520,29 @@ func checkHintStream(b []byte, lin *Linearization) error { // an indirect reference or a name is unusable by the very reader this // format exists for, which has fetched the front of the file and cannot // resolve anything. A presence-only check accepts `/S /x`. - if _, ok := sv.(Integer); !ok { + pos, ok := sv.(Integer) + if !ok { return fmt.Errorf("%w: the primary hint stream's /S is %T, and F.3.6 requires a direct "+ "integer position for every hint table it names", ErrLinearizationStale, sv) } + // ⛔ AND A POSITION CANNOT BE NEGATIVE. F.3.6: the positions "shall be + // counted in bytes relative to the beginning of the stream data". Review + // measured `/S -1` passing, with qpdf answering "overflow reading bit + // stream" — a symptom of seeking outside the data rather than a statement + // about /S. + // + // ⚠ THE UPPER BOUND IS NOT CHECKED, and that is an open limit rather than + // an oversight: /S is an offset into the DECODED stream data, so bounding + // it means decoding the hint stream — which is the first step of reading + // the tables themselves (F.4) and belongs with that work. qpdf reports + // "/S (shared object) offset is out of bounds" for `/S 99` here and this + // does not. Recorded on #40. + if pos < 0 { + return fmt.Errorf("%w: the primary hint stream's /S is %d, and a position counted in bytes "+ + "from the start of the stream data cannot be negative", + ErrLinearizationStale, int64(pos)) + } // ⛔ 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 diff --git a/linearized_test.go b/linearized_test.go index 880b3ac..096b2ed 100644 --- a/linearized_test.go +++ b/linearized_test.go @@ -893,6 +893,88 @@ func TestAPrimaryHintStreamWhoseSharedTablePositionIsNotANumberIsRefused(t *test } } +// ⛔ A POSITION CANNOT BE NEGATIVE, which review measured passing: `/S -1` +// was accepted here while qpdf answered "overflow reading bit stream" — a +// symptom of seeking outside the data rather than a statement about /S. +// +// `/S 43` -> `/S -1` is the same width, so again only the value changes. +func TestANegativeSharedTablePositionIsRefused(t *testing.T) { + b := editedFixturePadded(t, [2]string{"/S 43", "/S -1"}) + 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(), "cannot be negative") { + t.Errorf("it was refused, but not for the negative position: %v", err) + } +} + +// ⛔⛔ 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 +// `/S 43` can express. `/S 4.3` is six characters against five, and an +// indirect reference or an array is longer still — and the hint stream sits +// AFTER the fixture's padding run, so growing it there moves the object /H +// points at and the file fails for the wrong reason. +// +// ⛔ SO THE FUNCTION IS ASKED DIRECTLY. checkHintStream takes the bytes and +// the parameters, which is all these cases need: a stream object whose /S is +// the wrong kind, with /H's extent made consistent from the parse itself so +// that nothing EARLIER can be what refuses the file. Each case then differs +// from the accepted one in exactly one thing — the kind of /S. +func TestEveryNonIntegerSharedTablePositionIsRefused(t *testing.T) { + for _, c := range []struct{ name, s string }{ + {"a name", "/x"}, + {"an indirect reference", "9 0 R"}, + {"an array", "[ 43 ]"}, + {"a real", "4.3"}, + {"a string", "(43)"}, + {"a boolean", "true"}, + {"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) + if !errors.Is(err, ErrLinearizationStale) { + t.Fatalf("/S as %s was accepted (or refused oddly): %v", c.name, err) + } + if !strings.Contains(err.Error(), "F.3.6 requires a direct") { + t.Errorf("it was refused, but not for /S's kind: %v", err) + } + }) + } + + // ⛔ 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. + 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)} + if err := checkHintStream(b, lin); err != nil { + t.Fatalf("an integer /S was refused, so these cases prove nothing: %v", err) + } + }) +} + // ⛔⛔ 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