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

Audit image crate #3

Closed
Shnatsel opened this issue Jul 21, 2019 · 6 comments
Closed

Audit image crate #3

Shnatsel opened this issue Jul 21, 2019 · 6 comments

Comments

@Shnatsel
Copy link
Member

Shnatsel commented Jul 21, 2019

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
Copy link
Member Author

Shnatsel commented Jul 21, 2019

Done so far:
image-rs/image#985

@64
Copy link

64 commented Jul 21, 2019

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
Copy link
Member Author

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 image-rs/image#980!

@Lokathor
Copy link
Contributor

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
Copy link
Member Author

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

@Shnatsel Shnatsel reopened this Jul 21, 2019
@64
Copy link

64 commented Jul 22, 2019

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

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

No branches or pull requests

3 participants