search

netvl / immeta

Image metadata inspection library in Rust
MIT License
24 stars 18 forks source link

Panic in jpeg format loading cnn.com in Servo #8

Closed jdm closed 8 years ago

jdm commented 8 years ago 
thread 'ImageCacheThread' panicked at 'arithmetic operation overflowed', /Users/jdm/.cargo/registry/src/github.com-1ecc6299db9ec823/immeta-0.3.2/src/formats/jpeg.rs:37
stack backtrace:
   1:        0x10ac3df18 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
   2:        0x10ac443a5 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
   3:        0x10ac43fbe - std::panicking::default_hook::hc2c969e7453d080c
   4:        0x10a119e19 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h241fdc1bd2e9f090
   5:        0x10a119cb3 - _<std..thread..LocalKey<T>>::with::h7b8c266a788a1380
   6:        0x10a119b0a - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h49531732a5b6b1e1
   7:        0x10ac2bb52 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
   8:        0x10ac44966 - std::panicking::begin_panic::h4889569716505182
   9:        0x10ac2d448 - std::panicking::begin_panic_fmt::h484cd47786497f03
  10:        0x10ac445bf - rust_begin_unwind
  11:        0x10ac6bdb0 - core::panicking::panic_fmt::h257ceb0aa351d801
  12:        0x10ac6c0ac - core::panicking::panic::h4bb1497076d04ab9
  13:        0x10869cb02 - _<formats..jpeg..Metadata as traits..LoadableMetadata>::load::h1e940b8d58d4471e
  14:        0x10867a000 - immeta::generic::load::hecc3105c1cd6a3b3
  15:        0x10867978b - immeta::generic::load_from_buf::h99f4a09595977935
  16:        0x10855a5e3 - net::image_cache_thread::ImageCache::handle_progress::hb85efed105d4ae3a
  17:        0x108540fe0 - net::image_cache_thread::ImageCache::run::h8a7f9c4cea47c0ce
  18:        0x1085942c4 - net::image_cache_thread::new_image_cache_thread::_$u7b$$u7b$closure$u7d$$u7d$::hd955388743fd3934
  19:        0x1085941c3 - _<std..panic..AssertUnwindSafe<F> as std..ops..FnOnce<()>>::call_once::he46335d08fc20bad
  20:        0x10859410b - std::panicking::try::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h47315e3d894b7f3e
  21:        0x108594834 - std::panicking::try::call::h79a9ea3aa322847c
  22:        0x10ac47b2b - __rust_try
  23:        0x10ac47ac5 - __rust_maybe_catch_panic
  24:        0x10859405d - std::panicking::try::_$u7b$$u7b$closure$u7d$$u7d$::hdd5242960360b596
  25:        0x108593f9f - _<std..thread..LocalKey<T>>::with::h1ed8404e3e27cf1e
  26:        0x108593ded - std::panicking::try::h4b8e2e54c75fdc17
  27:        0x108593cca - std::panic::catch_unwind::h7a980a0a21f064ec
  28:        0x108593b26 - std::thread::Builder::spawn::_$u7b$$u7b$closure$u7d$$u7d$::h87e5067523346194
  29:        0x108594a2c - _<F as std..boxed..FnBox<A>>::call_box::ha6e60ff952ac3d25
  30:        0x10ac433c8 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  31:     0x7fff8f94a898 - _pthread_body
  32:     0x7fff8f94a729 - _pthread_start

This is https://github.com/netvl/immeta/blob/f06b1fd1fc13184cf2834e009853117b7c557a74/src/formats/jpeg.rs#L37 . Presumably we end up with 1 or 0.

netvl commented 8 years ago

Wow, thanks! I'll investigate it ASAP. Is there a direct link to the offending image?

jdm commented 8 years ago

Not yet; sorry.

jdm commented 8 years ago

The image appears to be data:image/webp;base64,UklGRkoAAABXRUJQVlA4WAoAAAAQAAAAAAAAAAAAQUxQSAwAAAABBxAR/Q9ERP8DAABWUDggGAAAADABAJ0BKgEAAQADADQlpAADcAD++/1QAA== interestingly enough.

jdm commented 8 years ago

The jpeg loader was only tried because the webp one didn't return an Ok value, apparently.

jdm commented 8 years ago 
&['R', 'I', 'F', 'F', 'J', '\0', '\0', '\0', 'W', 'E', 'B', 'P', 'V', 'P', '8', 'X', '\n', '\0', '\0', '\0', '\x10', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', '\0', 'A', 'L', 'P', 'H', '\f', '\0', '\0', '\0', '\x01', '\a', '\x10', '\x11', '�', '\x0f', 'D', 'D', '�', '\x03', '\0', '\0', 'V', 'P', '8', ' ', '\x18', '\0', '\0', '\0', '0', '\x01', '\0', '\x9d', '\x01', '*', '\x01', '\0', '\x01', '\0', '\x03', '\0', '4', '%', '�', '\0', '\x03', 'p', '\0', '�', '�', '�', 'P', '\0']
netvl commented 8 years ago

@jdm Thanks! Indeed this could happen, webp support is not full yet. Apparently it has VP8X chunk id which is not supported now. Do you want a quick fix which disables webp support for now or are you willing to wait until I add support for other webp chunk formats? I'll start working on it right away in the latter case, will probably finish it right after the weekend.

jdm commented 8 years ago

Perhaps make webp support a feature that can be enabled by users of the library? It would still be useful to harden the jpeg decoder to avoid this, too.

netvl commented 8 years ago

a feature

This somehow didn't even occur to me. Indeed, that would be a nice solution, thanks!

It would still be useful to harden the jpeg decoder to avoid this, too.

Yes, I agree. Unfortunately, JPEG is such a loose format that it is pretty hard to do it... I'll see what I can do.

netvl commented 8 years ago

Apparently I was wrong about the looseness of the JPEG format. I finally have found its specification, and I was able to improve the JPEG parser considerably. It does not contain the overflow problem anymore, and it also provides more pieces of metadata. See version 0.3.4.

immeta will now return "unsupported image format" error for the image in question. @jdm please tell if this is an acceptable behavior.

jdm commented 8 years ago

Perfect!

netvl commented 8 years ago

Okay, then I'll close this. Thanks!