portmidi-rs
portmidi-rs copied to clipboard
Some code is unsafe but not marked as unsafe
This example segfaults:
let output_port = {
let context = PortMidi::new().unwrap();
let device = context.device(device_number).unwrap();
context.output_port(device, 1024).unwrap()
};
output_port.write_message(message); // SEGFAULT
The problem is that output_port
outlives context
. To be more precise, context
calls Pm_Terminate
in its destructor rendering output_port
invalid. But, unfortunately, outport_port
is still accessible after the scope braces. The problem can be solved by adding an artificial lifetime to the DeviceInfo
struct, s.t. Rust knows that output_port
depends on a DeviceInfo
instance which depends on context
. Also, DeviceInfo::new
should be unsafe and PortMidi::device
should validate the device number.
Your remarks are relevant. Have you the time to implement them?
Yes, I can do it. In my opinion, there is not much that needs to be done. However, there will be some impact on the existing examples and tests. I think, some Arc
and Mutex
wrappers (Don't know the mutability by heart) will be necessary.
Perhaps it can be interesting to set Context and DeviceInfo send and sync to avoid the use of Arc and Mutex if the Context or DeviceInfo should be kept everywhere in the code. It's easier to use clone than Arc / Mutex. In Lapin RabbitMQ lib they have done like that
Hmm... Sync
and Send
usually are auto-derived. They should only be implemented manually if they are safe to be used in different threads.
An example: In the current setup, OutputPort
is Send
. You can create as many instances as you want and send them to different threads. This means all functions like write_message
, etc. can be executed concurrently. I am not sure if the underlying ffi::Pm_WriteShort
is actually thread-safe.
If the merged pull request resolve the issue, please close it and create a new release.
It doesn't solve all the pb but I'll do a release to have the lifetime update.
I do the cargo publish of the version 0.2.5