Undefined behavior detected by miri

[dodomorandi]

I just discovered an UB whilst testing another project with miri. Primitive::from_slice and Primitive::from_slice_mut perform an invalid unsafe cast, which is detected by miri.

EDIT: #1358 showed that it is just a matter of a wrong as_ptr fn, therefore Primitive::from_slice is likely to be unaffected.

Expected

cargo +nightly miri test should not raise any problem.

Actual behaviour

test buffer_::test::mut_iter triggers UB.

Reproduction steps

Just run miri on tests ☺️

[197g]

Did you find any other miri failures? Running all tests takes a long time and uses quite a lot of memory :)

[dodomorandi]

I did not try, but I am trying now to run all the tests with miri. 😉

[dodomorandi]

Sorry if I did not reply, but it looks like using miri on some test is hardly doable: I went OOM with 16GiB of RAM, then asked for help to @lu-zero, and it nearly OOM with 32GiB of RAM. He is going to try on another machine with 64GiB, let's see if it helps... Ouch, Miri is still unavailable for aarch64-unknown-linux-gnu. Still trying on the 32GiB machine with some swap...

[lu-zero]

So far codecs::png::tests::ensure_no_decoder_off_by_one is a bit outrageous

 8334 lu_zero   20   0   47.3g  30.6g    856 S 100.0  97.7 204:27.42 miri

[nabijaczleweli]

I have a 96G amd64 machine, I'll spin the tests there as well.

[197g]

It also shouldn't be strictly necessary to run all tests. Only those which eventually might call into unsafe code might yield interesting results in miri, apart from bugs in the compiler or standard library. I'm sure many tests are very verbose in this regard, in particular the reference image rendering test suite test a miniscule amount of unsafe code as many decoders are entirely safe.

[nabijaczleweli]

After 4h20 of CPU time (plus a single restart to add -Zmiri-disable-isolation 40 minutes in :v), I'm at 43/137 tests with miri. I'd rather not restart this for reasons I assume obvious, and it's currently at 52.5G resident set, so I can hardly run another set of tests in parallel (which miri is not doing for some reason, so those four hours are on one out of the twenty four CPUs, I'm pretty sure, which kinda sucks since they're relatively slow).

If there's an easy way to find out which tests resolve to unsafe blocks, as you say, then I'll be happy to restrict to those tests in future, but I don't know of one nor particularly feel like going through over a hundred tests when I can just let'er rip overnight instead tbh.

[dodomorandi]

@HeroicKatora You are totally right, I am guilty of being lazy not looking for unsafe code to test with miri 😅

Lets' summarize:

• buffer_::test::mut_iter already detected the issue with Primitive::from_slice_mut
• buffer_::benchmarks::test_image_buffer_copy_within_ (using --features benchmark) can be used to check buffer::slice_copy_within, and it passes flawlessly
• buffer_::test::get_pixel can be used to test Primitive::from_slice, and it passes flawlessly.

Is there anything I am missing? Otherwise we can close this 🎉

[lu-zero]

For those curious, codecs::png::tests::ensure_no_decoder_off_by_one is still going and we are at

1632769 root      20   0   87.6g  77.5g 142272 S 100.0  30.5 403:09.62 miri

[197g]

@dodomorandi Thanks for investigating. That should indeed cover most of it. There are some uses of bytemuck that are scary in the implementation but which we have determined to trust on int-to-byte transmutes.

I've tried running siderophile. It's not entirely accurate as it is using geiger which notoriously undercounts unsafe coming from macros but it should give a somewhat sensible first impression. Note that in particular the top results are entirely due to bytemuck if I'm not mistaken. Contrast this with hdr, gif, webp which are due to byteorder. Anyways, the full report is this:

