search

chris-zen / coremidi

CoreMIDI library for Rust
https://chris-zen.github.io/coremidi/coremidi/
MIT License
75 stars 20 forks source link

Serious safety problems with callbacks #2

Closed raphlinus closed 7 years ago

raphlinus commented 7 years ago

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.

chris-zen commented 7 years ago

Hi @raphlinus, thank you very much for the feedback. I am not very much involved with the project right now, so if you can PR the proposed changes I will be delighted to review ;-)