From 798a37d729a9e8690d4517c350ec2a7272dc01d5 Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Mon, 23 Jun 2025 01:04:40 +0100 Subject: [PATCH 01/10] Add bytes_per_pixel function on PixelLayout --- src/shared.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/shared.rs b/src/shared.rs index 39c3082..0432366 100644 --- a/src/shared.rs +++ b/src/shared.rs @@ -135,4 +135,11 @@ 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, + } + } } From 9b318cc04e4ab0c9c734f293070c6788579484c9 Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Mon, 23 Jun 2025 01:05:18 +0100 Subject: [PATCH 02/10] Add a private CheckedEncoder function that's not constructable from outside of a private module --- src/encoder.rs | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/src/encoder.rs b/src/encoder.rs index ee1f915..d927246 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -96,6 +96,85 @@ impl<'a> Encoder<'a> { } } +/// 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; + + /// Encoder paramters that were validated on creation + 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 { + // 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(); + + 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 { + // Warn or error if there's extra data that isn't a full pixel, + // depending on strictness. For now, let's allow larger buffers + // as long as they contain enough data. + } + + 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 + } + + } +} + pub(crate) unsafe fn new_picture( image: &[u8], layout: PixelLayout, From c50edfac0400b078489ab87d08291a6a79bb785d Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Mon, 23 Jun 2025 01:09:09 +0100 Subject: [PATCH 03/10] Add test validating the presence of buffer overflow checks --- src/encoder.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/encoder.rs b/src/encoder.rs index d927246..b0b24e8 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -215,3 +215,14 @@ unsafe fn encode( Err(picture.error_code) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + #[should_panic] + fn construct_encoder_with_buffer_overflow() { + Encoder::from_rgb(&[], 16383, 16383); + } +} \ No newline at end of file From 14631824205315e5455d5d783759c36988bda8da Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Mon, 23 Jun 2025 01:16:19 +0100 Subject: [PATCH 04/10] Replace the fields of Encoder with a CheckedEncoder --- src/encoder.rs | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/encoder.rs b/src/encoder.rs index b0b24e8..8868e75 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -3,25 +3,23 @@ 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> { /// Creates a new encoder from the given image data. /// 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 { + Self { e: CheckedEncoder::new( image, layout, width, height, - } + )} } #[cfg(feature = "img")] @@ -46,22 +44,22 @@ 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 { + Self { e: CheckedEncoder::new( image, - layout: PixelLayout::Rgb, + 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 { + Self { e: CheckedEncoder::new( image, - layout: PixelLayout::Rgba, + PixelLayout::Rgba, width, height, - } + )} } /// Encode the image with the given quality. @@ -89,7 +87,7 @@ 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); + let mut picture = new_picture(self.e.image(), self.e.layout(), self.e.width(), self.e.height()); let res = encode(&mut *picture, config); res } @@ -103,7 +101,8 @@ mod internal { use crate::shared::PixelLayout; - /// Encoder paramters that were validated on creation + /// 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, From dae0f607519b4a4158e8708c2d53e50b25b0de37 Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Mon, 23 Jun 2025 01:22:02 +0100 Subject: [PATCH 05/10] Add comment --- src/encoder.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/encoder.rs b/src/encoder.rs index 8868e75..a4415a6 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -133,6 +133,11 @@ mod internal { 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!( From d68c1ff35aca43a58309dfc0df1f4f975c90117c Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Mon, 23 Jun 2025 01:22:55 +0100 Subject: [PATCH 06/10] Turn a comment into an actionable TODO --- src/encoder.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/encoder.rs b/src/encoder.rs index a4415a6..cc37f88 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -147,9 +147,7 @@ mod internal { } if image.len() > expected_len { - // Warn or error if there's extra data that isn't a full pixel, - // depending on strictness. For now, let's allow larger buffers - // as long as they contain enough data. + // TODO: reject this in the future, once this function is made to return an error } CheckedEncoder { From 3bfda2f2810803c011954c99d62d4df0f1dd2a6b Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Mon, 23 Jun 2025 01:29:11 +0100 Subject: [PATCH 07/10] Clearly document another TODO --- src/encoder.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/encoder.rs b/src/encoder.rs index cc37f88..7d40c62 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -121,9 +121,14 @@ mod internal { height: u32, ) -> Self { // TODO: return an error instead of panicking in the next semver-breaking release - // if width == 0 || height == 0 { - // panic!("Width and height must be non-zero."); - // } + 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. @@ -147,7 +152,7 @@ mod internal { } if image.len() > expected_len { - // TODO: reject this in the future, once this function is made to return an error + // TODO: reject this in the future, once this function is made to return Result } CheckedEncoder { From 02a7de66e70ad87bcfbaa910c5ef46f23439dcb7 Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Mon, 23 Jun 2025 01:29:36 +0100 Subject: [PATCH 08/10] cargo fmt --- examples/convert_by_args.rs | 5 +--- src/encoder.rs | 51 ++++++++++++++++--------------------- 2 files changed, 23 insertions(+), 33 deletions(-) diff --git a/examples/convert_by_args.rs b/examples/convert_by_args.rs index d979dae..8e9d529 100644 --- a/examples/convert_by_args.rs +++ b/examples/convert_by_args.rs @@ -2,10 +2,8 @@ use image::*; use std::{env::args, fmt::format, path::Path}; use webp::*; - /// cargo run --example convert_by_args lake.jpg. fn main() { - //Add a get args functions let arg: Vec = args().collect(); if arg.len() != 2 { @@ -17,7 +15,6 @@ fn main() { let path = format(format_args!("assets/{}", arg[1])); let path = Path::new(&path); - // Using `image` crate, open the included .jpg file let img = image::open(path).unwrap(); let (w, h) = img.dimensions(); @@ -62,4 +59,4 @@ fn test_convert() { // Define and write the WebP-encoded file to a given path let output_path = Path::new("assets").join("lake").with_extension("webp"); std::fs::write(&output_path, &*webp).unwrap(); -} \ No newline at end of file +} diff --git a/src/encoder.rs b/src/encoder.rs index 7d40c62..dfd9681 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -14,12 +14,9 @@ impl<'a> Encoder<'a> { /// Creates a new encoder from the given image data. /// 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 { e: CheckedEncoder::new( - image, - layout, - width, - height, - )} + Self { + e: CheckedEncoder::new(image, layout, width, height), + } } #[cfg(feature = "img")] @@ -44,22 +41,16 @@ 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 { e: CheckedEncoder::new( - image, - PixelLayout::Rgb, - width, - height, - )} + Self { + 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 { e: CheckedEncoder::new( - image, - PixelLayout::Rgba, - width, - height, - )} + Self { + e: CheckedEncoder::new(image, PixelLayout::Rgba, width, height), + } } /// Encode the image with the given quality. @@ -87,7 +78,12 @@ impl<'a> Encoder<'a> { pub fn encode_advanced(&self, config: &WebPConfig) -> Result { unsafe { - let mut picture = new_picture(self.e.image(), self.e.layout(), self.e.width(), self.e.height()); + let mut picture = new_picture( + self.e.image(), + self.e.layout(), + self.e.width(), + self.e.height(), + ); let res = encode(&mut *picture, config); res } @@ -112,14 +108,10 @@ mod internal { 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 + 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. @@ -143,7 +135,9 @@ mod internal { // // 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); + 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 {}.", @@ -178,7 +172,6 @@ mod internal { pub(super) fn image(&self) -> &'a [u8] { self.image } - } } @@ -232,4 +225,4 @@ mod tests { fn construct_encoder_with_buffer_overflow() { Encoder::from_rgb(&[], 16383, 16383); } -} \ No newline at end of file +} From dc4b5689e0725d5792945e47bd161c2b366550c1 Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Tue, 26 Aug 2025 22:27:27 +0100 Subject: [PATCH 09/10] update tests --- src/encoder.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/encoder.rs b/src/encoder.rs index 962983a..e9b70f1 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -231,10 +231,10 @@ mod tests { fn test_encoder_new_assigns_fields() { let data = [1, 2, 3, 4, 5, 6]; 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] @@ -243,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")] From 7cf6d15a82dfd3d260e95fa327578069b44b1de3 Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Tue, 26 Aug 2025 22:47:09 +0100 Subject: [PATCH 10/10] Fix invalid input parameters in a test --- src/encoder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/encoder.rs b/src/encoder.rs index e9b70f1..2d0e69a 100644 --- a/src/encoder.rs +++ b/src/encoder.rs @@ -229,7 +229,7 @@ mod tests { #[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.e.image(), &data); assert_eq!(enc.e.layout(), shared::PixelLayout::Rgb);