rust-secure-code / safety-dance

Auditing crates for unsafe code which can be safely replaced
Apache License 2.0
536 stars 10 forks source link

Audit image crate #3

Closed Shnatsel closed 5 years ago

Shnatsel commented 5 years ago

https://crates.io/crates/image

Image manipulation, abstraction over image formats, some image format parsers. 1500 downloads/day. Contains unsafe code, which is notoriously hard to get right in binary format parsers.

Shnatsel commented 5 years ago

Done so far: https://github.com/image-rs/image/pull/985

64 commented 5 years ago

In fact the unsafe in image is minimal. Aside from four non-bounds checked functions (correctly marked as unsafe, not even used internally and deprecated), there is one other usage of unsafe to date:

fn from_slice(slice: &[T]) -> &$ident<T> {
        assert_eq!(slice.len(), $channels);
        unsafe { &*(slice.as_ptr() as *const $ident<T>) }
    }
    fn from_slice_mut(slice: &mut [T]) -> &mut $ident<T> {
        assert_eq!(slice.len(), $channels);
        unsafe { &mut *(slice.as_ptr() as *mut $ident<T>) }
    }

I don’t think there is another good way to do this yet in Rust.


As a side note, the imageproc crate contains large amounts of unsafe, mostly unchecked accesses. I suspect a lot of code in there could be rewritten with iterators to avoid bounds checks, so that might be of interest.

Shnatsel commented 5 years ago

No clear improvement to Rust can be made to remove the need for these blocks either, so closing as done. Thanks for the analysis and for https://github.com/image-rs/image/issues/980!

Lokathor commented 5 years ago

The general concept of slice casting is possible and can be implemented as a library which image then uses, so that's one approach we might get people to sign on for.

Shnatsel commented 5 years ago

Could https://crates.io/crates/zerocopy help?

64 commented 5 years ago

On second thoughts, I’m pretty sure this is possible with TryFrom.

EDIT: On third thoughts, TryFrom would do &[T] -> &[T; N] but there isn’t a safe way to get from that to the desired struct. However TryFrom + some cast/transmute may still be preferable as the checking is done by the standard library