search

ruuda / hound

A wav encoding and decoding library in Rust
https://codeberg.org/ruuda/hound
Apache License 2.0
491 stars 65 forks source link

Propose changes to address undefined behavior #58

Closed spacecams closed 2 years ago

spacecams commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Thanks!

spacecams commented 2 years ago

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 commented 1 year ago

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

ruuda commented 1 year ago

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.