portmidi-rs icon indicating copy to clipboard operation
portmidi-rs copied to clipboard

Some code is unsafe but not marked as unsafe

Open Woyten opened this issue 6 years ago • 7 comments

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.

Woyten avatar Aug 05 '18 17:08 Woyten

Your remarks are relevant. Have you the time to implement them?

musitdev avatar Aug 05 '18 18:08 musitdev

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.

Woyten avatar Aug 05 '18 18:08 Woyten

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

musitdev avatar Aug 06 '18 06:08 musitdev

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.

Woyten avatar Aug 13 '18 21:08 Woyten

If the merged pull request resolve the issue, please close it and create a new release.

piegamesde avatar Aug 21 '20 18:08 piegamesde

It doesn't solve all the pb but I'll do a release to have the lifetime update.

musitdev avatar Aug 21 '20 19:08 musitdev

I do the cargo publish of the version 0.2.5

musitdev avatar Aug 21 '20 20:08 musitdev