Soundness Bug - StreamContextInfo is not safe to send across threads

[erickt]

We found another soundness issue. In src/fsevent.rs we have this definition:

struct StreamContextInfo {
    event_fn: Arc<dyn EventFn>,
    recursive_info: HashMap<PathBuf, bool>,
}

This type is passed across a thread that actually listens to file changes. Unfortunately this type isn't actually send because while pub trait EventFn: 'static + Fn(Result<Event>) + Send {} implements Send, Arc<dyn EventFn> is not Send. That also requires EventFn to also implement Sync. Unfortunately just adding Sync doesn't compile because of some other conflicts.

[0xpr03]

To clarify, from my point of understanding Arc is only Send when T is Send. The trait itself is send, so Arc should be Send which is fine. But that isn't enough because of what ?

We could just wrap it inside a Mutex, but AFAIK whether we use a Mutex or a Send Trait doesn't make a difference (see above). Making it Sync is somewhat required as we otherwise have to guarantee that nobody else is calling the Trait (datarace)? At which point it'd be better to just move the whole dyn Trait to the thread instead of sharing a reference and hoping for the best?

[0xpr03]

Never mind, I just realized the core issue, guess it's a little bit late for me.

Edit: See also this answer on SO about Drop requirements for why it's always both..

[0xpr03]

Reading over the code: Is there any reason to have this inside an Arc instead of a raw Box that is kept by StreamContextInfo for the callback ? We should have some guarantee about whether it's ok for the callback to a non-sync call or not.

[0xpr03]

If we would simply expose a channel instead of a callback this could be completely eliminated. It would also allow #189. Though it's coming with its own limitations (and will introduce locking inside the callback).

[erickt]

StreamContextInfo has event_fn in an Arc because it needs to clone it and pass it into the event thread. We can't change it into a Box without either making EventFn cloneable, or restructuring the types to make Watcher take ownership over self, which might be pretty heavy.

I'm not familiar with #189, but the simplest solution for this would be to change StreamContextInfo's event_fn in Arc<RwLock<dyn EventFn>>, since EventFn doesn't allow mutability, so we'd always be grabbing a read lock, which should be quite cheap.

[erickt]

Actually I'm wrong, RwLock is only Sync if T is Sync. So the smallest fix for this would be to use Arc<Mutex<T>>. I suggest we do that to address the immediate soundness issues, then explore alternatives to get rid of the mutex.