Merge pull request #1696 from 5225225/fix-gif-oom

Fix gif OOM when decoding

src/codecs/gif.rs

@@ -43,22 +43,36 @@ use crate::error::{
     UnsupportedError, UnsupportedErrorKind,
 };
 use crate::image::{self, AnimationDecoder, ImageDecoder, ImageFormat};
+use crate::io::Limits;
 use crate::traits::Pixel;
 use crate::ImageBuffer;
 
 /// GIF decoder
 pub struct GifDecoder<R: Read> {
     reader: gif::Decoder<R>,
+    limits: Limits,
 }
 
 impl<R: Read> GifDecoder<R> {
-    /// Creates a new decoder that decodes the input steam ```r```
+    /// Creates a new decoder that decodes the input steam `r`
     pub fn new(r: R) -> ImageResult<GifDecoder<R>> {
         let mut decoder = gif::DecodeOptions::new();
         decoder.set_color_output(ColorOutput::RGBA);
 
         Ok(GifDecoder {
             reader: decoder.read_info(r).map_err(ImageError::from_decoding)?,
+            limits: Limits::default(),
+        })
+    }
+
+    /// Creates a new decoder that decodes the input steam `r`, using limits `limits`
+    pub fn with_limits(r: R, limits: Limits) -> ImageResult<GifDecoder<R>> {
+        let mut decoder = gif::DecodeOptions::new();
+        decoder.set_color_output(ColorOutput::RGBA);
+
+        Ok(GifDecoder {
+            reader: decoder.read_info(r).map_err(ImageError::from_decoding)?,
+            limits,
         })
     }
 }
@@ -154,7 +168,14 @@ impl<'a, R: 'a + Read> ImageDecoder<'a> for GifDecoder<R> {
         } else {
             // If the frame does not match the logical screen, read into an extra buffer
             // and 'insert' the frame from left/top to logical screen width/height.
-            let mut frame_buffer = vec![0; self.reader.buffer_size()];
+            let buffer_size = self.reader.buffer_size();
+
+            self.limits.reserve_usize(buffer_size)?;
+
+            let mut frame_buffer = vec![0; buffer_size];
+
+            self.limits.free_usize(buffer_size);
+
             self.reader
                 .read_into_buffer(&mut frame_buffer[..])
                 .map_err(ImageError::from_decoding)?;

src/io/mod.rs

@@ -1,5 +1,7 @@
 //! Input and output of images.
 
+use std::convert::TryFrom;
+
 use crate::{error, ImageError, ImageResult};
 
 pub(crate) mod free_functions;
@@ -122,13 +124,42 @@ impl Limits {
         Ok(())
     }
 
+    /// This function acts identically to [`reserve`], but takes a `usize` for convenience.
+    pub fn reserve_usize(&mut self, amount: usize) -> ImageResult<()> {
+        match u64::try_from(amount) {
+            Ok(n) => self.reserve(n),
+            Err(_) if self.max_alloc.is_some() => {
+                return Err(ImageError::Limits(error::LimitError::from_kind(
+                    error::LimitErrorKind::InsufficientMemory,
+                )));
+            }
+            Err(_) => {
+                // Out of bounds, but we weren't asked to consider any limit.
+                Ok(())
+            }
+        }
+    }
+
     /// This function increases the `max_alloc` limit with amount. Should only be used
-    /// togheter with [`reserve`].
+    /// together with [`reserve`].
     ///
     /// [`reserve`]: #method.reserve
     pub fn free(&mut self, amount: u64) {
         if let Some(max_alloc) = self.max_alloc.as_mut() {
             *max_alloc = max_alloc.saturating_add(amount);
         }
     }
+
+    /// This function acts identically to [`free`], but takes a `usize` for convenience.
+    pub fn free_usize(&mut self, amount: usize) {
+        match u64::try_from(amount) {
+            Ok(n) => self.free(n),
+            Err(_) if self.max_alloc.is_some() => {
+                panic!("max_alloc is set, we should have exited earlier when the reserve failed");
+            }
+            Err(_) => {
+                // Out of bounds, but we weren't asked to consider any limit.
+            }
+        }
+    }
 }

tests/regression.rs

@@ -59,3 +59,23 @@ fn bad_bmps() {
         assert!(im.is_err());
     }
 }
+
+#[test]
+fn bad_gif_oom() {
+    let data = [
+        71, 73, 70, 56, 55, 97, 0, 0, 0, 0, 0, 0, 0, 44, 255, 255, 219, 255, 172, 199, 199, 255,
+        216, 255, 255, 0, 0, 48, 230, 0, 195, 195, 195, 195, 255, 239, 0,
+    ];
+
+    // The original code made a vec![0; very_large_number] which due to overcommit *does not* OOM.
+    // It then exits normally with an EOF when reading.
+    //
+    // So instead we look for a limits error (or an unsupported error, for the case that we're
+    // running these tests without gif being actually supported)
+    let error = image::load_from_memory(&data).unwrap_err();
+
+    assert!(
+        matches!(error, image::ImageError::Limits(_))
+            | matches!(error, image::ImageError::Unsupported(_))
+    );
+}