$ ~/git/siderophile/target/release/siderophile --crate-name image
Badness  Function
    076  image::image::decoder_to_vec
    062  image::dynimage::DynamicImage::from_decoder
    062  image::dynimage::decoder_to_image
    020  image::io::free_functions::load
    010  <image::codecs::tiff::TiffDecoder<R> as ImageDecoder>::read_image
    010  image::dynimage::load_from_memory
    010  image::io::free_functions::open_impl
    003  image::utils::vec_copy_to_u8
    002  <image::codecs::gif::GifDecoder<R> as ImageDecoder>::read_image
    002  <image::codecs::hdr::decoder::HdrAdapter<R> as ImageDecoder>::read_image
    002  image::codecs::gif::GifDecoder<R>::new
    002  image::codecs::hdr::decoder::HdrAdapter<R>::read_image_data
    002  image::codecs::hdr::decoder::HdrDecoder<R>::read_image_ldr
    002  image::codecs::hdr::decoder::HdrDecoder<R>::read_image_transform
    002  image::codecs::hdr::decoder::HdrDecoder<R>::read_image_transform::{{closure}}
    002  image::codecs::webp::decoder::WebPDecoder<R>::new
    002  image::codecs::webp::decoder::WebPDecoder<R>::read_frame
    002  image::codecs::webp::decoder::WebPDecoder<R>::read_metadata
    002  image::io::free_functions::image_dimensions_impl
    002  image::io::free_functions::image_dimensions_with_format_impl
    001  <image::codecs::tiff::TiffEncoder<W> as ImageEncoder>::write_image
    001  image::codecs::tiff::TiffEncoder<W>::encode
    001  image::codecs::tiff::u8_slice_as_u16
    001  image::codecs::webp::vp8::Vp8Decoder<R>::decode_frame
    001  image::codecs::webp::vp8::Vp8Decoder<R>::init_partitions
    001  image::codecs::webp::vp8::Vp8Decoder<R>::read_frame_header
    001  image::dynimage::DynamicImage::as_bytes
    001  image::dynimage::DynamicImage::into_bytes
    001  image::dynimage::DynamicImage::to_bytes
    001  image::dynimage::image_as_bytes
    001  image::dynimage::image_into_bytes
    001  image::dynimage::image_to_bytes
    001  image::io::free_functions::save_buffer_impl
    001  image::io::free_functions::save_buffer_with_format_impl
    001  image::utils::vec_u16_into_u8
    001  image::utils::vec_u32_into_u8
    001  image::utils::vec_u64_into_u8

Since running MIRI on all tests seems to be incredibly resource intensive, maybe we could work on a dedicate test suite that targets these functions in particular and directly.

[lu-zero]

I can tell you that codecs::png::tests::ensure_no_decoder_off_by_one is probably showing a pathological case in miri.

1632769 root      20   0  118.3g 107.5g 142272 S 100.0  42.4 960:43.10 miri

[dodomorandi]

@HeroicKatora I understand what you are saying, but on the other hand I would like to emphasize the good work your are doing in image-rs: there are only three occurrences of unsafe usage in the entire crate (unsafe fn that are not actively used do not count IMHO).

If a safe function in an external crate triggers UB, I totally understand that you can think "it's our fault, we chose that crate", but on the other hand you also cannot test the soundness of each dependency from image-rs. A good approach would hopefully be to track down which unsafe fn are called at the end of the road, in order to check whether they are tested enough and if there is a valid reason for them to be unsafe.

Let's take image::image::decoder_to_vec for instance: the function do not directly use unsafe, if it triggers UB it could be a problem of bytemuck, num_traits or another function in image-rs (total_bytes, read_image or something downstream). Unfortunately siderophile (which I was unaware of, thank you for letting me know about it!) does not show which unsafe fns are called downstream, which makes unsafe tracking pretty hard. On the other hand I can see miri works on all tests of bytemuck, and it is pretty fast to run -- just some seconds. Obviously it would be necessary to analyze code coverage for the tests in order the check whether they are good enough, but it is a huge amount of work, especially if you are going to do the same thing on each dependency!

TL;DR: Using miri to check for UB correctness for all the project would be wonderful, but it looks like we are not still there. Manually tracking UBs requires huge efforts, I think that the best thing that can be done is cooperate with other crates' maintainers, especially in case of strange_ issues (like #1359). Moreover, when working with images and media in general, logic errors (i.e. wrapping behavior instead of saturating) are not safety issues but still very problematic, tools like miri won't help with these. Just assume that you safe code is really safe, otherwise you cannot rely on Rust safety abstractions 😊