fix(pdf-codec): hold the text state parameters in the graphics state - #861
Merged
Conversation
The font and size a Tf selects, along with Tc/Tw/Tz/TL/Ts, are graphics state parameters (ISO 32000-1 Table 52), but they lived in a separate TextState record the q/Q stack never touched and every runContentStream call initialised fresh. Two distinct wrong behaviours followed. A Tf inside a q...Q pair kept its font past the matching Q instead of being restored, so a later text object with no Tf of its own drew in the leaked font -- and read.ts decodes a run's glyph codes through whichever font its fontResourceName names, so a leaked name corrupts the extracted Unicode rather than merely mislabelling it. A form XObject invoked by Do started with no font selected at all, so a form relying on the caller's already-selected font -- legal per 8.10.2, which runs a form in the graphics state effective at the Do -- produced zero text items, since showTextArray and advanceThroughString both early-return on an undefined fontResourceName. GraphicsState now carries those parameters, leaving the text matrix and text line matrix in a TextObjectState the q/Q stack excludes: those are text object state (9.4.1), reset by BT and not saved by q. The existing gsStack push and handleDo's existing formState spread then cover the text state with no second stack and no new parameter, and the recursed call's own gs local keeps a form's own text-state changes from escaping back into the caller.
Mearman
marked this pull request as ready for review
September 3, 2026 08:08
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
|
🎉 This PR is included in version 3.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
1 participant
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.
What this changes
packages/pdf-codec:interpret.ts'sGraphicsStatenow carries the text state parameters -- the font and size aTfselects, plusTc/Tw/Tz/TL/Ts-- instead of keeping them in a separateTextStaterecord alongside the text matrix. What is left beside the text matrix is aTextObjectStateholdingTm/Tlmand nothing else.Why
ISO 32000-1 Table 52 lists those parameters among the device-independent graphics state parameters, so
qsaves them,Qrestores them, and a form XObject invoked byDoinherits them (8.10.2). The old split modelled none of that:gsStackheld only CTM/fill/stroke/line-width, andtswas a freshdefaultTextState()perrunContentStreamcall thatq/Qnever touched. Two wrong behaviours followed, one at each boundary.BT /F1 12 Tf (First) Tj ET q /F2 24 Tf Q BT 0 -20 Td (Second) Tj ETdrew the second run in F2 at 24pt. That is not only a mislabelled item:read.ts'sconvertTextdecodes a run's glyph codes through whichever font itsfontResourceNamenames, so a leaked name runs the codes through the wrong font's cmap and silently corrupts the extracted Unicode.A form XObject whose own content omits a redundant
Tfbecause it relies on the caller's already-selected font produced zero text items --showTextArrayandadvanceThroughStringboth early-return on an undefinedfontResourceName, and the form's freshly-defaulted state never had one. Same silent-drop failure shape as #851, at theDoboundary rather than theBTone, and exactly what a repeated header/footer form would hit.Why merged rather than a second parallel stack
#859 floated both options. Merging is what the spec's own division actually says, and it makes both fixes structural rather than two things each operator has to remember:
case "q"/case "Q"are unchanged. One push covers everything saved, because everything saved lives in one record.handleDo'sformState({ ...gs, ctm: ... }) is unchanged. Inbound inheritance falls out of the spread that was already carrying the CTM, with no newrunContentStreamparameter and no seeding argument to keep in sync.gslocal, so a form's ownTf/Tccannot escape back to the caller.BTbecomestext = defaultTextObjectState(). fix(pdf-codec): keep the selected font across a BT with no Tf of its own #856 had to reset exactly two of nine fields by hand and comment why; now the reset object is the two fields, so the same correctness is enforced by the type rather than by care.A parallel stack would have needed
Qto restore seven fields while deliberately skipping two, in lockstep withgsStack-- the kind of hand-maintained pairing that goes wrong later.The text operators now update
gsby spread (gs = { ...gs, charSpace: ... }), matching howw,g,rg, andcmalready work in the same switch.Tests
Nine new cases, each written against the issues' own reproduction content streams and confirmed failing first (7 red, for the stated reasons, before the change):
q/Qrestores the font (q/Q don't save/restore text-state parameters (Tf/Tc/Tw/Tz/TL/Ts), so a font set inside q...Q leaks past the matching Q #854's exact repro), character spacing (q/Q don't save/restore text-state parameters (Tf/Tc/Tw/Tz/TL/Ts), so a font set inside q...Q leaks past the matching Q #854's second repro), word spacing, horizontal scaling, leading, and rise.q/Qpair is still not unwound by a later unmatchedQ.QleavesTmalone, since the text matrix is text object state rather than graphics state.BT /F1 12 Tf (Outer) Tj ET /Fm2 Doover a form whose content isBT (FormText) Tj ETnow yields both runs, the form's in F1/12).Tf/Tcdo not survive back into the caller -- the direction Form XObjects don't inherit the caller's text state, so a form relying on an already-selected font has its text silently dropped #855 confirmed already correct, pinned so the merge cannot regress it.Full
pdf-codecunit suite (1242), workerd suite, lint, and typecheck all pass, as do every dependent package's suites (documents.js2617,document-cli331,document-mcp) viaturbo run _test --filter=...pdf-codec.Fixes #854
Fixes #855
#859 is a duplicate of #854 -- same gap, less detail -- and is closed by this PR too.