Undefined Behavior in Client and Properties

[jasongrlicky]

When auditing this crate to incorporate it into a project, I ran across several instances of definite and potential undefined behavior ("UB").

Most of the UB I found stemmed from use of the now-deprecated mem::uninitialized() function. This makes sense, since this crate was created before its replacement, MaybeUninit, was stabilized.

However, since that time been determined that using mem::uninitialized() will very frequently result in UB. When auditing this crate, I found several instances of UB from using this method, so it would likely be best to replace it throughout the crate.

The only other UB I found was that MIDIObjectGetStringProperty's out value could be NULL (note the _Nullable annotation in its signature), but the output parameter is immediately assigned to a mutable reference, which are required to never be NULL. The solution to this is to check if the output from MIDIObjectGetStringProperty is NULL before continuing.

Reasoning

I'll use this code as an example of what I'm referring to:

let mut client_ref: MIDIClientRef = unsafe { mem::uninitialized() };
let status = unsafe { MIDIClientCreate(
    client_name.as_concrete_TypeRef(),
    None, ptr::null_mut(),
    &mut client_ref)
};

Using mem::uninitialized() for out-parameters of C functions

MIDIClientRef is a typedef that ultimately resolves to a u32. Following the logic here, it is UB to initialize a u32 via mem::uninitialized(), because u32 can have any fixed bit pattern, but mem::uninitialized is specified as not having a fixed bit pattern. While it seems like the rules against uninitialized integers aren't fully settled yet, it is advised against until it is.

Passing references to out-parameters of C functions

Again following the logic on the MaybeUninit docs, taking a &mut reference to uninitialized memory is UB, since reference must be guaranteed to be aligned an non-NULL, which can't be said for uninitialized memory. It seems like the right thing to do would be to pass in * mut instead.

Next Steps

I am currently working on a pull request that addresses all these UB issues by replacing mem::uninitialized() with MaybeUninit and checking the out param of MIDIObjectGetStringProperty for NULL.

It's worth noting that my audit of the crate did not end up going very far into packet.rs, because there is a lot of unsafe code in there that made it hard to reason about what was happening. But I believe that at the very least, the call to Vec::set_len() in PacketBufferStorage is UB because of the requirement that the elements in the span of the new length have already been initialized. I will likely file a separate issue addressing the stability problems with this module and proposing a safer solution later.

Note: This is a breaking change because it will update the minimum rust version to the one that has MaybeUninit, which is 1.36