Propose changes to address undefined behavior

[spacecams]

Hello, thank you for making this crate. Here are some proposed changes to unsafe code that avoid undefined behavior. These changes either maintain the optimizations that the original versions contained or incur minimal overhead in the existing context.

For src/read.rs::read_bytes, creating a reference to uninitialized memory is considered undefined behavior (tracked issue). Since the function is called with n = 4 bytes, the initialization overhead should be minimal.

For calls to mem::transmute, we can add explicit return types to avoid possible unpredicted types from inference.

For the buffer fields in ChunksWriter and SampleWriter16, we can use MaybeUninit<u8> to be explicit about when the values are initialized to avoid undefined behavior of creating references to uninitialized memory.

In write_sample<S: Sample>the calls to write on MaybeUninit<u8>'s behave the same as writing the u8 values as before.

For write_u16_le_unchecked, we can use MaybeUninit::write() like we did for write_sample but instead of referencing a bounds checked element at an index, we can get an unchecked reference as before.

Assmebly code for write_u16_unchecked can be found here.

Thanks again for maintaining this awesome crate, I hope you find these proposed changes useful for future development and use.

[ruuda]

Thank you for taking the time to open a pull request. Nice that you also verified the assembly for write_u16_unchecked.

I have two small comments, but overall this looks like a nice improvement.

For MaybeUninit, in the past I refrained from using it because it was relatively new (Hound predates it by several years, you can probably also tell by the try! instead of ?) and at the time I did not think bumping the minimal supported Rust version was worth that. But by now, Rust 1.36 is pretty old, and making 1.36 the new MSRV should be fine. We can also go to 1.40 and get f32::from_le_bytes. Even Debian oldoldstable ships Rust 1.41, so bumping the MSRV to 1.40 should be fine.

If you address my comments, I’ll merge this, but also to set some expectations, master is not in a state right now where I’m comfortable releasing it. I want to review it front to back and fuzz it before making a new release, but in all honesty, I’ve been planning to do that for multiple years now and I never get to it, so it’s unlikely that I’ll release this soon.

[ruuda]

There is also from_le_bytes since Rust 1.40. If you are concerned about the unsafe, and we are bumping the minimum supported Rust version to 1.36 for the MaybeUninit anyway, then it’s a small step to go to 1.40, and then we can use from_le_bytes here, and get rid of the unsafe and transmute entirely.

[spacecams]

from_le_bytes looks good. I also called read_into directly instead of calling read_le_u32 to avoid going from [0u8; 4] to u32 to [0u8; 4] back to f32. If you'd rather keep the call to read_le_u32 I can change it back.

[ruuda]

Thanks!

[spacecams]

Absolutely! Regarding the current state of master, other than wanting to review and fuzz the code, do you have a punch list of items to fix? I may be able to work through that list to help get a release out the door.

Regarding fuzzing, do you just want to run the existing fuzz harness on master or are you also hoping to add new features? I may be able to help with this as well.

[maxwellmckinnon]

Interested in picking up these changes! @ruuda , would these changes be able to release out in a new crate version?

[ruuda]

So, @maxwellmckinnon backported these changes on top of v3.5.0, and I published those to crates.io as v3.5.1. As far as I am aware, the undefined behavior did not cause rustc to compile programs in a problematic way, but for good measure I submitted a RustSec advisory either way.