From 43337c7fd5f7b5123efcebcc84de35c394726e7a Mon Sep 17 00:00:00 2001 From: Hamza Date: Thu, 29 Oct 2026 19:06:11 +0500 Subject: [PATCH] Drop the dead operand-overflow branch rather than revive it Review is right that `Stack::push` always returned `Some(())`, so `overflowed` never became true and the whole-instruction discard was dead code. Making it live breaks the page. `an_operator_with_hundreds_of_operands_does_ not_end_the_content` (#4260) puts 300 stray operands before `0 1 0 rg` and asserts the `rg` still reads its own three: with the discard alive the fill stays the previous red, measured as `left: [255, 0, 0]` against `right: [0, 255, 0]`. The ring buffer already gives the right answer. It keeps the newest operands, which are the ones nearest the operator -- its own -- and no operator takes anywhere near 256 of them: `scn` takes one per colourant and ISO 32000 allows a DeviceN space 32. So the strays fall off the back and the operator is applied to its own operands, which is what Acrobat and Chromium do. Dropping the instruction would lose content that is there to be read. The dead branch and the flag are gone, `push` no longer returns a result it cannot produce, and a test pins what the buffer does: the strays are `1` and the operator's own operand is `0`, so the two answers differ. Keeping the oldest operands instead fails it at 256 strays. --- crates/render/src/raster/tests.rs | 34 +++++++++++++++++++++-- vendor/hayro-syntax/src/content/mod.rs | 38 ++++++++------------------ 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/crates/render/src/raster/tests.rs b/crates/render/src/raster/tests.rs index 696eaddb61..39a489bb1d 100644 --- a/crates/render/src/raster/tests.rs +++ b/crates/render/src/raster/tests.rs @@ -7058,8 +7058,9 @@ fn a_malformed_token_does_not_end_the_content_stream() { // A `re` with three operands: the typed iterator used to abort on a path operator it // could not dispatch, which is what #3568 reports. "20 10 10 re", - // More operands than the limit (ISO 32000-2 Annex C gives 10): the instruction is - // dropped, not the stream. + // Eleven operands: under the 256 the stack holds, so this one runs on its own last + // operand. Past the limit is covered by + // `an_operator_past_the_operand_limit_reads_its_own_operands` below. "0 0 0 0 0 0 0 0 0 0 0 g", // A stray delimiter: the lexer could not read an operator and jumped to the end. ")", @@ -7080,6 +7081,35 @@ fn a_malformed_token_does_not_end_the_content_stream() { } } +/// An operator preceded by more operands than the stack holds is applied to its own operands, +/// which are the ones nearest it, rather than to whichever of the strays survived. The parser +/// had a branch meant to drop such an instruction whole, dead because `Stack::push` always +/// reported success (noted in review of #3636); reviving it loses legitimate content, which +/// `an_operator_with_hundreds_of_operands_does_not_end_the_content` measures, so the dead branch +/// is gone and this pins what the ring buffer does instead. +#[test] +fn an_operator_past_the_operand_limit_reads_its_own_operands() { + // The strays are `1` and the operator's own operand is `0`, so the two answers differ: white + // if the operator read a stray, black if it read its own. + let page = |strays: usize| { + let stream = format!("{}0 g 0 0 20 20 re f", "1 ".repeat(strays)); + let pdf = format!( + "%PDF-1.7\n1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj\n2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj\n3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 20 20] /Contents 4 0 R >> endobj\n4 0 obj << /Length {} >> stream\n{stream}\nendstream endobj\ntrailer << /Root 1 0 R >>\n%%EOF", + stream.len() + ); + let mut r = PageRenderer::new(Arc::new(pdf.into_bytes()), RenderConfig::default()); + let p = r.render(RenderRequest { page: 0, kind: RequestKind::Pixels, scale: 1.0, ..Default::default() }); + assert!(p.error.is_none(), "{strays} strays: {:?}", p.error); + p.rgba.as_chunks::<4>().0.to_vec() + }; + let all_black = |pixels: &[[u8; 4]]| pixels.iter().all(|p| *p == [0, 0, 0, 255]); + + // None, a few, the limit, and past it: the fill is the black the `g` asked for every time. + for strays in [0, 5, 255, 256, 257, 600] { + assert!(all_black(&page(strays)), "{strays} stray operands: the `g` reads its own operand"); + } +} + /// Content after an inline image whose data holds `EI` is still read. This pins the property /// but does **not** exercise the look-ahead gating added for #3636's review: accepting a false /// `EI` truncates the image, while the text after the real one is read either way, and this diff --git a/vendor/hayro-syntax/src/content/mod.rs b/vendor/hayro-syntax/src/content/mod.rs index 33c0e5bd05..1d71d73d2e 100644 --- a/vendor/hayro-syntax/src/content/mod.rs +++ b/vendor/hayro-syntax/src/content/mod.rs @@ -190,11 +190,6 @@ impl<'a> UntypedIter<'a> { self.committed = self.skip_blanks_and_comments(); } - // PdfCraft patch: set when this instruction carried more operands than the limit. Its - // operator is then dropped with them, rather than applied to the ones that did fit - // (#3568). - let mut overflowed = false; - while !self.reader.at_end() { // I believe booleans/null never appear as an operator? if matches!( @@ -212,9 +207,7 @@ impl<'a> UntypedIter<'a> { if self.undecided() { return None; } - if self.stack.push(object).is_none() { - overflowed = true; - } + self.stack.push(object); } else if self.reader.read_without_context::>().is_some() { if self.undecided() { return None; @@ -234,7 +227,6 @@ impl<'a> UntypedIter<'a> { // content was deliberately not parsed. warn!("skipped an unreadable name in content stream"); self.stack.clear(); - overflowed = false; continue; } else { return None; @@ -253,7 +245,6 @@ impl<'a> UntypedIter<'a> { // the rest of the page was lost. Step over it and carry on (#3568). if !self.lookahead && self.skip_bad_token() { self.stack.clear(); - overflowed = false; continue; } @@ -302,7 +293,7 @@ impl<'a> UntypedIter<'a> { UnfilteredEnd::Found(end_data, end_after_ei) => { let image_data = stream_data.get(..end_data)?; let stream = Stream::new(image_data, dict.clone()); - self.stack.push(Object::Stream(stream))?; + self.stack.push(Object::Stream(stream)); self.reader.read_bytes(end_after_ei)?; self.reader.skip_white_spaces(); accepted = true; @@ -464,7 +455,7 @@ impl<'a> UntypedIter<'a> { return None; } - self.stack.push(Object::Stream(stream))?; + self.stack.push(Object::Stream(stream)); self.reader.read_bytes(2)?; self.reader.skip_white_spaces(); @@ -482,17 +473,6 @@ impl<'a> UntypedIter<'a> { } } - if overflowed && !self.lookahead { - // PdfCraft patch: see `overflowed` above. ISO 32000-2 Annex C gives 10 as the - // historical operand limit; past it the instruction is dropped whole. - warn!("dropped an instruction with more operands than the limit"); - self.stack.clear(); - overflowed = false; - self.committed = self.reader.offset(); - self.reader.skip_white_spaces_and_comments(); - continue; - } - self.operator = Some(operator); self.committed = self.reader.offset(); // PdfCraft patch (28): operands in reading order, if some were dropped. @@ -717,11 +697,16 @@ impl<'a> Stack<'a> { } /// PdfCraft patch (28): push an operand, dropping the oldest once `MAX_OPERANDS` are held - /// (see `MAX_OPERANDS`). Never fails: it used to, and the failure ended the content stream. - fn push(&mut self, operand: Object<'a>) -> Option<()> { + /// (see `MAX_OPERANDS`). It cannot fail: it used to, and the failure ended the content stream. + /// + /// The operands kept are the newest, so the ones nearest the operator -- which are its own. + /// No operator takes anywhere near `MAX_OPERANDS` of them (`scn` takes one per colourant, and + /// ISO 32000 allows a DeviceN space 32), so an instruction preceded by stray operands is + /// applied to its own and the strays fall off the back. + fn push(&mut self, operand: Object<'a>) { if self.data.len() < MAX_OPERANDS { self.data.push(operand); - return Some(()); + return; } self.dropped = true; @@ -729,7 +714,6 @@ impl<'a> Stack<'a> { *slot = operand; } self.oldest = (self.oldest + 1) % MAX_OPERANDS; - Some(()) } /// PdfCraft patch (28): put the operands back in the order they were read, oldest first,