search

helgoboss / reaper-rs

Rust bindings for the REAPER C++ API
MIT License
82 stars 8 forks source link

Reentrancy-related soundness issue in ControlSurface logic #54

Open helgoboss opened 2 years ago

helgoboss commented 2 years ago

Btw, would it be possible to make methods like set_track_list_change and ext_set_fx_change take &mut self or would that be unsafe? (I'm currently working around this by sending msgs to self that are then handled in run.)

@Boscop FYI, I realized there's a soundness issue even with the current restriction of allowing read access only in set_track_list_change and similar callbacks. If we are currently in the call stack of ControlSurface::run() and REAPER invokes one of the callback methods, then even giving read-only access to the control surface is wrong. We shouldn't grant any access at all in this case. I think the only sound way (from a Rust perspective) would be to defer the callback method invocation to a point after the run() method has completed. So either to execute right after run() has ended or wait until the next run() cycle ... when doing this consequently, one could even grant write access.

Originally posted by @helgoboss in https://github.com/helgoboss/reaper-rs/issues/39#issuecomment-1077681129

helgoboss commented 2 years ago

When we move that logic to reaper-rs, we should not forget to use ChangeEvent::is_still_valid() to only fire it when it's still valid. See method doc for details.