diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 07f37fb..8ff0801 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,14 +22,58 @@ 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 + # ⛔⛔ 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 @@ -94,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: @@ -157,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 ./... diff --git a/README.md b/README.md index ffb5f2c..6e9b538 100644 --- a/README.md +++ b/README.md @@ -149,8 +149,42 @@ 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. + +`/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 +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..8353dde 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,64 @@ 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. + 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`. + 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 960856e..096b2ed 100644 --- a/linearized_test.go +++ b/linearized_test.go @@ -845,6 +845,136 @@ 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 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) + } +} + +// ⛔ 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 @@ -938,10 +1068,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