Skip to content

Commit

Permalink
Fix OOM when decoding gif with very large buffer
Browse files Browse the repository at this point in the history
  • Loading branch information
5225225 committed Apr 17, 2022
1 parent ae1e661 commit cac78c2
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 3 deletions.
26 changes: 24 additions & 2 deletions src/codecs/gif.rs
Expand Up @@ -36,6 +36,7 @@ use gif::ColorOutput;
use gif::{DisposalMethod, Frame};
use num_rational::Ratio;

use crate::io::Limits;
use crate::animation;
use crate::color::{ColorType, Rgba};
use crate::error::{
Expand All @@ -49,16 +50,29 @@ 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,
})
}
}
Expand Down Expand Up @@ -116,6 +130,7 @@ impl<'a, R: 'a + Read> ImageDecoder<'a> for GifDecoder<R> {
}
};


let (width, height) = self.dimensions();

if frame.left == 0
Expand Down Expand Up @@ -154,7 +169,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(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)?;
Expand Down
20 changes: 19 additions & 1 deletion src/io/mod.rs
Expand Up @@ -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) {
Expand Down
11 changes: 11 additions & 0 deletions tests/regression.rs
Expand Up @@ -59,3 +59,14 @@ 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.
assert!(matches!(image::load_from_memory(&data), Err(image::ImageError::Limits(_))));
}

0 comments on commit cac78c2

Please sign in to comment.