Merge pull request #83 from image-rs/retry-oob-check

Revisit the frame oob check

src/reader/decoder.rs

@@ -1,16 +1,15 @@
 use std::cmp;
+use std::error;
+use std::fmt;
+use std::io;
 use std::mem;
 use std::default::Default;
 
-use std::io;
-
-use std::fmt;
-use std::error;
+use crate::common::{AnyExtension, Block, DisposalMethod, Extension, Frame};
+use crate::reader::DecodeOptions;
 
 use weezl::{BitOrder, decode::Decoder as LzwDecoder, LzwStatus};
 
-use crate::common::{AnyExtension, Block, DisposalMethod, Extension, Frame};
-
 /// GIF palettes are RGB
 pub const PLTE_CHANNELS: usize = 3;
 
@@ -192,6 +191,7 @@ pub struct StreamingDecoder {
     lzw_reader: Option<LzwDecoder>,
     decode_buffer: Vec<u8>,
     skip_extensions: bool,
+    check_frame_consistency: bool,
     version: &'static str,
     width: u16,
     height: u16,
@@ -206,11 +206,17 @@ pub struct StreamingDecoder {
 impl StreamingDecoder {
     /// Creates a new streaming decoder
     pub fn new() -> StreamingDecoder {
+        let options = DecodeOptions::new();
+        Self::with_options(&options)
+    }
+
+    pub(crate) fn with_options(options: &DecodeOptions) -> Self {
         StreamingDecoder {
             state: Some(Magic(0, [0; 6])),
             lzw_reader: None,
             decode_buffer: vec![],
             skip_extensions: true,
+            check_frame_consistency: options.check_frame_consistency,
             version: "",
             width: 0,
             height: 0,
@@ -431,7 +437,7 @@ impl StreamingDecoder {
 
                         self.current_frame_mut().interlaced = interlaced;
 
-                        {
+                        if self.check_frame_consistency {
                             // Consistency checks.
                             let (width, height) = (self.width, self.height);
                             let frame = self.current_frame_mut();

src/reader/mod.rs

@@ -28,24 +28,43 @@ pub enum ColorOutput {
     Indexed = 1,
 }
 
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 /// Memory limit in bytes. `MemoryLimit::Some(0)` means
 /// that there is no memory limit set.
 pub struct MemoryLimit(pub u32);
 
+impl MemoryLimit {
+    fn buffer_size(&self, color: ColorOutput, width: u16, height: u16) -> Option<usize> {
+        let pixels = u32::from(width) * u32::from(height);
+
+        let bytes_per_pixel = match color {
+            ColorOutput::Indexed => 1,
+            ColorOutput::RGBA => 4,
+        };
+
+        if self.0 > 0 && pixels > self.0 / bytes_per_pixel {
+            None
+        } else {
+            Some(pixels as usize * bytes_per_pixel as usize)
+        }
+    }
+}
+
 /// Options for opening a GIF decoder.
 #[derive(Clone, Debug)]
 pub struct DecodeOptions {
-    memory_limit: u32,
+    memory_limit: MemoryLimit,
     color_output: ColorOutput,
+    check_frame_consistency: bool,
 }
 
 impl DecodeOptions {
     /// Creates a new decoder builder
     pub fn new() -> DecodeOptions {
         DecodeOptions {
-            memory_limit: 50_000_000, // 50 MB
+            memory_limit: MemoryLimit(50_000_000), // 50 MB
             color_output: ColorOutput::Indexed,
+            check_frame_consistency: false,
         }
     }
 
@@ -55,15 +74,29 @@ impl DecodeOptions {
     }
 
     /// Configure a memory limit for decoding.
-    pub fn set_memory_limit(&mut self, MemoryLimit(limit): MemoryLimit) {
+    pub fn set_memory_limit(&mut self, limit: MemoryLimit) {
         self.memory_limit = limit;
     }
-
+
+    /// Configure if frames must be within the screen descriptor.
+    ///
+    /// The default is `false`.
+    ///
+    /// When turned on, all frame descriptors being read must fit within the screen descriptor or
+    /// otherwise an error is returned and the stream left in an unspecified state.
+    ///
+    /// When turned off, frames may be arbitrarily larger or offset in relation to the screen. Many
+    /// other decoder libraries handle this in highly divergent ways. This moves all checks to the
+    /// caller, for example to emulate a specific style.
+    pub fn check_frame_consistency(&mut self, check: bool) {
+        self.check_frame_consistency = check;
+    }
+
     /// Reads the logical screen descriptor including the global color palette
     ///
     /// Returns a `Decoder`. All decoder configuration has to be done beforehand.
     pub fn read_info<R: Read>(self, r: R) -> Result<Decoder<R>, DecodingError> {
-        Decoder::with_no_init(r, StreamingDecoder::new(), self.color_output, self.memory_limit).init()
+        Decoder::with_no_init(r, StreamingDecoder::with_options(&self), self).init()
     }
 }
 
@@ -106,7 +139,7 @@ impl<R: Read> ReadDecoder<R> {
 pub struct Decoder<R: Read> {
     decoder: ReadDecoder<R>,
     color_output: ColorOutput,
-    memory_limit: u32,
+    memory_limit: MemoryLimit,
     bg_color: Option<u8>,
     global_palette: Option<Vec<u8>>,
     current_frame: Frame<'static>,
@@ -124,20 +157,18 @@ impl<R> Decoder<R> where R: Read {
         DecodeOptions::new()
     }
 
-    fn with_no_init(reader: R, decoder: StreamingDecoder,
-           color_output: ColorOutput, memory_limit: u32
-    ) -> Decoder<R> {
+    fn with_no_init(reader: R, decoder: StreamingDecoder, options: DecodeOptions) -> Decoder<R> {
         Decoder {
             decoder: ReadDecoder {
                 reader: io::BufReader::new(reader),
-                decoder: decoder,
+                decoder,
                 at_eof: false
             },
             bg_color: None,
             global_palette: None,
             buffer: Vec::with_capacity(32),
-            color_output: color_output,
-            memory_limit: memory_limit,
+            color_output: options.color_output,
+            memory_limit: options.memory_limit,
             current_frame: Frame::default(),
         }
     }
@@ -186,14 +217,6 @@ impl<R> Decoder<R> where R: Read {
                             "no color table available for current frame"
                         ))
                     }
-                    if self.memory_limit > 0  && (
-                        (frame.width as u32 * frame.height as u32)
-                        > self.memory_limit
-                    ) {
-                        return Err(DecodingError::format(
-                            "image is too large to decode"
-                        ))
-                    }
                     break
                 },
                 Some(_) => (),
@@ -209,8 +232,20 @@ impl<R> Decoder<R> where R: Read {
     /// Do not call `Self::next_frame_info` beforehand.
     /// Deinterlaces the result.
     pub fn read_next_frame(&mut self) -> Result<Option<&Frame<'static>>, DecodingError> {
-        if self.next_frame_info()?.is_some() {
-            let mut vec = vec![0; self.buffer_size()];
+        if let Some(frame) = self.next_frame_info()? {
+            let (width, height) = (frame.width, frame.height);
+            let pixel_bytes = self.memory_limit
+                .buffer_size(self.color_output, width, height)
+                .ok_or_else(|| {
+                    DecodingError::format("image is too large to decode")
+                })?;
+
+            debug_assert_eq!(
+                pixel_bytes, self.buffer_size(),
+                "Checked computation diverges from required buffer size"
+            );
+
+            let mut vec = vec![0; pixel_bytes];
             self.read_into_buffer(&mut vec)?;
             self.current_frame.buffer = Cow::Owned(vec);
             self.current_frame.interlaced = false;

tests/decode.rs

@@ -0,0 +1,49 @@
+use gif::{DecodeOptions, DisposalMethod, Encoder, Frame};
+
+#[test]
+fn frame_consistency_is_configurable() {
+    let image = create_image_with_oob_frames();
+    let mut options = DecodeOptions::new();
+
+    {
+        let mut data = image.as_slice();
+        let mut decoder = options.clone().read_info(&mut data).unwrap();
+        assert!(decoder.read_next_frame().is_ok());
+        assert!(decoder.read_next_frame().is_err());
+    }
+
+    {
+        options.check_frame_consistency(false);
+        let mut data = image.as_slice();
+        let mut decoder = options.clone().read_info(&mut data).unwrap();
+        assert!(decoder.read_next_frame().is_ok());
+        assert!(decoder.read_next_frame().is_ok());
+    }
+}
+
+fn create_image_with_oob_frames() -> Vec<u8> {
+    let mut data = vec![];
+    let mut encoder = Encoder::new(&mut data, 2, 2, &[0, 0, 0]).unwrap();
+
+    let mut frame = Frame {
+        delay: 1,
+        dispose: DisposalMethod::Any,
+        transparent: None,
+        needs_user_input: false,
+        top: 0,
+        left: 0,
+        width: 2,
+        height: 2,
+        interlaced: false,
+        palette: None,
+        buffer: vec![0, 0, 0, 0].into(),
+    };
+
+    encoder.write_frame(&frame).unwrap();
+    frame.top = 1;
+    frame.left = 1;
+    encoder.write_frame(&frame).unwrap();
+
+    drop(encoder);
+    data
+}