From b41b4bbe614c8f028ad0d5fc0db098730cbff226 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Sun, 17 Apr 2022 01:48:11 +0100 Subject: [PATCH 1/2] Fix OOM when decoding gif with very large buffer --- src/codecs/gif.rs | 28 ++++++++++++++++++++++++++-- src/io/mod.rs | 20 +++++++++++++++++++- tests/regression.rs | 20 ++++++++++++++++++++ 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/codecs/gif.rs b/src/codecs/gif.rs index 70b748054c..748a445ccf 100644 --- a/src/codecs/gif.rs +++ b/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 { reader: gif::Decoder, + limits: Limits, } impl GifDecoder { - /// 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> { 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> { + 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,17 @@ impl<'a, R: 'a + Read> ImageDecoder<'a> for GifDecoder { } 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( + u64::try_from(buffer_size) + .expect("if buffer_size overflows a usize, we should have returned already"), + ); + self.reader .read_into_buffer(&mut frame_buffer[..]) .map_err(ImageError::from_decoding)?; diff --git a/src/io/mod.rs b/src/io/mod.rs index 71f20a0258..ff72f8a72d 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -122,8 +122,26 @@ impl Limits { Ok(()) } + /// This function acts identically to [`reserve`], but takes a `usize` for convenience. + pub fn reserve_usize(&mut self, amount: usize) -> ImageResult<()> { + use std::convert::TryFrom; + + 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) { diff --git a/tests/regression.rs b/tests/regression.rs index 2710185c27..5ae0871b38 100644 --- a/tests/regression.rs +++ b/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 bmp being actually supported) + let error = image::load_from_memory(&data).unwrap_err(); + + assert!( + matches!(error, image::ImageError::Limits(_)) + | matches!(error, image::ImageError::Unsupported(_)) + ); +} From 95b53b91c3de33417da5fbc8a8992cc8bcbacee8 Mon Sep 17 00:00:00 2001 From: 5225225 <5225225@mailbox.org> Date: Mon, 18 Apr 2022 18:22:01 +0100 Subject: [PATCH 2/2] Use free_usize instead of a u64::try_from in gif.rs --- src/codecs/gif.rs | 5 +---- src/io/mod.rs | 17 +++++++++++++++-- tests/regression.rs | 2 +- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/codecs/gif.rs b/src/codecs/gif.rs index 748a445ccf..593c490ba9 100644 --- a/src/codecs/gif.rs +++ b/src/codecs/gif.rs @@ -174,10 +174,7 @@ impl<'a, R: 'a + Read> ImageDecoder<'a> for GifDecoder { let mut frame_buffer = vec![0; buffer_size]; - self.limits.free( - u64::try_from(buffer_size) - .expect("if buffer_size overflows a usize, we should have returned already"), - ); + self.limits.free_usize(buffer_size); self.reader .read_into_buffer(&mut frame_buffer[..]) diff --git a/src/io/mod.rs b/src/io/mod.rs index ff72f8a72d..2cac99a47a 100644 --- a/src/io/mod.rs +++ b/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; @@ -124,8 +126,6 @@ impl Limits { /// This function acts identically to [`reserve`], but takes a `usize` for convenience. pub fn reserve_usize(&mut self, amount: usize) -> ImageResult<()> { - use std::convert::TryFrom; - match u64::try_from(amount) { Ok(n) => self.reserve(n), Err(_) if self.max_alloc.is_some() => { @@ -149,4 +149,17 @@ impl Limits { *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. + } + } + } } diff --git a/tests/regression.rs b/tests/regression.rs index 5ae0871b38..927447d7a2 100644 --- a/tests/regression.rs +++ b/tests/regression.rs @@ -71,7 +71,7 @@ fn bad_gif_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 bmp being actually supported) + // running these tests without gif being actually supported) let error = image::load_from_memory(&data).unwrap_err(); assert!(