image-rs / image

Encoding and decoding images in Rust
Apache License 2.0
4.74k stars 587 forks source link

Certain JPEGs cause assertion failure in zune-jpeg #2230

Open zphixon opened 1 month ago

zphixon commented 1 month ago

This happens in 0.25.1


Panic on unwrap

Actual behaviour

Assertion failure within the jpeg decoder

thread 'main' panicked at /home/zack/.cargo/registry/src/
assertion `left == right` failed
  left: 13824
 right: 6912
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/std/src/
   1: core::panicking::panic_fmt
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/
   2: core::panicking::assert_failed_inner
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/
   3: core::panicking::assert_failed
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/
   4: zune_jpeg::worker::upsample
             at /home/zack/.cargo/registry/src/
   5: zune_jpeg::mcu::<impl zune_jpeg::decoder::JpegDecoder<T>>::post_process
             at /home/zack/.cargo/registry/src/
   6: zune_jpeg::mcu::<impl zune_jpeg::decoder::JpegDecoder<T>>::decode_mcu_ycbcr_baseline
             at /home/zack/.cargo/registry/src/
   7: zune_jpeg::decoder::JpegDecoder<T>::decode_into
             at /home/zack/.cargo/registry/src/
   8: <image::codecs::jpeg::decoder::JpegDecoder<R> as image::image::ImageDecoder>::read_image
             at /home/zack/.cargo/registry/src/
   9: <image::codecs::jpeg::decoder::JpegDecoder<R> as image::image::ImageDecoder>::read_image_boxed
             at /home/zack/.cargo/registry/src/
  10: <alloc::boxed::Box<T> as image::image::ImageDecoder>::read_image
             at /home/zack/.cargo/registry/src/
  11: image::image::decoder_to_vec
             at /home/zack/.cargo/registry/src/
  12: image::dynimage::decoder_to_image
             at /home/zack/.cargo/registry/src/
  13: image::dynimage::DynamicImage::from_decoder
             at /home/zack/.cargo/registry/src/
  14: image::io::reader::Reader<R>::decode
             at /home/zack/.cargo/registry/src/
  15: what::main
             at ./src/
  16: core::ops::function::FnOnce::call_once
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/ops/

Reproduction steps

Hello! I ran into this issue: which is supposed to be fixed in 0.5.0-rc0. I've asked if it's possible to provide a 0.4 version with the assertion removed.

use image::io::Reader;

fn main() {
    let img = Reader::open("sadcomp.jpg")

This is sadcomp.jpg. Uploading to github doesn't seem to have changed the behavior.


(I've had this image since 2020, I promise I didn't mess with it on purpose :sweat_smile:)

izderadicka commented 1 month ago

I can confirm this problem - after update to 0.25 (which switched to to zune as default jpeg backend) I see assertion panic for several jpeg images. zune has this issue

Looks like solution is upgrade to zune 0.5 ...

For me it's quite serious problem - using library for icons creation in web server so panic is no go.

fintelia commented 1 month ago

Sorry that this bug is proving disruptive. Upstream has indicated that it'll be fixed in their next release, and we already have #2198 ready for upgrading to it. In the meantime, there isn't much more to be done on our end. You might be able to work around it using catch_unwind though.

As far as using using this crate for user input on a web server, I will caution that the code hasn't been fully hardened against malicious input. Over the last few years we've used fuzzing to significantly cut down on the number of places that can trigger panics from malicious input, but that work is not yet complete.

Finally, in light of some of the discussion on linked issues, I want to remind everyone to engage respectfully. These crates are all maintained by volunteers. You aren't owed a release on any particular schedule.

izderadicka commented 1 month ago

@fintelia - Thanks for reply - for now I've downgraded to 0.24 - which does not have these issues. Actually hyper or tokio is using catch unwind, so it only kills one of tokio worker threads (which I believe is then replaced), but anyhow panic is no good.

I definitely did not want to be pushy, I understand the burden of maintaining an opensource project, big thanks to all of image and zune contributors.

I just wanted to stress, that jpegs, that caused panic are generally common. It was not specially crafted jpeg - I do have some collection of downloaded covers and problem was with notable fraction of these - like %5 - so just wanted to highlight the importance of the issue.

fintelia commented 1 month ago

Yeah, unfortunately that's kind of common with image files. Some specific encoder might consistently generate weird files that all fail the same way, while images produced by other encoders might never fail that way. The zune-jpeg crate was tested on a corpus of ~40,000 images but evidently it either regressed or no images like yours were included

mqudsi commented 1 week ago

zune-jpeg has several functional regressions (in the form of full-blown panics) that present when decoding jpegs in image 0.25 that the zune author has patched in the api-incompatible 0.5 release but has thus far not responded to requests for backports for 0.4 that image uses (one of these is a one-line patch to the current 0.4 zune-jpeg release).

Could there be some sort of discussion/agreement between the authors of image and zune to get backported fixes for critical issues like this that effectively completely block users from upgrading to the current image version or force them to depend on multiple versions of image (and its transitive dependencies) to work around semver issues?

Otherwise, is there any consideration for using a patched/forkedf version of zune-jpeg for a new image point release until a release with full zune-jpeg 0.5 support is issued?

image is, at this point, a foundational crate in the rust ecosystem and it is really sad to see a major release completely broken by faulty assertions in a core transitive dependency that have already been patched upstream.