Repository navigation
Conversation
e8ef98b to
e6b61eb
Compare
Carries upstream PR ledongthuc#61 (ToUnicode-first font decoding) so browser-printed PDFs stop extracting as glyph-index mojibake. Only go.mod and README differ from that PR; the fix commits are AJ Roetker's, unmodified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ledongthuc
left a comment
There was a problem hiding this comment.
Thanks for this PR — the direction is correct and both fixes are spec-compliant (ToUnicode precedence per §9.10.2, and BaseEncoding + Differences per §9.6.6). I verified locally: it builds, go vet is clean, the existing tests pass, and it merges cleanly.
I'd like a few changes before merging:
-
Identity-Hfallback —Identity-His a 2-byte CMap for Type0/CID fonts, so decoding byte-by-byte throughpdfDocEncodingproduces garbage when ToUnicode is absent. This predates the PR, but since this line is being touched, can we leave it asnopEncoderfor this case (or explicitly note it's out of scope)? -
Unknown named encodings (
defaultcase) — this changed fromnopEncodertopdfDocEncoding, which silently alters behavior forSymbol,ZapfDingbats,StandardEncoding,Identity-V, etc. None of those equal PDFDocEncoding;Symbol/ZapfDingbatsespecially will decode to wrong (but plausible-looking) Latin text. Recommend keepingnopEncoderfor genuinely unknown names, or adding proper tables. -
MacExpertEncodingfallback — silently mapping it topdfDocEncodingis inaccurate; please at least log viaDebugOnor add a real table. -
Default
BaseEncoding= PDFDocEncoding — per spec the default should be the font's built-in encoding (usually StandardEncoding). The comment acknowledges the simplification, but consider adding aStandardEncodingtable since the 0x80–0x9F range differs from PDFDocEncoding. -
/.notdefin Differences —nameToRunehas no.notdef, so a/.notdefentry silently keeps the BaseEncoding value instead of mapping tonoRune. -
readCmapcan panic —readCmaphaspanic("missing beginbfchar")/panic("missing beginbfrange"). Now that ToUnicode is parsed for every font, this expands the panic surface;Content()has norecover(), so a malformed ToUnicode CMap could crash. Recommend converting those to the existingok = false/return nilpattern. -
Tests — the PR reports a big improvement but has no regression tests. Could you add a unit test for
newDictEncoder(BaseEncoding + Differences, incl. a gap and an out-of-range code) and a ToUnicode-precedence test (e.g., a synthetic PDF likeps_test.go)? -
Minor: the branch is ~20 commits behind master; it merges cleanly, but a rebase would be nice.
None of these are hard blockers except arguably (2) and (6). Happy to iterate.
The PDF specification states that ToUnicode CMap is the authoritative source for character-to-Unicode mapping. Previously, the library only checked ToUnicode for fonts with "Identity-H" encoding or null encoding, causing incorrect text extraction for many PDFs. This change: - Checks ToUnicode CMap first before falling back to Encoding - Falls back to pdfDocEncoding instead of nopEncoder for better compatibility with unknown encodings - Removes the now-redundant charmapEncoding() method This fixes text extraction issues where characters were being incorrectly decoded (e.g., '0' appearing as 'M') due to ToUnicode being ignored when an Encoding entry was present.
The PDF spec (section 9.6.6) requires that when an Encoding dictionary is present, the BaseEncoding (e.g., WinAnsiEncoding, MacRomanEncoding) should be applied first, then the Differences array overlays specific character code mappings on top. Previously, dictEncoder only looked at the Differences array and matched character codes one by one, which was both slow and incorrect for fonts that rely on BaseEncoding for most characters. This fix: - Builds a complete 256-entry lookup table at initialization time - Copies the BaseEncoding table first (defaulting to PDFDocEncoding) - Applies Differences array entries on top - Uses O(1) lookup instead of O(n) scanning during decoding Fixes font encoding corruption in PDFs where fonts use custom Encoding dictionaries with BaseEncoding + Differences (common in legal documents).
- Identity-H without a usable ToUnicode returns nopEncoder again: it is a 2-byte CMap for Type0/CID fonts, so byte-wise table decoding would produce garbage (proper CID decoding remains out of scope) - Unknown named encodings (Symbol, ZapfDingbats, Identity-V, ...) return nopEncoder instead of guessing PDFDocEncoding - MacExpertEncoding fallback is now logged via DebugOn - Add a StandardEncoding table (PDF 32000-1:2008 Table D.2) and use it as the default BaseEncoding in dictEncoder, since built-in encodings are usually StandardEncoding and its 0x27/0x60/0x80-0x9F ranges differ from PDFDocEncoding - Map /.notdef in Differences to noRune instead of keeping the base value - Convert readCmap's beginbfchar/beginbfrange panics to the existing ok = false pattern so a malformed ToUnicode CMap can't crash Content() - Add unit tests for newDictEncoder (BaseEncoding + Differences overlay, gaps, out-of-range codes, /.notdef, StandardEncoding default) and synthetic-PDF tests for ToUnicode precedence and malformed-ToUnicode fallback; update TestDictEncoder to the new constructor
e6b61eb to
5a79951
Compare
- Reject codespace ranges wider than 4 bytes: m.space is a fixed [4] array, so a 5-byte range like <0000000000> <FFFFFFFFFF> previously caused an index-out-of-range panic — reachable from any untrusted PDF now that ToUnicode is parsed for every font - Detect truncated CMaps: reset the section counter after endbfchar/ endbfrange (previously only endcodespacerange reset it, so a stray end operator after a completed section escaped the missing-begin check) and treat a stream ending inside an open section as a parse failure so extraction falls back to /Encoding - Treat a CMap with no bfchar/bfrange mappings as a parse failure for the same reason: falling back to /Encoding beats decoding every code to the replacement character - Keep advancing the Differences code counter past out-of-range codes so a bad start code doesn't drop later entries that land in range - Add regression tests for all three cases
- Cap section entry counts against the operand stack size: Pop on an exhausted stack returns zero values, so an unchecked count like '9223372036854775807 beginbfchar' looped allocating unbounded entries from a tiny untrusted stream - Track which begin section is open so an end operator of the wrong kind (beginbfchar ... endbfrange) is rejected instead of matched - Require at least one codespace range: mappings without one can never match a code, so everything decoded to the replacement character instead of falling back to /Encoding - utf16Decode: drop a trailing odd byte instead of panicking on s[i+1] - cmap.Decode: an empty bfrange destination string decodes to noRune instead of panicking on b[len(b)-1] when scaling - Add regression tests for all five cases
- Recover from Interpret panics (unmatched end, begin on a non-dict, currentdict with no dictionary, ...) at the readCmap boundary so a malformed ToUnicode stream falls back to /Encoding instead of crashing Content() or failing the whole page's GetPlainText - Validate section entry counts against operands pushed since the section began, not the whole stack, so an entry missing an operand can't pass by borrowing unrelated values like begincmap's dict - Decode destinations that produce no UTF-16 units (a malformed single byte) to noRune instead of silently deleting the source character; genuinely empty destinations still map to nothing - Rename a local that shadowed the package-level name type - Add regression tests for the three behavioral fixes
Summary
This PR improves font encoding handling to fix text extraction issues in PDFs with complex font configurations. It includes two key fixes:
Problem
When extracting text from certain PDFs (e.g., scanned legal documents), characters were being incorrectly decoded:
1:15-cv-07433-LAPwere corrupted because Differences were applied without the BaseEncoding foundationSolution
Fix 1: ToUnicode Priority
Per the PDF specification (section 9.10.2), ToUnicode CMap should be the primary source for mapping character codes to Unicode. This change checks ToUnicode first:
Fix 2: BaseEncoding + Differences
Per PDF spec section 9.6.6, when an Encoding dictionary is present:
The new
dictEncoder:Testing
Tested with legal document PDFs (court documents) that previously had incorrect text extraction:
All characters including numbers (0-9), punctuation, and case numbers are now correctly extracted.
[Update] Review feedback addressed (2026-09-03)
All 8 review points are addressed (rebased onto master + one new commit):
nopEncoderwhen ToUnicode is absent/unparseable, with a comment noting that proper 2-byte CID decoding is out of scope.defaultcase returnsnopEncoderagain (same for the unexpected-kinddefaultat the bottom ofgetEncoder).DebugOn; falls back to StandardEncoding with a comment explaining why no substitute table is accurate.standardEncodingtable (Table D.2) intext.goand made it thedictEncoderdefault, so 0x27/0x60 and the 0x80–0x9F range are now correct./.notdef— explicitly mapped tonoRunein the Differences loop.beginbfchar/beginbfrangepanics converted to the existingok = false/return nilpattern, withDebugOnlogging.encoding_test.go: unit tests fornewDictEncoder(BaseEncoding + Differences overlay, gap via restarted code counter, out-of-range codes,/.notdef, StandardEncoding default) plus two synthetic-PDF tests: ToUnicode precedence (CMap remaps A→Z over WinAnsi) and malformed-ToUnicode fallback to /Encoding (exercises the de-panicked readCmap path). Also updated the existingTestDictEncoderto the new constructor.go build,go vet, and the full test suite (including the new master tests) pass.Follow-up hardening (commit a47f781)
A further review pass of the expanded ToUnicode parsing surface found three more robustness gaps, now fixed with regression tests:
<0000000000> <FFFFFFFFFF>) previously panicked with index-out-of-range on the fixed[4]space table — reachable from any untrusted PDF now that ToUnicode is parsed for every font. Now rejected cleanly.endbfchar/endbfrangenever reset the section counter (onlyendcodespacerangedid), so a stream cut off inside a section, or a strayendoperator, was treated as successfully parsed and every code decoded to U+FFFD. A CMap ending inside an open section (or containing no bfchar/bfrange mappings at all) now returns nil so extraction falls back to/Encoding.Follow-up hardening, round 2 (commit e21cd86)
A second review pass over the now-always-parsed ToUnicode path found two panic/DoS vectors and one fallback gap, all pre-existing but newly reachable from every font. Fixed with regression tests:
readCmaplooped on the untrusted entry count (Popon an exhausted stack returns zero values), so9223372036854775807 beginbfchar endbfcharin a tiny stream allocated unbounded entries. Counts are now capped against the operand stack size, and the open section kind is tracked sobeginbfchar ... endbfrangeis rejected rather than matched.utf16Decodeindexed past the end on odd-length destination strings (e.g. a bfchar destination of(Z)), and bfrange scaling indexedb[len(b)-1]on an empty destination. Both now decode to U+FFFD instead of panicking./Encodingrather than emitting only replacement characters.Follow-up hardening, round 3 (commit 01784f4)
readCmap—Interpretpanics on malformed PostScript (unmatchedend,beginon a non-dict,currentdictwith no dictionary);readCmapnow recovers and reports parse failure so extraction falls back to/Encodinginstead of crashingContent(). This completes item 6: no malformed ToUnicode input can now escape the CMap parsing boundary.begin*), so an entry missing an operand can't pass by borrowing unrelated stack values.nametype.