diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 03698fe7a0..a0c8e24206 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -52,21 +52,36 @@ jobs: override: true - name: build run: cargo build -v --no-default-features --features="avif" - # We don't test this yet, as it requires libdav1d which isn't in ubuntu 20.04 - # run: cargo build -v --no-default-features --features="avif-decoding" + test_avif_decoding: + runs-on: ubuntu-20.04 + steps: + - name: install-dependencies + run: sudo apt update && sudo apt install ninja-build meson nasm + - uses: actions/checkout@v2 + - uses: actions-rs/toolchain@v1 + with: + toolchain: stable + override: true + - name: build + run: cargo build -v --no-default-features --features="avif-decoder" + env: + SYSTEM_DEPS_DAV1D_BUILD_INTERNAL: always clippy: - runs-on: ubuntu-latest + runs-on: ubuntu-20.04 steps: + - name: install-dependencies + run: sudo apt update && sudo apt install ninja-build meson nasm - uses: actions/checkout@v2 - uses: actions-rs/toolchain@v1 with: toolchain: stable override: true components: clippy - - name: Run clippy check - uses: actions-rs/cargo@v1 + - uses: actions-rs/cargo@v1 with: - command: clippy + args: clippy --all-features -- -D warnings + env: + SYSTEM_DEPS_DAV1D_BUILD_INTERNAL: always build_fuzz_afl: name: "Fuzz targets (afl)" runs-on: ubuntu-latest diff --git a/CHANGES.md b/CHANGES.md index 17cfdfec5e..3698edaead 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,18 @@ Rust image aims to be a pure-Rust implementation of various popular image format ## Changes +### Version 0.23.14 + +- Unified gif blending in different decode methods, fixing out-of-bounds checks + in a number of weirdly positioned frames. +- Hardened TGA decoder against a number of malicious inputs. +- Fix forward incompatible usage of the panic macro. +- Fix load_rect for gif reaching `unreachable!()` code. + +- Added `ExtendedColorType::A8`. +- Allow TGA to load alpha-only images. +- Optimized load_rect to avoid unnecessary seeks. + ### Version 0.23.13 - Fix an inconsistency in supported formats of different methods for encoding diff --git a/Cargo.toml b/Cargo.toml index aff8cd7315..33bbea63dc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "image" -version = "0.23.13" +version = "0.23.14" edition = "2018" license = "MIT" description = "Imaging library written in Rust. Provides basic filters and decoders for the most common image formats." @@ -27,7 +27,7 @@ num-iter = "0.1.32" num-rational = { version = "0.3", default-features = false } num-traits = "0.2.0" gif = { version = "0.11.1", optional = true } -jpeg = { package = "jpeg-decoder", version = "0.1.17", default-features = false, optional = true } +jpeg = { package = "jpeg-decoder", version = "0.1.22", default-features = false, optional = true } png = { version = "0.16.5", optional = true } scoped_threadpool = { version = "0.1", optional = true } tiff = { version = "0.6.0", optional = true } diff --git a/README.md b/README.md index 1d15a7ea1d..b06c45933e 100644 --- a/README.md +++ b/README.md @@ -136,22 +136,23 @@ assert!(subimg.dimensions() == (100, 100)); ## Image Processing Functions These are the functions defined in the `imageops` module. All functions operate on types that implement the `GenericImage` trait. +Note that some of the functions are very slow in debug mode. Make sure to use release mode if you experience any performance issues. + **blur**: Performs a Gaussian blur on the supplied image. -+ **brighten**: Brighten the supplied image -+ **huerotate**: Hue rotate the supplied image by degrees -+ **contrast**: Adjust the contrast of the supplied image -+ **crop**: Return a mutable view into an image ++ **brighten**: Brighten the supplied image. ++ **huerotate**: Hue rotate the supplied image by degrees. ++ **contrast**: Adjust the contrast of the supplied image. ++ **crop**: Return a mutable view into an image. + **filter3x3**: Perform a 3x3 box filter on the supplied image. -+ **flip_horizontal**: Flip an image horizontally -+ **flip_vertical**: Flip an image vertically -+ **grayscale**: Convert the supplied image to grayscale ++ **flip_horizontal**: Flip an image horizontally. ++ **flip_vertical**: Flip an image vertically. ++ **grayscale**: Convert the supplied image to grayscale. + **invert**: Invert each pixel within the supplied image This function operates in place. -+ **resize**: Resize the supplied image to the specified dimensions ++ **resize**: Resize the supplied image to the specified dimensions. + **rotate180**: Rotate an image 180 degrees clockwise. + **rotate270**: Rotate an image 270 degrees clockwise. + **rotate90**: Rotate an image 90 degrees clockwise. -+ **unsharpen**: Performs an unsharpen mask on the supplied image ++ **unsharpen**: Performs an unsharpen mask on the supplied image. For more options, see the [`imageproc`](https://crates.io/crates/imageproc) crate. diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000000..e299faf7c9 --- /dev/null +++ b/clippy.toml @@ -0,0 +1 @@ +msrv = "1.34.2" diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index c808cb7c0c..98d1a04a0f 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -11,7 +11,7 @@ cargo-fuzz = true [dependencies.image] path = ".." [dependencies.libfuzzer-sys] -git = "https://github.com/rust-fuzz/libfuzzer-sys.git" +version = "0.3" # Prevent this from interfering with workspaces [workspace] diff --git a/fuzz/fuzzers/fuzzer_script_tga.rs b/fuzz/fuzzers/fuzzer_script_tga.rs index 3d1d763dac..3bd6252f9b 100644 --- a/fuzz/fuzzers/fuzzer_script_tga.rs +++ b/fuzz/fuzzers/fuzzer_script_tga.rs @@ -3,5 +3,16 @@ extern crate image; fuzz_target!(|data: &[u8]| { - let _ = image::load_from_memory_with_format(data, image::ImageFormat::Tga); + let _ = decode(data); }); + +fn decode(data: &[u8]) -> Result<(), image::ImageError> { + use image::ImageDecoder; + let decoder = image::codecs::tga::TgaDecoder::new(std::io::Cursor::new(data))?; + if decoder.total_bytes() > 4_000_000 { + return Ok(()); + } + let mut buffer = vec![0; decoder.total_bytes() as usize]; + decoder.read_image(&mut buffer)?; + Ok(()) +} diff --git a/src/codecs/bmp/decoder.rs b/src/codecs/bmp/decoder.rs index 95b1092a17..ff5bb165b1 100644 --- a/src/codecs/bmp/decoder.rs +++ b/src/codecs/bmp/decoder.rs @@ -1467,7 +1467,7 @@ impl<'a, R: 'a + Read + Seek> ImageDecoderExt<'a> for BmpDecoder { progress_callback: F, ) -> ImageResult<()> { let start = self.reader.seek(SeekFrom::Current(0))?; - image::load_rect(x, y, width, height, buf, progress_callback, self, |_, _| unreachable!(), + image::load_rect(x, y, width, height, buf, progress_callback, self, |_, _| Ok(()), |s, buf| s.read_image_data(buf))?; self.reader.seek(SeekFrom::Start(start))?; Ok(()) @@ -1476,7 +1476,7 @@ impl<'a, R: 'a + Read + Seek> ImageDecoderExt<'a> for BmpDecoder { #[cfg(test)] mod test { - use super::Bitfield; + use super::*; #[test] fn test_bitfield_len() { @@ -1492,4 +1492,13 @@ mod test { } } } + + #[test] + fn read_rect() { + let f = std::fs::File::open("tests/images/bmp/images/Core_8_Bit.bmp").unwrap(); + let mut decoder = super::BmpDecoder::new(f).unwrap(); + + let mut buf: Vec = vec![0; 8 * 8 * 3]; + decoder.read_rect(0, 0, 8, 8, &mut *buf).unwrap(); + } } diff --git a/src/codecs/gif.rs b/src/codecs/gif.rs index f623818808..54da98e3a2 100644 --- a/src/codecs/gif.rs +++ b/src/codecs/gif.rs @@ -94,51 +94,61 @@ impl<'a, R: 'a + Read> ImageDecoder<'a> for GifDecoder { fn read_image(mut self, buf: &mut [u8]) -> ImageResult<()> { assert_eq!(u64::try_from(buf.len()), Ok(self.total_bytes())); - let (f_width, f_height, left, top); + let frame = match self.reader.next_frame_info() + .map_err(ImageError::from_decoding)? { + Some(frame) => FrameInfo::new_from_frame(frame), + None => { + return Err(ImageError::Parameter(ParameterError::from_kind( + ParameterErrorKind::NoMoreData, + ))) + } + }; - if let Some(frame) = self.reader.next_frame_info().map_err(ImageError::from_decoding)? { - left = u32::from(frame.left); - top = u32::from(frame.top); - f_width = u32::from(frame.width); - f_height = u32::from(frame.height); - } else { - return Err(ImageError::Parameter( - ParameterError::from_kind(ParameterErrorKind::NoMoreData) - )); - } + let (width, height) = self.dimensions(); - self.reader.read_into_buffer(buf).map_err(ImageError::from_decoding)?; - - let (width, height) = (u32::from(self.reader.width()), u32::from(self.reader.height())); - if (left, top) != (0, 0) || (width, height) != (f_width, f_height) { - // This is somewhat of an annoying case. The image we read into `buf` doesn't take up - // the whole buffer and now we need to properly insert borders. For simplicity this code - // currently takes advantage of the `ImageBuffer::from_fn` function to make a second - // ImageBuffer that is properly positioned, and then copies it back into `buf`. - // - // TODO: Implement this without any allocation. - - // Recover the full image - let image_buffer = { - // See the comments inside `::next` about - // the error handling of `from_raw`. - let image = ImageBuffer::from_raw(f_width, f_height, &mut *buf).ok_or_else( - || ImageError::Unsupported(UnsupportedError::from_format_and_kind( + if (frame.left, frame.top) == (0, 0) && (frame.width, frame.height) != self.dimensions() { + // If the frame matches the logical screen, directly read into it + self.reader.read_into_buffer(buf).map_err(ImageError::from_decoding)?; + } 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()]; + self.reader.read_into_buffer(&mut frame_buffer[..]).map_err(ImageError::from_decoding)?; + + let frame_buffer = ImageBuffer::from_raw(frame.width, frame.height, frame_buffer); + let image_buffer = ImageBuffer::from_raw(width, height, buf); + + // `buffer_size` uses wrapping arithmetics, thus might not report the + // correct storage requirement if the result does not fit in `usize`. + // `ImageBuffer::from_raw` detects overflow and reports by returning `None`. + if frame_buffer.is_none() || image_buffer.is_none() { + return Err(ImageError::Unsupported( + UnsupportedError::from_format_and_kind( ImageFormat::Gif.into(), - UnsupportedErrorKind::GenericFeature(format!("Image dimensions ({}, {}) are too large", f_width, f_height)))))?; - - ImageBuffer::from_fn(width, height, |x, y| { - let x = x.wrapping_sub(left); - let y = y.wrapping_sub(top); - if x < image.width() && y < image.height() { - *image.get_pixel(x, y) - } else { - Rgba([0, 0, 0, 0]) - } - }) - }; - buf.copy_from_slice(&image_buffer.into_raw()); + UnsupportedErrorKind::GenericFeature(format!( + "Image dimensions ({}, {}) are too large", + frame.width, frame.height + )), + ), + )); + } + + let frame_buffer = frame_buffer.unwrap(); + let mut image_buffer = image_buffer.unwrap(); + + for (x, y, pixel) in image_buffer.enumerate_pixels_mut() { + let frame_x = x.wrapping_sub(frame.left); + let frame_y = y.wrapping_sub(frame.top); + + if frame_x < frame.width && frame_y < frame.height { + *pixel = *frame_buffer.get_pixel(frame_x, frame_y); + } else { + // this is only necessary in case the buffer is not zeroed + *pixel = Rgba([0, 0, 0, 0]); + } + } } + Ok(()) } } @@ -152,7 +162,6 @@ struct GifFrameIterator { non_disposed_frame: ImageBuffer, Vec>, } - impl GifFrameIterator { fn new(decoder: GifDecoder) -> GifFrameIterator { let (width, height) = decoder.dimensions(); @@ -174,32 +183,23 @@ impl GifFrameIterator { } } - impl Iterator for GifFrameIterator { type Item = ImageResult; fn next(&mut self) -> Option> { // begin looping over each frame - let (left, top, delay, dispose, f_width, f_height); - match self.reader.next_frame_info() { + let frame = match self.reader.next_frame_info() { Ok(frame_info) => { if let Some(frame) = frame_info { - left = u32::from(frame.left); - top = u32::from(frame.top); - f_width = u32::from(frame.width); - f_height = u32::from(frame.height); - - // frame.delay is in units of 10ms so frame.delay*10 is in ms - delay = Ratio::new(u32::from(frame.delay) * 10, 1); - dispose = frame.dispose; + FrameInfo::new_from_frame(frame) } else { // no more frames return None; } - }, + } Err(err) => return Some(Err(ImageError::from_decoding(err))), - } + }; let mut vec = vec![0; self.reader.buffer_size()]; if let Err(err) = self.reader.read_into_buffer(&mut vec) { @@ -211,13 +211,18 @@ impl Iterator for GifFrameIterator { // correct storage requirement if the result does not fit in `usize`. // on the other hand, `ImageBuffer::from_raw` detects overflow and // reports by returning `None`. - let mut frame_buffer = match ImageBuffer::from_raw(f_width, f_height, vec) { + let mut frame_buffer = match ImageBuffer::from_raw(frame.width, frame.height, vec) { Some(frame_buffer) => frame_buffer, None => { - return Some(Err(ImageError::Unsupported(UnsupportedError::from_format_and_kind( - ImageFormat::Gif.into(), - UnsupportedErrorKind::GenericFeature(format!("Image dimensions ({}, {}) are too large", f_width, f_height)), - )))) + return Some(Err(ImageError::Unsupported( + UnsupportedError::from_format_and_kind( + ImageFormat::Gif.into(), + UnsupportedErrorKind::GenericFeature(format!( + "Image dimensions ({}, {}) are too large", + frame.width, frame.height + )), + ), + ))) } }; @@ -253,22 +258,22 @@ impl Iterator for GifFrameIterator { // if `frame_buffer`'s frame exactly matches the entire image, then // use it directly, else create a new buffer to hold the composited // image. - let image_buffer = if (left, top) == (0, 0) + let image_buffer = if (frame.left, frame.top) == (0, 0) && (self.width, self.height) == frame_buffer.dimensions() { for (x, y, pixel) in frame_buffer.enumerate_pixels_mut() { let previous_pixel = self.non_disposed_frame.get_pixel_mut(x, y); - blend_and_dispose_pixel(dispose, previous_pixel, pixel); + blend_and_dispose_pixel(frame.disposal_method, previous_pixel, pixel); } frame_buffer } else { ImageBuffer::from_fn(self.width, self.height, |x, y| { - let frame_x = x.wrapping_sub(left); - let frame_y = y.wrapping_sub(top); + let frame_x = x.wrapping_sub(frame.left); + let frame_y = y.wrapping_sub(frame.top); let previous_pixel = self.non_disposed_frame.get_pixel_mut(x, y); if frame_x < frame_buffer.width() && frame_y < frame_buffer.height() { let mut pixel = *frame_buffer.get_pixel(frame_x, frame_y); - blend_and_dispose_pixel(dispose, previous_pixel, &mut pixel); + blend_and_dispose_pixel(frame.disposal_method, previous_pixel, &mut pixel); pixel } else { // out of bounds, return pixel from previous frame @@ -278,7 +283,7 @@ impl Iterator for GifFrameIterator { }; Some(Ok(animation::Frame::from_parts( - image_buffer, 0, 0, animation::Delay::from_ratio(delay), + image_buffer, 0,0, frame.delay, ))) } } @@ -289,8 +294,31 @@ impl<'a, R: Read + 'a> AnimationDecoder<'a> for GifDecoder { } } +struct FrameInfo { + left: u32, + top: u32, + width: u32, + height: u32, + disposal_method: DisposalMethod, + delay: animation::Delay, +} + +impl FrameInfo { + fn new_from_frame(frame: &Frame) -> FrameInfo { + FrameInfo { + left: u32::from(frame.left), + top: u32::from(frame.top), + width: u32::from(frame.width), + height: u32::from(frame.height), + disposal_method: frame.dispose, + // frame.delay is in units of 10ms so frame.delay*10 is in ms + delay: animation::Delay::from_ratio(Ratio::new(u32::from(frame.delay) * 10, 1)), + } + } +} + /// Number of repetitions for a GIF animation -#[derive(Clone, Copy)] +#[derive(Clone, Copy, Debug)] pub enum Repeat { /// Finite number of repetitions Finite(u16), @@ -477,3 +505,26 @@ impl ImageError { } } } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn frames_exceeding_logical_screen_size() { + // This is a gif with 10x10 logical screen, but a 16x16 frame + 6px offset inside. + let data = vec![ + 0x47, 0x49, 0x46, 0x38, 0x39, 0x61, 0x0A, 0x00, 0x0A, 0x00, 0xF0, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x0E, 0xFF, 0x1F, 0x21, 0xF9, 0x04, 0x09, 0x64, 0x00, 0x00, 0x00, 0x2C, + 0x06, 0x00, 0x06, 0x00, 0x10, 0x00, 0x10, 0x00, 0x00, 0x02, 0x23, 0x84, 0x8F, 0xA9, + 0xBB, 0xE1, 0xE8, 0x42, 0x8A, 0x0F, 0x50, 0x79, 0xAE, 0xD1, 0xF9, 0x7A, 0xE8, 0x71, + 0x5B, 0x48, 0x81, 0x64, 0xD5, 0x91, 0xCA, 0x89, 0x4D, 0x21, 0x63, 0x89, 0x4C, 0x09, + 0x77, 0xF5, 0x6D, 0x14, 0x00, 0x3B, + ]; + + let decoder = GifDecoder::new(Cursor::new(data)).unwrap(); + let mut buf = vec![0u8; decoder.total_bytes() as usize]; + + assert!(decoder.read_image(&mut buf).is_ok()); + } +} diff --git a/src/codecs/ico/decoder.rs b/src/codecs/ico/decoder.rs index 86da65c5e1..7b7322173e 100644 --- a/src/codecs/ico/decoder.rs +++ b/src/codecs/ico/decoder.rs @@ -116,9 +116,20 @@ enum InnerDecoder { struct DirEntry { width: u8, height: u8, + // We ignore some header fields as they will be replicated in the PNG, BMP and they are not + // necessary for determining the best_entry. + #[allow(unused)] color_count: u8, + // Wikipedia has this to say: + // Although Microsoft's technical documentation states that this value must be zero, the icon + // encoder built into .NET (System.Drawing.Icon.Save) sets this value to 255. It appears that + // the operating system ignores this value altogether. + #[allow(unused)] reserved: u8, + // We ignore some header fields as they will be replicated in the PNG, BMP and they are not + // necessary for determining the best_entry. + #[allow(unused)] num_color_planes: u16, bits_per_pixel: u16, @@ -148,32 +159,32 @@ fn read_entries(r: &mut R) -> ImageResult> { } fn read_entry(r: &mut R) -> ImageResult { - let mut entry = DirEntry::default(); - - entry.width = r.read_u8()?; - entry.height = r.read_u8()?; - entry.color_count = r.read_u8()?; - // Reserved value (not used) - entry.reserved = r.read_u8()?; - - // This may be either the number of color planes (0 or 1), or the horizontal coordinate - // of the hotspot for CUR files. - entry.num_color_planes = r.read_u16::()?; - if entry.num_color_planes > 256 { - return Err(DecoderError::IcoEntryTooManyPlanesOrHotspot.into()); - } - - // This may be either the bit depth (may be 0 meaning unspecified), - // or the vertical coordinate of the hotspot for CUR files. - entry.bits_per_pixel = r.read_u16::()?; - if entry.bits_per_pixel > 256 { - return Err(DecoderError::IcoEntryTooManyBitsPerPixelOrHotspot.into()); - } - - entry.image_length = r.read_u32::()?; - entry.image_offset = r.read_u32::()?; - - Ok(entry) + Ok(DirEntry { + width: r.read_u8()?, + height: r.read_u8()?, + color_count: r.read_u8()?, + reserved: r.read_u8()?, + num_color_planes: { + // This may be either the number of color planes (0 or 1), or the horizontal coordinate + // of the hotspot for CUR files. + let num = r.read_u16::()?; + if num > 256 { + return Err(DecoderError::IcoEntryTooManyPlanesOrHotspot.into()); + } + num + }, + bits_per_pixel: { + // This may be either the bit depth (may be 0 meaning unspecified), + // or the vertical coordinate of the hotspot for CUR files. + let num = r.read_u16::()?; + if num > 256 { + return Err(DecoderError::IcoEntryTooManyBitsPerPixelOrHotspot.into()); + } + num + }, + image_length: r.read_u32::()?, + image_offset: r.read_u32::()?, + }) } /// Find the entry with the highest (color depth, size). diff --git a/src/codecs/pnm/encoder.rs b/src/codecs/pnm/encoder.rs index 355f190d6f..e50b7c2234 100644 --- a/src/codecs/pnm/encoder.rs +++ b/src/codecs/pnm/encoder.rs @@ -661,12 +661,11 @@ impl<'a> TupleEncoding<'a> { samples: FlatSamples::U16(samples), } => samples .iter() - .map(|&sample| { + .try_for_each(|&sample| { writer .write_u16::(sample) .map_err(ImageError::IoError) - }) - .collect(), + }), TupleEncoding::Ascii { samples: FlatSamples::U8(samples), diff --git a/src/codecs/tga/decoder.rs b/src/codecs/tga/decoder.rs index c9df7b7d6c..9c75e4cffe 100644 --- a/src/codecs/tga/decoder.rs +++ b/src/codecs/tga/decoder.rs @@ -37,9 +37,9 @@ impl ColorMap { } /// Get one entry from the color map - pub(crate) fn get(&self, index: usize) -> &[u8] { + pub(crate) fn get(&self, index: usize) -> Option<&[u8]> { let entry = self.start_offset + self.entry_size * index; - &self.bytes[entry..entry + self.entry_size] + self.bytes.get(entry..entry + self.entry_size) } } @@ -54,6 +54,7 @@ pub struct TgaDecoder { image_type: ImageType, color_type: ColorType, + original_color_type: Option, header: Header, color_map: Option, @@ -76,6 +77,7 @@ impl TgaDecoder { image_type: ImageType::Unknown, color_type: ColorType::L8, + original_color_type: None, header: Header::default(), color_map: None, @@ -153,6 +155,11 @@ impl TgaDecoder { (0, 24, true) => self.color_type = ColorType::Rgb8, (8, 8, false) => self.color_type = ColorType::La8, (0, 8, false) => self.color_type = ColorType::L8, + (8, 0, false) => { + // alpha-only image is treated as L8 + self.color_type = ColorType::L8; + self.original_color_type = Some(ExtendedColorType::A8); + }, _ => { return Err(ImageError::Unsupported( UnsupportedError::from_format_and_kind( @@ -179,6 +186,8 @@ impl TgaDecoder { fn read_color_map(&mut self) -> ImageResult<()> { if self.header.map_type == 1 { + // FIXME: we could reverse the map entries, which avoids having to reverse all pixels + // in the final output individually. self.color_map = Some(ColorMap::from_reader( &mut self.r, self.header.map_origin, @@ -190,7 +199,7 @@ impl TgaDecoder { } /// Expands indices into its mapped color - fn expand_color_map(&self, pixel_data: &[u8]) -> Vec { + fn expand_color_map(&self, pixel_data: &[u8]) -> io::Result> { #[inline] fn bytes_to_index(bytes: &[u8]) -> usize { let mut result = 0usize; @@ -203,14 +212,24 @@ impl TgaDecoder { let bytes_per_entry = (self.header.map_entry_size as usize + 7) / 8; let mut result = Vec::with_capacity(self.width * self.height * bytes_per_entry); - let color_map = self.color_map.as_ref().unwrap(); + if self.bytes_per_pixel == 0 { + return Err(io::ErrorKind::Other.into()); + } + + let color_map = self.color_map + .as_ref() + .ok_or_else(|| io::Error::from(io::ErrorKind::Other))?; for chunk in pixel_data.chunks(self.bytes_per_pixel) { let index = bytes_to_index(chunk); - result.extend(color_map.get(index).iter().cloned()); + if let Some(color) = color_map.get(index) { + result.extend(color.iter().cloned()); + } else { + return Err(io::ErrorKind::Other.into()); + } } - result + Ok(result) } /// Reads a run length encoded data for given number of bytes @@ -244,6 +263,13 @@ impl TgaDecoder { } } + if pixel_data.len() > num_bytes { + // FIXME: the last packet contained more data than we asked for! + // This is at least a warning. We truncate the data since some methods rely on the + // length to be accurate in the success case. + pixel_data.truncate(num_bytes); + } + Ok(pixel_data) } @@ -284,11 +310,11 @@ impl TgaDecoder { /// /// TGA files are stored in the BGRA encoding. This function swaps /// the blue and red bytes in the `pixels` array. - fn reverse_encoding(&mut self, pixels: &mut [u8]) { + fn reverse_encoding_in_output(&mut self, pixels: &mut [u8]) { // We only need to reverse the encoding of color images match self.color_type { ColorType::Rgb8 | ColorType::Rgba8 => { - for chunk in pixels.chunks_mut(self.bytes_per_pixel) { + for chunk in pixels.chunks_mut(self.color_type.bytes_per_pixel().into()) { chunk.swap(0, 2); } } @@ -304,6 +330,10 @@ impl TgaDecoder { /// This function checks the bit, and if it's 0, flips the image vertically. fn flip_vertically(&mut self, pixels: &mut [u8]) { if self.is_flipped_vertically() { + if self.height == 0 { + return; + } + let num_bytes = pixels.len(); let width_bytes = num_bytes / self.height; @@ -352,9 +382,9 @@ impl TgaDecoder { // expand the indices using the color map if necessary if self.image_type.is_color_mapped() { - pixel_data = self.expand_color_map(&pixel_data) + pixel_data = self.expand_color_map(&pixel_data)?; } - self.reverse_encoding(&mut pixel_data); + self.reverse_encoding_in_output(&mut pixel_data); // copy to the output buffer buf[..pixel_data.len()].copy_from_slice(&pixel_data); @@ -376,6 +406,10 @@ impl<'a, R: 'a + Read + Seek> ImageDecoder<'a> for TgaDecoder { self.color_type } + fn original_color_type(&self) -> ExtendedColorType { + self.original_color_type.unwrap_or_else(|| self.color_type().into()) + } + fn scanline_bytes(&self) -> u64 { // This cannot overflow because TGA has a maximum width of u16::MAX_VALUE and // `bytes_per_pixel` is a u8. @@ -392,24 +426,38 @@ impl<'a, R: 'a + Read + Seek> ImageDecoder<'a> for TgaDecoder { fn read_image(mut self, buf: &mut [u8]) -> ImageResult<()> { assert_eq!(u64::try_from(buf.len()), Ok(self.total_bytes())); + // In indexed images, we might need more bytes than pixels to read them. That's nonsensical + // to encode but we'll not want to crash. + let mut fallback_buf = vec![]; // read the pixels from the data region - let len = if self.image_type.is_encoded() { + let rawbuf = if self.image_type.is_encoded() { let pixel_data = self.read_all_encoded_data()?; - buf[0..pixel_data.len()].copy_from_slice(&pixel_data); - pixel_data.len() + if self.bytes_per_pixel <= usize::from(self.color_type.bytes_per_pixel()) { + buf[..pixel_data.len()].copy_from_slice(&pixel_data); + &buf[..pixel_data.len()] + } else { + fallback_buf = pixel_data; + &fallback_buf[..] + } } else { let num_raw_bytes = self.width * self.height * self.bytes_per_pixel; - self.r.by_ref().read_exact(&mut buf[0..num_raw_bytes])?; - num_raw_bytes + if self.bytes_per_pixel <= usize::from(self.color_type.bytes_per_pixel()) { + self.r.by_ref().read_exact(&mut buf[..num_raw_bytes])?; + &buf[..num_raw_bytes] + } else { + fallback_buf.resize(num_raw_bytes, 0u8); + self.r.by_ref().read_exact(&mut fallback_buf[..num_raw_bytes])?; + &fallback_buf[..num_raw_bytes] + } }; // expand the indices using the color map if necessary if self.image_type.is_color_mapped() { - let pixel_data = self.expand_color_map(&buf[0..len]); + let pixel_data = self.expand_color_map(rawbuf)?; buf.copy_from_slice(&pixel_data); } - self.reverse_encoding(buf); + self.reverse_encoding_in_output(buf); self.flip_vertically(buf); diff --git a/src/codecs/tiff.rs b/src/codecs/tiff.rs index 8380642c5f..fd8fa7eae4 100644 --- a/src/codecs/tiff.rs +++ b/src/codecs/tiff.rs @@ -81,7 +81,7 @@ impl TiffDecoder fn check_sample_format(sample_format: u16) -> Result<(), ImageError> { match tiff::tags::SampleFormat::from_u16(sample_format) { - Some(tiff::tags::SampleFormat::Uint) => Ok({}), + Some(tiff::tags::SampleFormat::Uint) => Ok(()), Some(other) => { Err(ImageError::Unsupported(UnsupportedError::from_format_and_kind( ImageFormat::Tiff.into(), diff --git a/src/codecs/webp/vp8.rs b/src/codecs/webp/vp8.rs index 1d0fb9bb88..56b1732bd7 100644 --- a/src/codecs/webp/vp8.rs +++ b/src/codecs/webp/vp8.rs @@ -1259,11 +1259,9 @@ impl Vp8Decoder { fn read_macroblock_header(&mut self, mbx: usize) -> ImageResult<(bool, MacroBlock)> { let mut mb = MacroBlock::default(); - mb.segmentid = if self.segments_enabled && self.segments_update_map { - self.b - .read_with_tree(&SEGMENT_ID_TREE, &self.segment_tree_probs, 0) as u8 - } else { - 0 + if self.segments_enabled && self.segments_update_map { + mb.segmentid = self.b + .read_with_tree(&SEGMENT_ID_TREE, &self.segment_tree_probs, 0) as u8; }; let skip_coeff = if self.prob_skip_false.is_some() { @@ -1436,7 +1434,7 @@ impl Vp8Decoder { i16::from(DCT_CAT_BASE[(category - DCT_CAT1) as usize]) + extra } - c => panic!(format!("unknown token: {}", c)), + c => panic!("unknown token: {}", c), }); skip = false; diff --git a/src/color.rs b/src/color.rs index 771e43fc1b..0166c082aa 100644 --- a/src/color.rs +++ b/src/color.rs @@ -91,6 +91,8 @@ impl ColorType { /// decoding from and encoding to such an image format. #[derive(Copy, PartialEq, Eq, Debug, Clone, Hash)] pub enum ExtendedColorType { + /// Pixel is 8-bit alpha + A8, /// Pixel is 1-bit luminance L1, /// Pixel is 1-bit luminance with an alpha channel @@ -152,6 +154,7 @@ impl ExtendedColorType { /// an opaque datum by the library. pub fn channel_count(self) -> u8 { match self { + ExtendedColorType::A8 | ExtendedColorType::L1 | ExtendedColorType::L2 | ExtendedColorType::L4 | diff --git a/src/image.rs b/src/image.rs index daa4213086..bb8635dbdd 100644 --- a/src/image.rs +++ b/src/image.rs @@ -121,7 +121,7 @@ impl ImageFormat { fn inner(path: &Path) -> ImageResult { let exact_ext = path.extension(); exact_ext - .and_then(|ext| ImageFormat::from_extension(ext)) + .and_then(ImageFormat::from_extension) .ok_or_else(|| { let format_hint = match exact_ext { None => ImageFormatHint::Unknown, @@ -387,11 +387,34 @@ pub(crate) fn load_rect<'a, D, F, F1, F2, E>(x: u32, y: u32, width: u32, height: let mut bytes_read = 0u64; let mut current_scanline = 0; let mut tmp = Vec::new(); + let mut tmp_scanline = None; { // Read a range of the image starting from byte number `start` and continuing until byte // number `end`. Updates `current_scanline` and `bytes_read` appropiately. - let mut read_image_range = |start: u64, end: u64| -> ImageResult<()> { + let mut read_image_range = |mut start: u64, end: u64| -> ImageResult<()> { + // If the first scanline we need is already stored in the temporary buffer, then handle + // it first. + let target_scanline = start / scanline_bytes; + if tmp_scanline == Some(target_scanline) { + let position = target_scanline * scanline_bytes; + let offset = start.saturating_sub(position); + let len = (end - start) + .min(scanline_bytes - offset) + .min(end - position); + + buf[(bytes_read as usize)..][..len as usize] + .copy_from_slice(&tmp[offset as usize..][..len as usize]); + bytes_read += len; + start += len; + + progress_callback(Progress {current: bytes_read, total: total_bytes}); + + if start == end { + return Ok(()); + } + } + let target_scanline = start / scanline_bytes; if target_scanline != current_scanline { seek_scanline(decoder, target_scanline)?; @@ -407,6 +430,7 @@ pub(crate) fn load_rect<'a, D, F, F1, F2, E>(x: u32, y: u32, width: u32, height: } else { tmp.resize(scanline_bytes as usize, 0u8); read_scanline(decoder, &mut tmp)?; + tmp_scanline = Some(current_scanline); let offset = start.saturating_sub(position); let len = (end - start) @@ -1185,6 +1209,45 @@ mod tests { } } + #[test] + fn test_load_rect_single_scanline() { + const DATA: [u8; 25] = [0, 1, 2, 3, 4, + 5, 6, 7, 8, 9, + 10, 11, 12, 13, 14, + 15, 16, 17, 18, 19, + 20, 21, 22, 23, 24]; + + struct MockDecoder; + impl<'a> ImageDecoder<'a> for MockDecoder { + type Reader = Box; + fn dimensions(&self) -> (u32, u32) {(5, 5)} + fn color_type(&self) -> ColorType { ColorType::L8 } + fn into_reader(self) -> ImageResult {unimplemented!()} + fn scanline_bytes(&self) -> u64 { 25 } + } + + // Ensure that seek scanline is called only once. + let mut seeks = 0; + let seek_scanline = |_d: &mut MockDecoder, n: u64| -> io::Result<()> { + seeks += 1; + assert_eq!(n, 0); + assert_eq!(seeks, 1); + Ok(()) + }; + + fn read_scanline(_m: &mut MockDecoder, buf: &mut [u8]) -> io::Result<()> { + buf.copy_from_slice(&DATA); + Ok(()) + } + + let mut output = [0; 26]; + load_rect(1, 1, 2, 4, &mut output, |_|{}, + &mut MockDecoder, + seek_scanline, read_scanline).unwrap(); + assert_eq!(output[0..9], [6, 7, 11, 12, 16, 17, 21, 22, 0]); + } + + #[test] fn test_image_format_from_path() { fn from_path(s: &str) -> ImageResult { diff --git a/src/imageops/mod.rs b/src/imageops/mod.rs index 46f507760a..d24845a8d7 100644 --- a/src/imageops/mod.rs +++ b/src/imageops/mod.rs @@ -174,13 +174,11 @@ where /// ```no_run /// use image::{RgbaImage}; /// -/// fn main() { -/// let mut img = RgbaImage::new(1920, 1080); -/// let tile = image::open("tile.png").unwrap(); +/// let mut img = RgbaImage::new(1920, 1080); +/// let tile = image::open("tile.png").unwrap(); /// -/// image::imageops::tile(&mut img, &tile); -/// img.save("tiled_wallpaper.png").unwrap(); -/// } +/// image::imageops::tile(&mut img, &tile); +/// img.save("tiled_wallpaper.png").unwrap(); /// ``` pub fn tile(bottom: &mut I, top: &J) where @@ -202,14 +200,12 @@ where /// ```no_run /// use image::{Rgba, RgbaImage, Pixel}; /// -/// fn main() { -/// let mut img = RgbaImage::new(100, 100); -/// let start = Rgba::from_slice(&[0, 128, 0, 0]); -/// let end = Rgba::from_slice(&[255, 255, 255, 255]); +/// let mut img = RgbaImage::new(100, 100); +/// let start = Rgba::from_slice(&[0, 128, 0, 0]); +/// let end = Rgba::from_slice(&[255, 255, 255, 255]); /// -/// image::imageops::vertical_gradient(&mut img, start, end); -/// img.save("vertical_gradient.png").unwrap(); -/// } +/// image::imageops::vertical_gradient(&mut img, start, end); +/// img.save("vertical_gradient.png").unwrap(); pub fn vertical_gradient(img: &mut I, start: &P, stop: &P) where I: GenericImage, @@ -237,14 +233,12 @@ where /// ```no_run /// use image::{Rgba, RgbaImage, Pixel}; /// -/// fn main() { -/// let mut img = RgbaImage::new(100, 100); -/// let start = Rgba::from_slice(&[0, 128, 0, 0]); -/// let end = Rgba::from_slice(&[255, 255, 255, 255]); +/// let mut img = RgbaImage::new(100, 100); +/// let start = Rgba::from_slice(&[0, 128, 0, 0]); +/// let end = Rgba::from_slice(&[255, 255, 255, 255]); /// -/// image::imageops::horizontal_gradient(&mut img, start, end); -/// img.save("horizontal_gradient.png").unwrap(); -/// } +/// image::imageops::horizontal_gradient(&mut img, start, end); +/// img.save("horizontal_gradient.png").unwrap(); pub fn horizontal_gradient(img: &mut I, start: &P, stop: &P) where I: GenericImage, diff --git a/src/lib.rs b/src/lib.rs index 0f395e872e..10198ffba6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -91,6 +91,8 @@ #![cfg_attr(all(test, feature = "benchmarks"), feature(test))] // it's a bit of a pain otherwise #![allow(clippy::many_single_char_names)] +// it's a backwards compatibility break +#![allow(clippy::wrong_self_convention, clippy::enum_variant_names)] #[cfg(all(test, feature = "benchmarks"))] extern crate test; diff --git a/tests/reference_images.rs b/tests/reference_images.rs index 5382175941..2f209fb8d3 100644 --- a/tests/reference_images.rs +++ b/tests/reference_images.rs @@ -54,7 +54,7 @@ fn render_images() { println!("UNSUPPORTED {}: {}", path.display(), e); return; } - Err(err) => panic!(format!("decoding of {:?} failed with: {}", path, err)), + Err(err) => panic!("decoding of {:?} failed with: {}", path, err), }; let mut crc = Crc32::new(); crc.update(&*img); @@ -161,7 +161,7 @@ fn check_references() { // This might happen because the testsuite contains unsupported images // or because a specific decoder included via a feature. Err(image::ImageError::Unsupported(_)) => return, - Err(err) => panic!(format!("{}", err)), + Err(err) => panic!("{}", err), }; let (filename, testsuite) = { @@ -199,17 +199,17 @@ fn check_references() { Ok(decoder) => decoder, Err(image::ImageError::Unsupported(_)) => return, Err(err) => { - panic!(format!("decoding of {:?} failed with: {}", img_path, err)) + panic!("decoding of {:?} failed with: {}", img_path, err) } }; let mut frames = match decoder.into_frames().collect_frames() { Ok(frames) => frames, Err(image::ImageError::Unsupported(_)) => return, - Err(err) => panic!(format!( + Err(err) => panic!( "collecting frames of {:?} failed with: {}", img_path, err - )), + ), }; // Select a single frame @@ -228,17 +228,17 @@ fn check_references() { Ok(decoder) => decoder.apng(), Err(image::ImageError::Unsupported(_)) => return, Err(err) => { - panic!(format!("decoding of {:?} failed with: {}", img_path, err)) + panic!("decoding of {:?} failed with: {}", img_path, err) } }; let mut frames = match decoder.into_frames().collect_frames() { Ok(frames) => frames, Err(image::ImageError::Unsupported(_)) => return, - Err(err) => panic!(format!( + Err(err) => panic!( "collecting frames of {:?} failed with: {}", img_path, err - )), + ), }; // Select a single frame @@ -262,7 +262,7 @@ fn check_references() { // This might happen because the testsuite contains unsupported images // or because a specific decoder included via a feature. Err(image::ImageError::Unsupported(_)) => return, - Err(err) => panic!(format!("decoding of {:?} failed with: {}", img_path, err)), + Err(err) => panic!("decoding of {:?} failed with: {}", img_path, err), }; } } @@ -330,22 +330,3 @@ fn check_hdr_references() { assert_eq!(decoded, reference); } } - -/// Check that BMP files with large values could cause OOM issues are rejected. -/// -/// The images are postfixed with `bad_bmp` to not be loaded by the other test. -#[test] -fn bad_bmps() { - let path: PathBuf = BASE_PATH - .iter() - .collect::() - .join(IMAGE_DIR) - .join("bmp/images") - .join("*.bad_bmp"); - - let pattern = &*format!("{}", path.display()); - for path in glob::glob(pattern).unwrap().filter_map(Result::ok) { - let im = image::open(path); - assert!(im.is_err()); - } -} diff --git a/tests/regression.rs b/tests/regression.rs new file mode 100644 index 0000000000..94e7a53326 --- /dev/null +++ b/tests/regression.rs @@ -0,0 +1,55 @@ +use std::path::PathBuf; + +const BASE_PATH: [&str; 2] = [".", "tests"]; +const IMAGE_DIR: &str = "images"; +const REGRESSION_DIR: &str = "regression"; + +fn process_images(dir: &str, input_decoder: Option<&str>, func: F) +where + F: Fn(PathBuf), +{ + let base: PathBuf = BASE_PATH.iter().collect(); + let decoders = &["tga", "tiff", "png", "gif", "bmp", "ico", "jpg", "hdr", "pbm", "webp"]; + for decoder in decoders { + let mut path = base.clone(); + path.push(dir); + path.push(decoder); + path.push("**"); + path.push( + "*.".to_string() + match input_decoder { + Some(val) => val, + None => decoder, + }, + ); + let pattern = &*format!("{}", path.display()); + for path in glob::glob(pattern).unwrap().filter_map(Result::ok) { + func(path) + } + } +} + +#[test] +fn check_regressions() { + process_images(REGRESSION_DIR, None, |path| { + let _ = image::open(path); + }) +} + +/// Check that BMP files with large values could cause OOM issues are rejected. +/// +/// The images are postfixed with `bad_bmp` to not be loaded by the other test. +#[test] +fn bad_bmps() { + let path: PathBuf = BASE_PATH + .iter() + .collect::() + .join(IMAGE_DIR) + .join("bmp/images") + .join("*.bad_bmp"); + + let pattern = &*format!("{}", path.display()); + for path in glob::glob(pattern).unwrap().filter_map(Result::ok) { + let im = image::open(path); + assert!(im.is_err()); + } +} diff --git a/tests/regression/tga/bad-color-map.tga b/tests/regression/tga/bad-color-map.tga new file mode 100644 index 0000000000..c6d2dd9fea Binary files /dev/null and b/tests/regression/tga/bad-color-map.tga differ diff --git a/tests/regression/tga/flip-zero-height.tga b/tests/regression/tga/flip-zero-height.tga new file mode 100644 index 0000000000..d578507dd4 Binary files /dev/null and b/tests/regression/tga/flip-zero-height.tga differ diff --git a/tests/regression/tga/flip-zero-height2.tga b/tests/regression/tga/flip-zero-height2.tga new file mode 100644 index 0000000000..7bdec027ae Binary files /dev/null and b/tests/regression/tga/flip-zero-height2.tga differ diff --git a/tests/regression/tga/overfull-image.tga b/tests/regression/tga/overfull-image.tga new file mode 100644 index 0000000000..110b9e715b Binary files /dev/null and b/tests/regression/tga/overfull-image.tga differ diff --git a/tests/regression/tga/sixteen-bit-gray-colormap.tga b/tests/regression/tga/sixteen-bit-gray-colormap.tga new file mode 100644 index 0000000000..7c934e5bc9 Binary files /dev/null and b/tests/regression/tga/sixteen-bit-gray-colormap.tga differ diff --git a/tests/regression/tga/sixteen-bit-gray-colormap2.tga b/tests/regression/tga/sixteen-bit-gray-colormap2.tga new file mode 100644 index 0000000000..a51ce9cca7 Binary files /dev/null and b/tests/regression/tga/sixteen-bit-gray-colormap2.tga differ