chris-zen / coremidi Goto Github PK

If MIDIGetDestination or MIDIGetSource fail (e.g. because an invalid port number was given), they return NULL, which is not handled in from_index. Instead, from_index should return a Result.

I'm going to prepare a PR if you don't beat me to it.

Hi,

In pod-ui I'm using coremidi through midir. I have a badly-behaving M-Vave MS-1 bluetooth MIDI device that keeps crashing the app. I know, midir 0.9.x uses an old version of coremidi, I've tried Boddlnagg/midir#139 and Boddlnagg/midir#142, but the error still prevails.

I understand that MS-1 is the culprit here, but is there some sanity-checking in PacketListIterator that can be done to prevent crashing?

Here are the backtraces:
coremidi-panic-midir0.9.1.txt
coremidi-panic-midir0.9.2+coremidi0.7.0.txt
coremidi-panic-midir0.9.2+coremidi0.8.0.txt

Hey there! I'm using your library in my iOS app and came across one of those pesky CoreMIDI bugs. I traced it down to the MIDIClientDispose call in the Drop implementation for Client.

What is happening

I have a document-based app and every document has a MIDI client attached, i.e. when opening a doc, a MIDI client gets created (MIDIClientCreate) and when closing the doc, it gets destroyed (MIDIClientDispose). This works fine as long as the app is used in the foreground. However, when I put the app to background after closing a doc - then bringing it to the front and opening a doc, all subsequent calls of MIDIClientCreate fail with OSStatus -50.

Digging a little into it, -50 will be returned either for invalid arguments (checked them, all good) or issues with the MIDI server (src). Finally, I came across this discussion note for the dispose call in the Apple docs:

Don’t explicitly dispose of your client; the system automatically disposes all clients when an app terminates. However, if you call this method to dispose the last or only client owned by an app, the MIDI server may exit if there are no other clients remaining in the system. If this occurs, all subsequent calls by your app to MIDIClientCreate and MIDIClientCreateWithBlock fail.

My hypothesis is, as all clients are disposed when backgrounding the app, iOS decides to kill the MIDI server and doesn't restart it when bringing the app back to the foreground. This might be categorized as an iOS bug, but on the other hand, Apple explicitly discourages the use of MIDIClientDispose. Once the MIDI server is killed by the OS, there is no chance to recover unless doing a full app restart.

Suggestion

Remove the Drop implementation from Client as cleanup is handled by the system.

Workaround

As long as one MIDI client is around, the MIDI server stays alive. Therefore calling MIDIClientCreate somewhere during app init and omitting the dispose call does the trick.

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

Is there a reason why you did not use MIDIPacketListAdd and instead built your own PacketBuffer with quite a bit of unsafe code, which might not really be necessary?

Maybe I'm missing something, but right now I don't understand it. If there is a good reason, it should at least be documented in the source code.

I am creating a Tauri app that can get a list of midi devices and receive midi packets. I am using your library CoreMIDI. The issue that I am having is that when using the code block to receive midi packets is that it run through the code block line by line and does not give opportunity for me to input midi from my midi device. It's like the code runs and complete without me pressing enter. It works in just using a plain Rust project just does not working when using it with Tauri

If you can help me with this I will be grateful

I have attached example of the code I created
https://github.com/elmcapp/elmc-midi

Tauri setup:
https://tauri.studio/docs/getting-started/setting-up-macos
and you can run the project using yarn tauri dev

Also here is a video to show what's going wrong

Is it intentional that coremidi::Client is not Send?

    = help: within `common::MidiInput`, the trait `std::marker::Send` is not implemented for `*mut std::boxed::Box<(dyn for<'r> std::ops::FnMut(&'r backend::coremidi::coremidi::Notification) + 'static)>`
    = note: required because it appears within the type `backend::coremidi::coremidi::BoxedCallback<backend::coremidi::coremidi::Notification>`
    = note: required because it appears within the type `backend::coremidi::coremidi::Client`
    = note: required because it appears within the type `backend::coremidi::MidiInput`

I was not able to find any information regarding the thread safety of MIDIClientRef, but I also don't know about thread-safety in the macOS APIs in general ...

I had users of midir complain that some structs are not Send, so I'm trying to implement Send for everything, but of course I want to know whether it's actually safe ...

Hi there! I am working on integrating this library for a project, and am having trouble getting notified when MIDI devices are added or removed (or any notifications at all for that matter). Even debugging with a symbolic breakpoint on notify_proc, it seems like the notification function is never called when adding or removing MIDI devices from the system.

To reproduce it, just create a Client with notifications and see that none come in when adding or removing MIDI devices. You can do this with this minimal example, which prints any notifications received. If you do not have any MIDI devices to add or remove, you can enable and disable the IAC Bus, as shown in this article.

I am planning on looking into this further, but figured I'd start the conversation to see if anyone has any ideas about what could be happening? Thank you!

When auditing this crate to incorporate it into a project, I found issues around CFStringRefs being over-released in certain circumstances when creating *-Property structs using ::new().

The problem would arise when using an incorrect user-supplied string when initializing a property, making the following line of code result in a panic with a SIGILL:

let name = destination.get_property_string("NotARealPropertyName").unwrap();

