musitdev / portmidi-rs

High-level PortMidi bindings and wrappers for Rust
Apache License 2.0
77 stars 22 forks source link

Some code is unsafe but not marked as unsafe #33

Open Woyten opened 6 years ago

Woyten commented 6 years ago

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.

musitdev commented 6 years ago

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

Woyten commented 6 years ago

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.

musitdev commented 6 years ago

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

Woyten commented 6 years ago

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.

piegamesde commented 4 years ago

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

musitdev commented 4 years ago

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

musitdev commented 4 years ago

I do the cargo publish of the version 0.2.5