Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic when an animated webp image is loaded #1775

Closed
PhilipK opened this issue Aug 22, 2022 · 4 comments · Fixed by #1823
Closed

Panic when an animated webp image is loaded #1775

PhilipK opened this issue Aug 22, 2022 · 4 comments · Fixed by #1823

Comments

@PhilipK
Copy link

PhilipK commented Aug 22, 2022

This happens in BoilR
PhilipK/BoilR#203

Whenever an animated webp image is loaded it will lead to a panic:

Expected

Expected the image to load a static image, or just give an Result::Error instead of panic

Actual behaviour

A panic is thrown

Reproduction steps

Download an animated image, eaxmple: https://cdn2.steamgriddb.com/file/sgdb-cdn/logo/42cd4e870371f116e5695912004e1b5a.webp
Load the image with the following code:

  #[test]
    pub fn image_decode_webp_animated() {
        let path = std::path::Path::new("src/testdata/hollow.webp");
        if let Ok(reader) = image::io::Reader::open(path){
            //This should give an error but instead panics
            if let Ok(image)  = reader.decode(){
                println!("It worked");
            }
        }
    }

The code will now panic

@Denzo77
Copy link

Denzo77 commented Nov 5, 2022

I have a similar but subtley different issue (running v0.24.4, but it looks like this still applies to the patch above). The reproduction steps are the same, but I think it's specific to animated webps with no transparency such as this example:
https://www.tutorialexample.com/wp-content/uploads/2020/09/gif-test-2.webp

I get the following panic:

thread 'main' panicked at 'source slice length (750000) does not match destination slice length (1000000)', .cargo/registry/src/github.com-1ecc6299db9ec823/image-0.24.4/./src/codecs/webp/extended.rs:462:21

The slice lengths looked suspiciously like it's trying to copy RGB values into an RGBA buffer and panicking because the lengths don't match. Inspecting the header with webpinfo, the VP8X header has Alpha set to true but the actual frames all have it set to false.

$ webpinfo resources/gif-test-2.webp
File: resources/gif-test-2.webp
RIFF HEADER:
  File size: 813092
Chunk VP8X at offset     12, length     18
  ICCP: 0
  Alpha: 1   <=== TRUE HERE
  EXIF: 0
  XMP: 0
  Animation: 1
  Canvas size 500 x 500
Chunk ANIM at offset     30, length     14
  Background color:(ARGB) 00 18 18 18
  Loop count      : 0
Chunk ANMF at offset     44, length  15832
  Offset_X: 0
  Offset_Y: 0
  Width: 500
  Height: 500
  Duration: 0
  Dispose: 0
  Blend: 1
Chunk VP8  at offset     68, length  15808
  Width: 500
  Height: 500
  Alpha: 0   <=== FALSE HERE
  Animation: 0
  Format: Lossy (1)

...

I ran a debugger and it seems like the codec reads the VP8X header and assumes the frames have an alpha channel, but the frames encode that individually. If I set a breakpoint here and set info.alpha to false, everything works as expected.
https://github.com/image-rs/image/blob/master/src/codecs/webp/decoder.rs#L177

@mattfbacon
Copy link
Contributor

diff --git a/src/codecs/webp/extended.rs b/src/codecs/webp/extended.rs
index 8ae61a9..a7868d9 100644
--- a/src/codecs/webp/extended.rs
+++ b/src/codecs/webp/extended.rs
@@ -249,14 +249,20 @@ impl ExtendedImage {
         anim_image: &AnimatedFrame,
         background_color: Rgba<u8>,
     ) -> Option<ImageResult<Frame>> {
-        let mut buffer = vec![0; (anim_image.width * anim_image.height * 4) as usize];
+        let mut buffer = vec![0; anim_image.image.get_buf_size()];
         anim_image.image.fill_buf(&mut buffer);
+        let has_alpha = match &anim_image.image {
+            WebPStatic::LossyWithAlpha(..) => true,
+            WebPStatic::LossyWithoutAlpha(..) => false,
+            WebPStatic::Lossless(..) => true,
+        };
+        let pixel_len = if has_alpha { 4 } else { 3 };

         for x in 0..anim_image.width {
             for y in 0..anim_image.height {
                 let canvas_index: (u32, u32) = (x + anim_image.offset_x, y + anim_image.offset_y);
-                let index: usize = (y * 4 * anim_image.width + x * 4).try_into().unwrap();
-                canvas[canvas_index] = if anim_image.use_alpha_blending {
+                let index: usize = ((y * anim_image.width + x) * pixel_len).try_into().unwrap();
+                canvas[canvas_index] = if anim_image.use_alpha_blending && has_alpha {
                     let buffer: [u8; 4] = buffer[index..][..4].try_into().unwrap();
                     ExtendedImage::do_alpha_blending(buffer, canvas[canvas_index])
                 } else {
@@ -264,7 +270,7 @@ impl ExtendedImage {
                         buffer[index],
                         buffer[index + 1],
                         buffer[index + 2],
-                        buffer[index + 3],
+                        if has_alpha { buffer[index + 3] } else { 255 },
                     ])
                 };
             }

@wez
Copy link

wez commented Mar 15, 2023

I'm running into this with 0.24.5. I can see that this is fixed in your main line. Is there an ETA for making a release with this fix?

wez added a commit to wez/wezterm that referenced this issue Mar 15, 2023
Use mainline image crate to avoid an otherwise unavoidable panic
in the upstream: image-rs/image#1775

Explicitly operate on the frames from the animation.

refs: #3250
@fintelia
Copy link
Contributor

Thanks for the reminder. I'll try to get out a release this weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants