From 59043bd22c3b2dd54efe693d06f9b81b67e7fb46 Mon Sep 17 00:00:00 2001 From: Lukasz Anforowicz Date: Mon, 23 Sep 2024 04:15:56 +0000 Subject: [PATCH] Idempotent handling of missing `fdAT` chunk in a truncated PNG file. Fuzzing found a broken PNG file where the sequence of chunks looks more or less like this: `IHDR`, `acTL`, `IDAT`, `fcTL`, `fctL`, EOF. This commit preserves this repro file under `fuzz/corpus/buf_independent/regressions`. Before this commit such fuzzing input would lead to problems when resuming a call to `next_frame` after an interittent EOF. There were a few loosely related problems: 1. `fn read_until_image_data` would unnecessarily set `self.subframe` in the `Decoded::FrameControl` branch. This was unnecessary because `self.subframe` is also set after breaking out of the loop. 2. `fn read_until_image_data` is called to position the input stream at the beginning of the next image frame - it breaks out of its loop and returns after encountering the start of the next `IDAT` or `fdAT` chunk. This means that there is a discrepancy: - Whether we are in the middle of an `IDAT`/`fdAT` data stream can be determined by checking `self.subframe.consumed_and_flushed`. - But before this commit `fn next_frame` would decide whether to call `read_until_image_data` based on `self.next_frame`, rather than based on `self.subframe.consumed_and_flushed`. 3. After calling `fn finish_decoding` from `fn next_frame` the `self.subframe.consumed_and_flushed` wouldn't be reset. Problem2 above meant that after an intermittent `UnexpectedEof` in `read_until_image_data` retrying a call to `next_frame` would *not* retry `read_until_image_data` (because `self.next_frame` would be updated by the previous `read_until_image_data`, and now `fn next_frame` would incorrectly treat this as not needing to call `read_until_image_data`, even though we still haven't reached an `IDAT` nor `fdAT` chunk). This was fixed by changing how `fn next_frame` decides whether to call `read_until_image_data` (as a side-effect this allowed deleting `subframe_idx`). Fixing problem2 was insufficient, because problem1 meant that `consumed_and_flushed` was prematurely set (before actually encountering an `IDAT` or `fdAT` frame). And we also needed to fix problem3 to ensure that the next call to `next_frame` knows that it is time to advance to the next `IDAT` / `fdAT` chunks. --- ...-from-32e60253faddfdf03b267e0b55c7b91a674d74bd | Bin 0 -> 242 bytes src/decoder/mod.rs | 12 ++++-------- 2 files changed, 4 insertions(+), 8 deletions(-) create mode 100644 fuzz/corpus/buf_independent/regressions/minimized-from-32e60253faddfdf03b267e0b55c7b91a674d74bd diff --git a/fuzz/corpus/buf_independent/regressions/minimized-from-32e60253faddfdf03b267e0b55c7b91a674d74bd b/fuzz/corpus/buf_independent/regressions/minimized-from-32e60253faddfdf03b267e0b55c7b91a674d74bd new file mode 100644 index 0000000000000000000000000000000000000000..c59c1202ee12f1075005e14b4ad000d263d63ff4 GIT binary patch literal 242 zcmeAS@N?(olHy`uU~uvDa0y~yZv-+Kgc(7kcSv$t0|VC@AV`A(H89Q4iO|Li*2ciV zSOEbHjF-OE*8_EPBqoRW7&0(0GgL8{NOJLlOq5Ckih&qFPzA)yU>1nrVn|_N%wu`y z0AhQ(IEF-UCQGt1{AWl23V;DjFUV<3K+Fvh29rRsAP5(x3Zw$05?K|H8^izr1C}IJ literal 0 HcmV?d00001 diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index d6e85aea..4d8d2c01 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -415,7 +415,6 @@ impl Reader { Some(Decoded::ChunkBegin(_, chunk::IDAT)) | Some(Decoded::ChunkBegin(_, chunk::fdAT)) => break, Some(Decoded::FrameControl(_)) => { - self.subframe = SubframeInfo::new(self.info()); // The next frame is the one to which this chunk applies. self.next_frame = SubframeIdx::Some(self.fctl_read); // TODO: what about overflow here? That would imply there are more fctl chunks @@ -474,17 +473,13 @@ impl Reader { /// Output lines will be written in row-major, packed matrix with width and height of the read /// frame (or subframe), all samples are in big endian byte order where this matters. pub fn next_frame(&mut self, buf: &mut [u8]) -> Result { - let subframe_idx = match self.decoder.info().unwrap().frame_control() { - None => SubframeIdx::Initial, - Some(_) => SubframeIdx::Some(self.fctl_read - 1), - }; - if self.next_frame == SubframeIdx::End { return Err(DecodingError::Parameter( ParameterErrorKind::PolledAfterEndOfImage.into(), )); - } else if self.next_frame != subframe_idx { - // Advance until we've read the info / fcTL for this frame. + } else if self.subframe.consumed_and_flushed { + // Advance until the next `fdAT` + // (along the way we should encounter the fcTL for this frame). self.read_until_image_data()?; } @@ -539,6 +534,7 @@ impl Reader { // Advance over the rest of data for this (sub-)frame. if !self.subframe.consumed_and_flushed { self.decoder.finish_decoding()?; + self.subframe.consumed_and_flushed = true; } // Advance our state to expect the next frame.