So when boiled down, the problem is basically the same one as in #17 - a CFString was created and then immediately dropped in *-Property::new(), meaning any CFStringRefs created from it are now invalid. The solution was to abstract over *-Propertys now owning a CFString or a CFStringRef, depending of if the Property struct was initialized with a constant from CoreMidi or a user-supplied string. This is what the PropertyKeyStorage struct does.

My application uses the midir crate that uses coremidi as a dependency. The original issue is tracked here Boddlnagg/midir#86

The reason for continously creating a client for scanning the ports because the cross-platform midir crate does not support change notifications yet.

While there is a workaround for the problem, it still would be nice to address the issue if possible. I would expect that the acquired client resource is closed at the end of each loop iteration as the variable goes out of scope.

Demo code:

use coremidi::Client;

fn main() {
    loop {
        let client = Client::new("test midi client");

        match client {
            Ok(client) => {
                println!("{:?}", &client);
            }
            Err(error) => {
                println!("{:?}", error);
            }
        }

        std::thread::sleep(std::time::Duration::from_millis(1000));
    }
}

Hey Christian,
you would be interested in a PR that adds the block crate and replaces calls to MIDIInputPortCreate with a call to MIDIInputPortCreateWithBlock and call to MIDIClientCreate with call to MIDIClientCreateWithBlock . It would make the codebase easier to understand. I just tried a basic proof of concept and it seems to work.

Hello, we have found a vulnerability in this repository, which may present serious security risks. However, we have been unable to locate a contact email to report the details of this vulnerability. We would like to know if you can provide an email address to confidentially discuss this matter and avoid public disclosure.

Client::input_port takes a callback by move, then passes the pointer to the C ffi. I believe that's going to work ok if the callback happens to be a plain function, but fail pretty badly if it contains any context. The right thing to do is put it in a Box. See https://github.com/RustAudio/coreaudio-rs/blob/master/src/audio_unit/render_callback.rs#L383 for how coreaudio deals with a similar issue.

The box needs to be dropped with the port. I believe that's a slightly more far-reaching change. Probably Object needs to store a second box for type erasure of the polymorphic drop.

The callback also needs Send and 'static bounds, to indicate that it might be called from a different thread, and after the function returns. Also, Fn should probably be weakened to FnMut (assuming Core MIDI guarantees that only one callback at a time will be outstanding, which I think is a reasonable assumption but I'm not 100% sure - https://lists.apple.com/archives/coreaudio-api/2002/Apr/msg00002.html seems to indicate it's ok).

I can send a PR if you'd like - I have this working as a prototype except it just leaks the box with the function.

Hi there! I was just investigating the issue with the notifications callback not being called, and figured getting the tests for this crate running would be a good first step. However, I ran into an error when running one of the tests:

> cargo test -- notifications::tests::notification_from_property_changed --exact

... returns the following error message when running on my machine:

    Finished test [unoptimized + debuginfo] target(s) in 1.65s
     Running target/debug/deps/coremidi-8245418d84ec7987

running 1 test
error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `/Users/jason/projects/coremidi/target/debug/deps/coremidi-8245418d84ec7987 'notifications::tests::notification_from_property_changed' --exact` (signal: 4, SIGILL: illegal instruction)

I'm using Rust 1.43 stable on macOS Mojave 10.14.6.

From debugging the test in VSCode, it seems like a CFString might be getting over-released upon being drop()ed in Notification::from_property_changed(), which is called by Notification::from(&MidiNotification). I'm not sure, as I haven't programmed directly against the core-foundation crate yet, so I'm familiar with how things are supposed to work there.

I'll be looking into this further, but figured I'd get the conversation started to see if anyone has any ideas about what could be causing this particular test to break. Thank you!

Currently it is possible (and may happen by accident) that a Client is dropped, while a port or virtual endpoint is still open.

If the port is an output, you can still send to the output without an error, but no message is actually sent. Similarly, if the port is an input, you just won't receive any messages.

I don't think that this is a memory safety issue, but it might be a source of surprising behavior. The fix would be to couple the lifetimes of ports to the Client, though this makes the API slightly more complicated.

Sorry for a random thought, I started using the lib today and have puzzled through a few things. One thing that would be nicer is if PacketBuffer consumed itself and returned Self like EventBuffer currently does. It makes things easier with bindings, and also avoids the need for a &* deref when sending, etc.

EventBuffer is a little nicer to work with in that way, but it did take me a while to work out that EventBuffer only supports the new Unified Midi Packet format - I believe? It's not really mentioned anywhere specifically! If you want to send old-style MIDI I think PacketBuffer is still the way?

Looks like this is the raw midi data. Is there any way to convert it to friendly humans readable midi data

I was finally able to spend some time porting my latest midir changes to macOS, but ran into a problem. The goal is to be able to have a input/output port identifier that is not just an index, in the hope that they would be more stable than a plain index (imagine you have a list of devices, select index 2, then device 1 is plugged out before you establish the connection; so you end up with what was originally index 3 or an out-of-range index).
I tried to solve this by storing a Destination or Source object directly, instead of an index. However, my connection API takes Destination by reference, not by value, but I need to store it then to use it for sending.
Long story short: it would be handy if I could just clone() the Destination, but that's currently not possible. Since technically a Destination seems to just wrap an integer, I wondered if it would be possible to change this. But I'm not very familiar with the ObjC object model ...