Require BufRead + Seek bound for decoding (#558)

Author: fintelia

benches/decoder.rs

@@ -1,4 +1,4 @@
-use std::fs;
+use std::{fs, io::Cursor};
 
 use criterion::{
     criterion_group, criterion_main, measurement::WallTime, BenchmarkGroup, Criterion, Throughput,
@@ -95,8 +95,8 @@ fn bench_read_row(g: &mut BenchmarkGroup<WallTime>, data: Vec<u8>, name: &str) {
     });
 }
 
-fn create_reader(data: &[u8]) -> Reader<&[u8]> {
-    let mut decoder = Decoder::new(data);
+fn create_reader(data: &[u8]) -> Reader<Cursor<&[u8]>> {
+    let mut decoder = Decoder::new(Cursor::new(data));
 
     // Cover default transformations used by the `image` crate when constructing
     // `image::codecs::png::PngDecoder`.

examples/change-png-info.rs

@@ -1,7 +1,7 @@
 /// Tests "editing"/re-encoding of an image:
 /// decoding, editing, re-encoding
 use std::fs::File;
-use std::io::BufWriter;
+use std::io::{BufReader, BufWriter};
 use std::path::Path;
 pub type BoxResult<T> = Result<T, Box<dyn std::error::Error + Send + Sync>>;
 
@@ -11,7 +11,7 @@ fn main() -> BoxResult<()> {
     let path_in = Path::new(r"./tests/pngsuite/basi0g01.png");
     // The decoder is a build for reader and can be used to set various decoding options
     // via `Transformations`. The default output transformation is `Transformations::IDENTITY`.
-    let decoder = png::Decoder::new(File::open(path_in)?);
+    let decoder = png::Decoder::new(BufReader::new(File::open(path_in)?));
     let mut reader = decoder.read_info()?;
     // Allocate the output buffer.
     let png_info = reader.info();

examples/corpus-bench.rs

@@ -1,4 +1,4 @@
-use std::{fs, path::PathBuf};
+use std::{fs, io::Cursor, path::PathBuf};
 
 use clap::Parser;
 use png::Decoder;
@@ -62,7 +62,7 @@ fn run_encode(
 
 #[inline(never)]
 fn run_decode(image: &[u8], output: &mut [u8]) {
-    let mut reader = Decoder::new(image).read_info().unwrap();
+    let mut reader = Decoder::new(Cursor::new(image)).read_info().unwrap();
     reader.next_frame(output).unwrap();
 }
 
@@ -107,7 +107,7 @@ fn main() {
 
             // Parse
             let data = fs::read(entry.path()).unwrap();
-            let mut decoder = Decoder::new(&*data);
+            let mut decoder = Decoder::new(Cursor::new(&*data));
             if decoder.read_header_info().ok().map(|h| h.color_type)
                 == Some(png::ColorType::Indexed)
             {

fuzz/fuzz_targets/buf_independent.rs

@@ -26,41 +26,58 @@
 use libfuzzer_sys::fuzz_target;
 
 use std::fmt::Debug;
-use std::io::Read;
+use std::io::{BufRead, BufReader, Cursor, Seek};
 
 mod smal_buf {
+    use std::io::{BufRead, Cursor, Read, Seek};
 
-    use std::io::Read;
-
-    /// A reader that reads at most `n` bytes.
-    pub struct SmalBuf<R: Read> {
-        inner: R,
-        cap: usize,
+    /// A reader that returns at most 1 byte in a single call to `read`.
+    pub struct SmalBuf {
+        inner: Cursor<Vec<u8>>,
     }
 
-    impl<R: Read> SmalBuf<R> {
-        pub fn new(inner: R, cap: usize) -> Self {
-            SmalBuf { inner, cap }
+    impl SmalBuf {
+        pub fn new(inner: Vec<u8>) -> Self {
+            SmalBuf {
+                inner: Cursor::new(inner),
+            }
         }
     }
 
-    impl<R: Read> Read for SmalBuf<R> {
+    impl Read for SmalBuf {
         fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
-            let len = buf.len().min(self.cap);
-            self.inner.read(&mut buf[..len])
+            if buf.is_empty() {
+                return Ok(0);
+            }
+            self.inner.read(&mut buf[..1])
+        }
+    }
+    impl BufRead for SmalBuf {
+        fn fill_buf(&mut self) -> std::io::Result<&[u8]> {
+            let buf = self.inner.fill_buf()?;
+            Ok(&buf[..buf.len().min(1)])
+        }
+
+        fn consume(&mut self, amt: usize) {
+            self.inner.consume(amt);
+        }
+    }
+    impl Seek for SmalBuf {
+        fn seek(&mut self, pos: std::io::SeekFrom) -> std::io::Result<u64> {
+            self.inner.seek(pos)
         }
     }
 }
 
 mod intermittent_eofs {
 
     use std::cell::Cell;
-    use std::io::Read;
+    use std::io::{BufRead, Read, Seek};
     use std::rc::Rc;
 
     /// A reader that returns `std::io::ErrorKind::UnexpectedEof` errors in every other read.
     /// EOFs can be temporarily disabled and re-enabled later using the associated `EofController`.
-    pub struct IntermittentEofs<R: Read> {
+    pub struct IntermittentEofs<R: BufRead + Seek> {
         inner: R,
 
         /// Controls whether intermittent EOFs happen at all.
@@ -71,7 +88,7 @@ mod intermittent_eofs {
         eof_soon: bool,
     }
 
-    impl<R: Read> IntermittentEofs<R> {
+    impl<R: BufRead + Seek> IntermittentEofs<R> {
         pub fn new(inner: R) -> Self {
             Self {
                 inner,
@@ -85,7 +102,7 @@ mod intermittent_eofs {
         }
     }
 
-    impl<R: Read> Read for IntermittentEofs<R> {
+    impl<R: BufRead + Seek> Read for IntermittentEofs<R> {
         fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
             if self.controller.are_intermittent_eofs_enabled() && self.eof_soon {
                 self.eof_soon = false;
@@ -101,6 +118,20 @@ mod intermittent_eofs {
             inner_result
         }
     }
+    impl<R: BufRead + Seek> BufRead for IntermittentEofs<R> {
+        fn fill_buf(&mut self) -> std::io::Result<&[u8]> {
+            self.inner.fill_buf()
+        }
+
+        fn consume(&mut self, amt: usize) {
+            self.inner.consume(amt);
+        }
+    }
+    impl<R: BufRead + Seek> Seek for IntermittentEofs<R> {
+        fn seek(&mut self, pos: std::io::SeekFrom) -> std::io::Result<u64> {
+            self.inner.seek(pos)
+        }
+    }
 
     pub struct EofController {
         are_intermittent_eofs_enabled: Cell<bool>,
@@ -141,18 +172,24 @@ fuzz_target!(|data: &[u8]| {
     let _ = test_data(data);
 });
 
+trait BufReadSeek: BufRead + Seek {}
+impl<T> BufReadSeek for T where T: BufRead + Seek {}
+
 #[inline(always)]
 fn test_data<'a>(data: &'a [u8]) -> Result<(), ()> {
-    let baseline_reader = Box::new(data);
-    let byte_by_byte_reader = Box::new(smal_buf::SmalBuf::new(data, 1));
+    let baseline_reader = Box::new(Cursor::new(data));
+    let byte_by_byte_reader = Box::new(smal_buf::SmalBuf::new(data.to_owned()));
     let intermittent_eofs_reader = Box::new(intermittent_eofs::IntermittentEofs::new(
-        smal_buf::SmalBuf::new(data, 1),
+        smal_buf::SmalBuf::new(data.to_owned()),
     ));
     let intermittent_eofs_controller = intermittent_eofs_reader.controller();
-    let data_readers: Vec<Box<dyn Read + 'a>> = vec![
-        baseline_reader,
-        byte_by_byte_reader,
-        intermittent_eofs_reader,
+
+    // `Decoder` used to internally wrap the provided reader with a `BufReader`. Now that it has
+    // been removed, fuzzing would be far too slow if we didn't use a BufReader here.
+    let data_readers: Vec<BufReader<Box<dyn BufReadSeek>>> = vec![
+        BufReader::new(baseline_reader),
+        BufReader::new(byte_by_byte_reader),
+        BufReader::new(intermittent_eofs_reader),
     ];
 
     let decoders = data_readers
@@ -196,8 +233,14 @@ fn test_data<'a>(data: &'a [u8]) -> Result<(), ()> {
             .zip(buffers.iter_mut())
             .enumerate()
             .map(|(i, (png_reader, buffer))| {
-                let eof_controller = if i == 2 { Some(&intermittent_eofs_controller) } else { None };
-                retry_after_eofs(eof_controller, || png_reader.next_frame(buffer.as_mut_slice()))
+                let eof_controller = if i == 2 {
+                    Some(&intermittent_eofs_controller)
+                } else {
+                    None
+                };
+                retry_after_eofs(eof_controller, || {
+                    png_reader.next_frame(buffer.as_mut_slice())
+                })
             })
             .assert_all_results_are_consistent()
             .collect::<Result<Vec<_>, _>>()
@@ -222,7 +265,7 @@ fn retry_after_eofs<T>(
                         }
                     }
                 }
-            },
+            }
             _ => (),
         }
         break result;

fuzz/fuzz_targets/decode.rs

@@ -1,11 +1,12 @@
 #![no_main]
 
 use libfuzzer_sys::fuzz_target;
+use std::io::Cursor;
 
 #[inline(always)]
 fn png_decode(data: &[u8]) -> Result<(Option<png::OutputInfo>, Vec<u8>), ()> {
     let limits = png::Limits { bytes: 1 << 16 };
-    let decoder = png::Decoder::new_with_limits(data, limits);
+    let decoder = png::Decoder::new_with_limits(Cursor::new(data), limits);
     let  mut reader = decoder.read_info().map_err(|_| ())?;
 
     if reader.info().raw_bytes() > 5_000_000 {

fuzz/fuzz_targets/roundtrip.rs

@@ -1,6 +1,7 @@
 #![no_main]
 
 use libfuzzer_sys::fuzz_target;
+use std::io::Cursor;
 use png::{Filter, ColorType, BitDepth};
 
 fuzz_target!(|data: (u8, u8, u8, u8, u8, Vec<u8>, Vec<u8>)| {
@@ -63,7 +64,7 @@ fn encode_png<'a>(width: u8, filter: u8, compression: u8, color_type: u8, raw_bi
 }
 
 fn decode_png(data: &[u8]) -> (png::OutputInfo, Vec<u8>) {
-    let decoder = png::Decoder::new(data);
+    let decoder = png::Decoder::new(Cursor::new(data));
     let  mut reader = decoder.read_info().unwrap();
 
     let mut img_data = vec![0u8; reader.info().raw_bytes()];

src/decoder/mod.rs

@@ -10,7 +10,7 @@ use self::stream::{DecodeOptions, DecodingError, FormatErrorInner, CHUNK_BUFFER_
 use self::transform::{create_transform_fn, TransformFn};
 use self::unfiltering_buffer::UnfilteringBuffer;
 
-use std::io::Read;
+use std::io::{BufRead, Seek};
 use std::mem;
 
 use crate::adam7::{self, Adam7Info};
@@ -86,7 +86,7 @@ impl Default for Limits {
 }
 
 /// PNG Decoder
-pub struct Decoder<R: Read> {
+pub struct Decoder<R: BufRead + Seek> {
     read_decoder: ReadDecoder<R>,
     /// Output transformations
     transform: Transformations,
@@ -121,7 +121,7 @@ impl<'data> Row<'data> {
     }
 }
 
-impl<R: Read> Decoder<R> {
+impl<R: BufRead + Seek> Decoder<R> {
     /// Create a new decoder configuration with default limits.
     pub fn new(r: R) -> Decoder<R> {
         Decoder::new_with_limits(r, Limits::default())
@@ -159,17 +159,18 @@ impl<R: Read> Decoder<R> {
     ///
     /// ```
     /// use std::fs::File;
+    /// use std::io::BufReader;
     /// use png::{Decoder, Limits};
     /// // This image is 32×32, 1bit per pixel. The reader buffers one row which requires 4 bytes.
     /// let mut limits = Limits::default();
     /// limits.bytes = 3;
-    /// let mut decoder = Decoder::new_with_limits(File::open("tests/pngsuite/basi0g01.png").unwrap(), limits);
+    /// let mut decoder = Decoder::new_with_limits(BufReader::new(File::open("tests/pngsuite/basi0g01.png").unwrap()), limits);
     /// assert!(decoder.read_info().is_err());
     ///
     /// // This image is 32x32 pixels, so the decoder will allocate less than 10Kib
     /// let mut limits = Limits::default();
     /// limits.bytes = 10*1024;
-    /// let mut decoder = Decoder::new_with_limits(File::open("tests/pngsuite/basi0g01.png").unwrap(), limits);
+    /// let mut decoder = Decoder::new_with_limits(BufReader::new(File::open("tests/pngsuite/basi0g01.png").unwrap()), limits);
     /// assert!(decoder.read_info().is_ok());
     /// ```
     pub fn set_limits(&mut self, limits: Limits) {
@@ -248,8 +249,9 @@ impl<R: Read> Decoder<R> {
     /// eg.
     /// ```
     /// use std::fs::File;
+    /// use std::io::BufReader;
     /// use png::Decoder;
-    /// let mut decoder = Decoder::new(File::open("tests/pngsuite/basi0g01.png").unwrap());
+    /// let mut decoder = Decoder::new(BufReader::new(File::open("tests/pngsuite/basi0g01.png").unwrap()));
     /// decoder.set_ignore_text_chunk(true);
     /// assert!(decoder.read_info().is_ok());
     /// ```
@@ -262,8 +264,9 @@ impl<R: Read> Decoder<R> {
     /// eg.
     /// ```
     /// use std::fs::File;
+    /// use std::io::BufReader;
     /// use png::Decoder;
-    /// let mut decoder = Decoder::new(File::open("tests/iccp/broken_iccp.png").unwrap());
+    /// let mut decoder = Decoder::new(BufReader::new(File::open("tests/iccp/broken_iccp.png").unwrap()));
     /// decoder.set_ignore_iccp_chunk(true);
     /// assert!(decoder.read_info().is_ok());
     /// ```
@@ -281,7 +284,7 @@ impl<R: Read> Decoder<R> {
 /// PNG reader (mostly high-level interface)
 ///
 /// Provides a high level that iterates over lines or whole images.
-pub struct Reader<R: Read> {
+pub struct Reader<R: BufRead + Seek> {
     decoder: ReadDecoder<R>,
     bpp: BytesPerPixel,
     subframe: SubframeInfo,
@@ -317,7 +320,7 @@ struct SubframeInfo {
     consumed_and_flushed: bool,
 }
 
-impl<R: Read> Reader<R> {
+impl<R: BufRead + Seek> Reader<R> {
     /// Advances to the start of the next animation frame and
     /// returns a reference to the `FrameControl` info that describes it.
     /// Skips and discards the image data of the previous frame if necessary.

src/decoder/read_decoder.rs

@@ -1,9 +1,7 @@
-use super::stream::{
-    DecodeOptions, Decoded, DecodingError, FormatErrorInner, StreamingDecoder, CHUNK_BUFFER_SIZE,
-};
+use super::stream::{DecodeOptions, Decoded, DecodingError, FormatErrorInner, StreamingDecoder};
 use super::Limits;
 
-use std::io::{BufRead, BufReader, ErrorKind, Read};
+use std::io::{BufRead, ErrorKind, Read, Seek};
 
 use crate::chunk;
 use crate::common::Info;
@@ -18,14 +16,14 @@ use crate::common::Info;
 /// * `finish_decoding_image_data()` - discarding remaining data from `IDAT` / `fdAT` sequence
 /// * `read_until_end_of_input()` - reading until `IEND` chunk
 pub(crate) struct ReadDecoder<R: Read> {
-    reader: BufReader<R>,
+    reader: R,
     decoder: StreamingDecoder,
 }
 
-impl<R: Read> ReadDecoder<R> {
+impl<R: BufRead + Seek> ReadDecoder<R> {
     pub fn new(r: R) -> Self {
         Self {
-            reader: BufReader::with_capacity(CHUNK_BUFFER_SIZE, r),
+            reader: r,
             decoder: StreamingDecoder::new(),
         }
     }
@@ -34,10 +32,7 @@ impl<R: Read> ReadDecoder<R> {
         let mut decoder = StreamingDecoder::new_with_options(options);
         decoder.limits = Limits::default();
 
-        Self {
-            reader: BufReader::with_capacity(CHUNK_BUFFER_SIZE, r),
-            decoder,
-        }
+        Self { reader: r, decoder }
     }
 
     pub fn set_limits(&mut self, limits: Limits) {