diff --git a/src/encoder.rs b/src/encoder.rs index 4590061..b64c9c5 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -3,13 +3,11 @@ use image::DynamicImage; use libwebp_sys::*; use crate::shared::*; +use internal::CheckedEncoder; /// An encoder for WebP images. It uses the default configuration of libwebp. pub struct Encoder<'a> { - image: &'a [u8], - layout: PixelLayout, - width: u32, - height: u32, + e: CheckedEncoder<'a>, } impl<'a> Encoder<'a> { @@ -17,10 +15,7 @@ impl<'a> Encoder<'a> { /// The image data must be in the pixel layout of the color parameter. pub fn new(image: &'a [u8], layout: PixelLayout, width: u32, height: u32) -> Self { Self { - image, - layout, - width, - height, + e: CheckedEncoder::new(image, layout, width, height), } } @@ -47,20 +42,14 @@ impl<'a> Encoder<'a> { /// Creates a new encoder from the given image data in the RGB pixel layout. pub fn from_rgb(image: &'a [u8], width: u32, height: u32) -> Self { Self { - image, - layout: PixelLayout::Rgb, - width, - height, + e: CheckedEncoder::new(image, PixelLayout::Rgb, width, height), } } /// Creates a new encoder from the given image data in the RGBA pixel layout. pub fn from_rgba(image: &'a [u8], width: u32, height: u32) -> Self { Self { - image, - layout: PixelLayout::Rgba, - width, - height, + e: CheckedEncoder::new(image, PixelLayout::Rgba, width, height), } } @@ -89,8 +78,99 @@ impl<'a> Encoder<'a> { pub fn encode_advanced(&self, config: &WebPConfig) -> Result { unsafe { - let mut picture = new_picture(self.image, self.layout, self.width, self.height); - encode(&mut picture, config) + let mut picture = new_picture( + self.e.image(), + self.e.layout(), + self.e.width(), + self.e.height(), + ); + let res = encode(&mut *picture, config); + res + } + } +} + +/// This module contains the private CheckedEncoder so that it cannot be constructed directly from the outside. +/// That ensures that the only way to construct one validates the supplied parameters. +mod internal { + use std::convert::TryInto; + + use crate::shared::PixelLayout; + + /// Validated encoder parameters, guaranteeing `image.len() >= width * height * bytes_per_pixel`. + /// Required for memory safety. Their absence would allow out-of-bounds reads. + pub(super) struct CheckedEncoder<'a> { + image: &'a [u8], + layout: PixelLayout, + width: u32, + height: u32, + } + + impl<'a> CheckedEncoder<'a> { + /// Creates a new instance of `CheckedEncoder`, and performs the necessary bounds checks. + /// + /// This is the only way to create a `CheckedEncoder` exposed outside the `internal` module. + pub(super) fn new(image: &'a [u8], layout: PixelLayout, width: u32, height: u32) -> Self { + // TODO: return an error instead of panicking in the next semver-breaking release + + if width == 0 || height == 0 { + // TODO: enable this check once this function returns a Result. + // As of v0.3.0 it's possible to contruct an Encoder with width or height 0, + // but encoding will fail and the error is really uninformative: + // "called `Result::unwrap()` on an `Err` value: VP8_ENC_ERROR_BAD_DIMENSION" + + //panic!("Width and height must be non-zero."); + } + + // We're going to compare the incoming width * height * bpp against the length of a slice. + // The length of a slice is a `usize`, so we are going to do arithmetic in `usize` as well. + // + // On 32-bit and 64-bit platforms these conversions always suceeed and get optimized out. + let width_u: usize = width.try_into().unwrap(); + let height_u: usize = height.try_into().unwrap(); + let bytes_per_pixel_u: usize = layout.bytes_per_pixel().into(); + + // If we simply calculate `width * height * bytes_per_pixel`, arithmetic may overflow in release mode + // and make it possible to bypass the length check. So we need either .checked_mul or .saturating_mul here. + // + // Using `saturating_mul` is enough because it's not possible to construct a valid slice of size `usize::MAX` anyway, + // and it makes for simpler code and a nicer error message. + let expected_len = width_u + .saturating_mul(height_u) + .saturating_mul(bytes_per_pixel_u); + if image.len() < expected_len { + panic!( + "Image buffer too small. Expected at least {} bytes for a {}x{} image with {:?} layout, got {}.", + expected_len, width, height, layout, image.len() + ); + } + + if image.len() > expected_len { + // TODO: reject this in the future, once this function is made to return Result + } + + CheckedEncoder { + image, + layout, + width, + height, + } + } + + pub(super) fn width(&self) -> u32 { + self.width + } + + pub(super) fn height(&self) -> u32 { + self.height + } + + pub(super) fn layout(&self) -> PixelLayout { + self.layout + } + + pub(super) fn image(&self) -> &'a [u8] { + self.image } } } @@ -141,14 +221,20 @@ mod tests { use super::*; use crate::shared; + #[test] + #[should_panic] + fn construct_encoder_with_buffer_overflow() { + Encoder::from_rgb(&[], 16383, 16383); + } + #[test] fn test_encoder_new_assigns_fields() { - let data = [1, 2, 3, 4, 5, 6]; + let data = [5; 18]; let enc = Encoder::new(&data, shared::PixelLayout::Rgb, 2, 3); - assert_eq!(enc.image, &data); - assert_eq!(enc.layout, shared::PixelLayout::Rgb); - assert_eq!(enc.width, 2); - assert_eq!(enc.height, 3); + assert_eq!(enc.e.image(), &data); + assert_eq!(enc.e.layout(), shared::PixelLayout::Rgb); + assert_eq!(enc.e.width(), 2); + assert_eq!(enc.e.height(), 3); } #[test] @@ -157,12 +243,12 @@ mod tests { let rgba = [1, 2, 3, 4, 5, 6, 7, 8]; let enc_rgb = Encoder::from_rgb(&rgb, 2, 1); let enc_rgba = Encoder::from_rgba(&rgba, 2, 1); - assert_eq!(enc_rgb.layout, shared::PixelLayout::Rgb); - assert_eq!(enc_rgba.layout, shared::PixelLayout::Rgba); - assert_eq!(enc_rgb.image, &rgb); - assert_eq!(enc_rgba.image, &rgba); - assert_eq!(enc_rgb.width, 2); - assert_eq!(enc_rgba.height, 1); + assert_eq!(enc_rgb.e.layout(), shared::PixelLayout::Rgb); + assert_eq!(enc_rgba.e.layout(), shared::PixelLayout::Rgba); + assert_eq!(enc_rgb.e.image(), &rgb); + assert_eq!(enc_rgba.e.image(), &rgba); + assert_eq!(enc_rgb.e.width(), 2); + assert_eq!(enc_rgba.e.height(), 1); } #[cfg(feature = "img")] diff --git a/src/shared.rs b/src/shared.rs index 232fd48..6381923 100644 --- a/src/shared.rs +++ b/src/shared.rs @@ -139,6 +139,13 @@ impl PixelLayout { pub fn is_alpha(self) -> bool { self == PixelLayout::Rgba } + + pub fn bytes_per_pixel(self) -> u8 { + match self { + PixelLayout::Rgb => 3, + PixelLayout::Rgba => 4, + } + } } #[cfg(test)]