fix: reopen object stream reader when header parse overshoots a non-first revision - #411
Open
tru-cooper wants to merge 1 commit into
Open
fix: reopen object stream reader when header parse overshoots a non-first revision#411tru-cooper wants to merge 1 commit into
tru-cooper wants to merge 1 commit into
Conversation
…irst revision ParseExistingInDirectStreamObject parses an object stream's header through mObjectParser's generic tokenizer, which does its own internal buffered reads and can pull more raw bytes from the underlying stream than the header's logical size requires. skipperStream's GetCurrentPosition()/mAmountRead tracks every byte physically read, not how much the tokenizer logically consumed, so its reported position can end up past the byte offset of the object actually being requested for some later revision. When that happens, the existing code called SkipTo() on the same already-open stream without checking it could still reach the target. SkipTo() can only move forward, so it silently no-oped instead of seeking, and the parser went on to misparse unrelated bytes as the requested object - surfacing as e.g. PDFWriter failing to resolve an indirect /Length pointing at a non-first object in a shared /ObjStm while merging or embedding a page. Fix: check CanSkipTo(target) before trusting SkipTo() to reach it. When the target is still reachable this costs nothing extra - the common case, and the one the original "should already be there" optimization covers. Only when the header parse's overshoot has actually carried us past the target do we reopen a fresh reader (whose position is a known 0, so the seek is then guaranteed reachable) instead of silently misreading. Adds ObjectStreamNonFirstIndexLengthTest, appending each page of a minimal hand-built 2-page fixture through its own fresh PDFWriter so the first-ever access to the shared object stream is for the non-first revision. Confirmed it fails identically without the fix and passes with it; full existing suite (104 tests) still passes.
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.
Problem
PDFParser::ParseExistingInDirectStreamObjectparses an object stream's header throughmObjectParser's generic tokenizer, which does its own internal buffered reads and can pull more raw bytes from the underlying stream than the header's logical size requires.skipperStream'sGetCurrentPosition()/mAmountReadtracks every byte physically read, not how much the tokenizer logically consumed, so its reported position can end up past the byte offset of the object actually being requested — for whichever revision happens to be the first one resolved from that stream.When that happens, the existing code called
SkipTo()on the same already-open stream without checking it could still reach the target.SkipTo()can only move forward, so it silently no-oped instead of seeking, and the parser went on to misparse unrelated bytes as the requested object.In practice this surfaced as
PDFWriterfailing to resolve an indirect/Lengthpointing at a non-first object in a shared/ObjStm, throwingunable to append to page, make sure source file existswhile merging or embedding a page — but it can affect any object-stream lookup where this overshoot happens to land past the target.Fix
Check
CanSkipTo(target)before trustingSkipTo()to reach it. When the target is still reachable — the common case, and the one the original "should already be there" optimization was written for — this costs nothing extra. Only when the header parse's overshoot has actually carried us past the target do we reopen a fresh reader (whose position is a known 0, so the seek is then guaranteed reachable) instead of silently misreading.Testing
Added
ObjectStreamNonFirstIndexLengthTest, using a minimal, hand-built 2-page PDF (no external tooling involved in producing it) where page 2's content-stream/Lengthis the second object packed into a shared object stream. Each page is appended through its own freshPDFWriter, since the bug only surfaces on the first-ever access to the shared stream at a non-zero revision — appending page 0 first would populate the per-stream header cache and mask it. Confirmed the test fails identically without the fix and passes with it. Full existing suite (104 tests) still passes.Context
Originally patched downstream in
muhammara(julianhille/MuhammaraJS#525); per @julianhille's note there, filing the actual fix here since MuhammaraJS just vendors this code.