jonas-k / coremidi-sys

Low level Rust bindings for CoreMIDI
MIT License
8 stars 5 forks source link

MIDIPacketNext UB: Reference to Misaligned Pointer #10

Closed jasongrlicky closed 3 years ago

jasongrlicky commented 4 years ago

Hi there! So I've been auditing the dependencies for a project of mine to try and reduce the amount of Undefined Behavior (UB) that is relied on. I'm not an expert by any means, but my intention was to catch the obvious stuff.

Context

The MIDIPacket is #pragma pack 4 in the MIDIServices.h header, so my understanding is that it is correctly annotated with #[repr(C, packed(4))] by bindgen and thus 4-byte aligned.

In the MIDIPacketList comment from the CoreMIDI framework, it says that MIDIPackets may be unaligned:

The MIDIPacketNext macro is especially important when considering that the alignment requirements of MIDIPacket may differ between CPU architectures. On Intel and PowerPC, MIDIPacket is unaligned. On ARM, MIDIPacket must be 4-byte aligned.

My interpretation of this comment is that, though MIDIPacket is 4-byte-aligned, the structs may start at unaligned addresses in a MIDIPacketList that comes from CoreMIDI. This makes working with it safely a lot harder, so if anyone has any further clarification here, that would be super helpful to share.

The Problem

(Link to source)

On the first line of the function, we dereference and re-borrow the pointer to MIDIPacket that is passed in. If the struct that that pointer is pointing to is unaligned, as the CoreMIDI docs say that it is, then it is UB to dereference that pointer:

  • Dereferencing (using the * operator on) a dangling or unaligned raw pointer.

Here is a link to a playground where you can reproduce the UB. I couldn't figure out how to safely create a MIDIPacketList, so I just used a wrapper struct to manually misalign the MIDIPacket. You can run the code with Miri by selecting the Tools -> Miri menu option, and see that it notices this alignment issue.

Paths to Research

I believe this is what the raw reference operator RFC is designed to solve, but that work is not on stable Rust yet.

I was trying to figure out a way to address this on the current stable Rust (1.43). One potential avenue could be to cast the *const MIDIPacket to a pointer with a compatible layout that is #[repr(C, packed(1))]. I am not sure if the pointer cast that lowers the alignment itself would be UB or not, though, since I know that this type of type-punning in C is UB. Another option might be to use a Union to do the punning.

All of this said, I don't even know if lowering the alignment via type-punning is even effective for making the behavior defined.

Another option could be to cast the *const MIDIPointer to an integer and do arithmetic on it to calculate the addresses of the fields we need to use, since we know the layout of the struct.

jasongrlicky commented 3 years ago

With the release of Rust 1.51 today, the ability to create raw pointers to unaligned fields has landed in the form of the std::ptr::addr_of macro! I am not sure what this crate's minimum-supported-rust-version policy is, but at least there is theoretically a clear path forward for both this ticket and #11 now. 👍

jonas-k commented 3 years ago

Thank you, that sounds great. There is no minimum-supported-rust-version policy. Since this problem can only now be solved with version 1.51 and we needed to bump the major version anyway, we can also bump the minimum required rust version.

jonas-k commented 3 years ago

Thanks again, @jasongrlicky! Version 3.0.0 is published now landing the fix for this issue.