Repository navigation
Conversation
Rendering a page with an MMR-coded JBIG2 generic region killed the process with SIGTRAP (exit 133). The vendored stub of pdfium's faxmodule.cpp had drifted from upstream in three places: - FaxG4GetRow was rewritten without the end-of-row returns and without pass mode, and with the T.6 mode codes shifted by one bit. A decoded row ran on into the next row's codes, a0 passed the row width, the reference-line lookup indexed past the stride and the span bounds check trapped. Rows that did not trap decoded wrong. - kFaxWhiteRunIns was missing the entry for white run 18 (0x27, 18, 0). The table is a sequence of count-prefixed groups, so every code after the 7-bit group was misread and a lookup could walk off the table. - FindBit lacked upstream's clamp of the found position to max_pos. Restore the upstream text (pdfium chromium/7000, the revision the stub matches elsewhere) for all three, keeping the stub's signature. The row decoder also takes the run-length-exceeds-row guard from current pdfium main. Every JBIG2 user of MMR (generic regions, pattern dictionaries, halftone regions, Huffman symbol dictionaries) goes through this path. The new unit test writes MMR regions with libtiff through Pillow and compares every rendered sample with the source bitmap; its fixtures cover the issue's 64x16 box, a 32x32 all-white region, a 33x7 all-black region and a 100x24 bitmap that exercises pass, horizontal and all vertical modes. All four trapped before this change. Closes docling-project#440 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Jeff Witt <1848307+wittjeff@users.noreply.github.com>
Contributor
|
✅ DCO Check Passed Thanks @wittjeff, all your commits are properly signed off. 🎉 |
Contributor
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
This branch has not been deployed
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.
Rendering a page that draws a JBIG2 image with an MMR-coded generic region killed the process with SIGTRAP (exit 133), in every release since the JBIG2 decoder landed in #252. Arithmetic-coded JBIG2 was fine; parsing without a
render_configwas fine.Resolves #440
Cause. The vendored stub of pdfium's
core/fxcodec/fax/faxmodule.cpphad drifted from upstream (diffed against pdfiumchromium/7000, the revision the rest of the stub matches):FaxG4GetRowwas rewritten without the end-of-row returns and without pass mode, and with the T.6 mode codes shifted by one bit (0001pass mode read as V±2,00001xV±2 read as V±3, V±3 read as end of row). A decoded row ran on into the next row's codes,a0passed the row width, the reference-line lookupref_buf[a0 / 8]indexed past the stride and the span bounds check trapped. That is theStartDecodeMMR → FaxG4Decodetrap in the report. Rows that did not trap decoded wrong. It also explains the size pattern in the report: at 8x8 the stride is 4 bytes anda0 / 8stays inside it, at 32x32 the width equals the stride in bits and the first lookup past the row traps.kFaxWhiteRunInswas missing one entry,0x27, 18, 0(white run 18). The table is a sequence of count-prefixed groups, so every code after the 7-bit group was misread and a lookup could walk off the end of the table into the span bounds check. Any white run coded with 7 or more bits hit this (white run 0 is an 8-bit code).FindBitlacked upstream's clamp of the found position tomax_pos.Fix. Restore the upstream text for all three, keeping the stub's
FaxG4Decodesignature. The row decoder also takes the run-length-exceeds-row guard from current pdfiummain. The white table change is one inserted entry; the rest of that table andkFaxBlackRunInsandkOneLeadPosalready matched upstream. Every JBIG2 user of MMR reachesFaxG4Decodethrough the one call inCJBig2_GRDProc::StartDecodeMMR: generic regions (JBig2_Context.cpp), pattern dictionaries (JBig2_PddProc.cpp), halftone regions (JBig2_HtrdProc.cpp) and the collective bitmaps of Huffman symbol dictionaries (JBig2_SddProc.cpp), so the "not tried" cases in the report are covered by the same change. Nothing else calls it, so arithmetic-coded JBIG2 and/CCITTFaxDecode(which has its own decoder insrc/parse/utils/ccitt/) are untouched.I did not route MMR through the in-tree CCITT decoder as the report suggests: that needs a stride packer and a bit-position handoff back to the JBIG2 stream (symbol dictionaries continue after the collective bitmap), which is more new code than restoring the upstream text. pdfium's remaining
CHECKs still abort the process by design; making the vendored tree fail one image instead is a separate policy change and is out of scope here.Test.
tests/test_unit_jbig2_mmr.pywrites MMR regions with libtiff through Pillow (a Group 4 TIFF strip holds exactly the T.6 codes a JBIG2 MMR region carries) and compares every rendered sample with the source bitmap. Fixtures: the report's 64x16 box, a 32x32 all-white region, a 33x7 all-black region and a 100x24 bitmap whose rows shift by 0 to 3 samples, add and drop runs and touch the right edge, so pass, horizontal and all vertical modes are exercised. All four fixtures also decode identically in pypdfium2. Skipped where Pillow has no libtiff.After the row decoder alone was restored, the 33x7 and 100x24 fixtures still trapped; that is how the missing table entry was found.
Evidence:
The regression dataset has no MMR-coded JBIG2: a segment-header scan of all 453 documents finds JBIG2 in one document only (
complex_jbig2_overlays.pdf, arithmetic symbol dictionaries and text regions), so no existing page reproduces the problem and the corpus-wide pypdfium2 metrics cannot move with this change. The pdfium test corpus does have real MMR-coded JBIG2 (22 files; 21 trap onmain, all survive on this branch). Cited pages, rendered at scale 1 with the threaded parser; "before" is the 7.22.1 wheel, "after" this branch, reference pypdfium2 5.13.0:pdfium/bug_880920.pdfp1 (a 3300x2550 scanned page: Huffman symbol dictionary with MMR collective bitmaps)third_party/jbig2/bitmap-mmr.pdf(generic region, MMR)third_party/jbig2/bitmap-halftone-10bpp-mmr.pdf(pattern dictionary + halftone region, MMR)third_party/jbig2/bitmap-symbol-symhuff-texthuff.pdf(Huffman symbol dictionary, MMR collective bitmap)Over all 52 files of the corpus'
third_party/jbig2set (SerenityOS JBIG2 tests converted to PDF, BSD-2-Clause) plusbug_880920.pdf: the 31 non-MMR files render the same before and after; of the 21 MMR files, 15 now match pypdfium2 pixel for pixel and 6 survive but render blank because a second, Huffman refinement/aggregate symbol dictionary fails after the MMR bitmap decoded (StartDecodereturnskError). pypdfium2 renders those 6; that is a separate limitation of the vendored decoder's Huffman refinement path, not touched here. Panels, the per-file table and the corpus scan are on the evidence branch.I have not opened a dataset pull request. The
third_party/jbig2files are BSD-2-Clause if you wantbitmap-mmr.pdfandbitmap-halftone-10bpp-mmr.pdfin the regression dataset.Ground truth changes:
None. The changed function is only reachable from MMR-coded JBIG2 segments, which no regression page contains.
Checklist:
main, no merge commitsLocal checks:
tests/test_unit_*.py381 passed, 1 skipped; ruff and mypy clean.🤖 Generated with Claude Code