Prevent PDF uploads from freezing on parser panics#653
Open
AnthonyRonning wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR pins ChangesPDF extraction pipeline
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant extract_document_content
participant run_pdf_extraction
participant spawn_blocking
participant pdf_extract
extract_document_content->>run_pdf_extraction: submit PDF extraction closure
run_pdf_extraction->>spawn_blocking: execute blocking parser task
spawn_blocking->>pdf_extract: call extract_text_from_mem
pdf_extract-->>spawn_blocking: extracted text or extraction error
spawn_blocking-->>run_pdf_extraction: task result or panic/cancellation
run_pdf_extraction-->>extract_document_content: standardized extraction result
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Deploying maple with
|
| Latest commit: |
8a9cfe2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5ab906fc.maple-ca8.pages.dev |
| Branch Preview URL: | https://codex-maple-pdf-panic-recove.maple-ca8.pages.dev |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src-tauri/patches/pdf-extract-0.12.0/src/glyphlist-extended.txt`:
- Line 4552: Update the glyph-list generation flow in glyphlist-export.py so the
corrected Germandbls mapping at the later entry is not discarded by the earlier
duplicate; replace the original mapping or add an explicit override, then
regenerate glyphnames.rs to include the corrected value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2601c926-a004-4477-9725-e5145740cec0
⛔ Files ignored due to path filters (20)
frontend/src-tauri/Cargo.lockis excluded by!**/*.lockfrontend/src-tauri/patches/pdf-extract-0.12.0/Cargo.lockis excluded by!**/*.lockfrontend/src-tauri/tests/fixtures/absent-cff-charset-glyph.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/declared-encoding-overrides-builtin.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/differences-only-embedded-cff.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/embedded-non-zapf-a1.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/finite-form-reentry.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/fontawesome-tounicode-occupied.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/invalid.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/no-encoding-missing-slot-cff.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/non-zapf-differences.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/notdef-difference-embedded-cff.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/page-font-name-collision.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/recursive-form-xobject.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/symbol-differences.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/type1-encoding-code-300.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/type4-separation.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/xobject-depth-101.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/xobject-font-name-collision.pdfis excluded by!**/*.pdffrontend/src-tauri/tests/fixtures/zapf-dingbats-differences.pdfis excluded by!**/*.pdf
📒 Files selected for processing (14)
.gitattributesfrontend/src-tauri/Cargo.tomlfrontend/src-tauri/patches/pdf-extract-0.12.0/Cargo.tomlfrontend/src-tauri/patches/pdf-extract-0.12.0/Cargo.toml.origfrontend/src-tauri/patches/pdf-extract-0.12.0/MAPLE_PATCHES.mdfrontend/src-tauri/patches/pdf-extract-0.12.0/README.mdfrontend/src-tauri/patches/pdf-extract-0.12.0/src/core_fonts.rsfrontend/src-tauri/patches/pdf-extract-0.12.0/src/encodings.rsfrontend/src-tauri/patches/pdf-extract-0.12.0/src/glyphlist-export.pyfrontend/src-tauri/patches/pdf-extract-0.12.0/src/glyphlist-extended.txtfrontend/src-tauri/patches/pdf-extract-0.12.0/src/glyphnames.rsfrontend/src-tauri/patches/pdf-extract-0.12.0/src/lib.rsfrontend/src-tauri/patches/pdf-extract-0.12.0/src/zapfglyphnames.rsfrontend/src-tauri/src/pdf_extractor.rs
| d;0064 | ||
| e;0065 | ||
| f;0066 | ||
| Germandbls;0053 0053 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the Germandbls correction effective.
This duplicate is ignored because glyphlist-export.py keeps the Line 4314 entry and skips later names. Replace the earlier mapping or add an explicit override step, then regenerate glyphnames.rs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src-tauri/patches/pdf-extract-0.12.0/src/glyphlist-extended.txt` at
line 4552, Update the glyph-list generation flow in glyphlist-export.py so the
corrected Germandbls mapping at the later entry is not discarded by the earlier
duplicate; replace the original mapping or add an explicit override, then
regenerate glyphnames.rs to include the corrected value.
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.
Related to #571
Summary
pdf-extractfrom 0.7.12 to 0.12.0 andlopdfto 0.42.0, fixing the reported crash on valid PDFs containing Type 4 calculator functions.pdf-extract0.12.0 source with four documented Maple corrections:.notdef, absent glyphs, explicit/ToUnicode, and out-of-range Type1 codesUser impact
PDFs containing Type 4 functions can now be processed without freezing Maple. If
pdf-extractreturns an error or triggers an ordinary Rust unwind panic on another PDF edge case, Maple shows the existingFailed to process documentmessage and immediately re-enables the attachment picker. Recursive Form XObjects are rejected before they can overflow the native stack.Dependency and review notes
The vendored source starts from the published 0.12.0 crate, with every Maple-specific change documented in
MAPLE_PATCHES.md. Moving tolopdf0.42 also removes its known crafted-PDF stack-overflow advisory. The remainingttf-parserRustSec notice is informational and has no patched release.The recovery boundary catches ordinary unwind panics; process aborts, OOM, native faults, and parser hangs remain outside an in-process panic boundary. Native stack overflow is likewise not catchable, so the XObject traversal now prevents the demonstrated cycle/depth cases before overflow.
Independent reviews approached the diff from runtime safety, dependency/upstream parity, PDF specification and font-encoding correctness, fixture validity, and final-diff perspectives. Review findings were reproduced before changes were accepted, and the final reviewers reported no remaining major or critical issue. Broader pre-existing custom-CFF completeness limitations and vendored warning cleanup were intentionally left out because they would require unrelated complexity.
Validation
nix develop -c ./scripts/ci/frontend.sh- 131 passed; lint completed with 12 pre-existing warningsnix develop -c ./scripts/ci/rust.sh- 142 passedqpdf --checkandpdfinfo/ToUnicode, missing glyphs, recursive/deep XObjects, and finite re-entryTAURI_SIGNING_PRIVATE_KEYFailed to process documentwithout terminating the appgit diff --checkSummary by CodeRabbit
Bug Fixes
Tests