aleleba/pdfcraft
1
0
Fork 0
mirror of https://github.com/storytold/pdfcraft.git synced 2026-11-01 05:58:12 -06:00
Code Issues Packages Projects Releases Wiki Activity

Drop the dead operand-overflow branch rather than revive it

Browse Source 
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.
This commit is contained in:
Hamza
2026-10-29 19:06:11 +05:00
parent 30ed249a28
commit 43337c7fd5
2 changed files with 43 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/render/src/raster/tests.rs
+32 -2
View File
@@ -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
vendor/hayro-syntax/src/content/mod.rs
Vendored
+11 -27
View File
@@ -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::<Operator<'_>>().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,