Repository navigation
Conversation
Font::widths computes c1 + array.len() - 1 for the CID width table. Both operands come from the /W array in the PDF, so an empty sub-array makes len() - 1 wrap. 9d83215 added the c1 == usize::MAX guard five lines above, which stops the sum wrapping from the top; nothing stops len() - 1 wrapping from the bottom. In debug /W [0 []] panics with 'attempt to subtract with overflow'; in release the double wrap reaches ensure_cid as usize::MAX and reserve aborts with 'capacity overflow'. Widths::ensure_cid and Widths::get already use checked_sub for the same reason; do the same at the two call sites.
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.
Font::widthsbuilds the CID width table from the/Warray (pdf/src/font.rs:350, and again at:357for the indirect-reference arm):Both
c1andarraycome straight out of the CIDFont's/Wentry, so a document with an empty sub-array --/W [0 []]-- makesarray.len() - 1wrap.In release the double wrap lands on
usize::MAX,ensure_cid'schecked_sub(0)succeeds, andvalues.reserve(usize::MAX)aborts. To be precise about the release case: it needsc1 == 0. Forc1 >= 1the two wraps cancel toc1 - 1andensure_cidis a no-op, so release is benign there. Debug panics for anyc1.This is a hole in a guard you already wrote
9d83215("sanity checks") added thec1 == usize::MAXcheck five lines above, in the same loop -- so the sum cannot wrap from the top. Nothing coverslen() - 1wrapping from the bottom.And the callee already does it correctly:
Widths::ensure_cid(font.rs:234) usescid.checked_sub(self.first_char), andWidths::get(font.rs:218) guardscid < self.first_charbefore subtracting. Only the two call sites compute the argument unchecked. The fix mirrorsensure_cid's ownchecked_sub.Verification
cargo test --workspace --no-fail-fast-- 0 failed, matching the pre-existing count. CI (.github/workflows/test.yml) runs exactlycargo test --workspace; there is no fmt or clippy gate. I ran both profiles, since this is an overflow: debug panics on the subtraction, release on the allocation.Two tests added, one per call site. Mutation-checked separately, anchored to each match arm:
w_array_empty_underflowfails atfont.rs:350, the reference test still passesw_array_empty_underflow_via_referencefails, the direct test still passesEach mutant is killed by exactly one test, and the reported panic line confirms the mutation landed where I intended.
Behaviour change
A PDF with an empty
/Wsub-array now returnsOkinstead of panicking. No existing test row changes -- the suite has no occurrence ofensure_cid,usize::MAX, or any/Wboundary value.What I did not change
if! c1 == 0guard itself. It parses as!c1 == 0, i.e.c1 == usize::MAX, which is what it is meant to check -- rewriting it for readability would blur this fix.c1 + iinside the loop and thec1..=c2arm.Primitive::as_usizeonly accepts a non-negativePrimitive::Integer(i32), soc1 <= i32::MAXand those cannot wrap on a 64-bit target; I had no repro for them.SampledFunction::apply, which is issue Panic (multiply overflow) inSampledFunction::apply#289 and still open, andfont.rs:278'sself.values[cid - self.first_char], which is already dominated by thecid < self.first_charearly return above it.I noticed #284 (
ObjectStream::get_object_sliceoverflow) and #286 (PsFunc::exec_innerunderflow) are both closed and fixed, which is why I sent this as a plain PR rather than asking first.Disclosure: AI-assisted. I found and prepared this with an AI assistant, and I ran and verified everything above myself.