From 000246678622bfd3b3bc7895ac603e0d83aa9d79 Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Wed, 2 Jun 2021 10:11:00 -0700 Subject: [PATCH 1/3] Fix StreamContextInfo to be Send In #324, we found that we were passing StreamContextInfo to another thread, even though the type is not actually Send. Fixes #324 --- src/fsevent.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/fsevent.rs b/src/fsevent.rs index 69967839..7bf4725a 100644 --- a/src/fsevent.rs +++ b/src/fsevent.rs @@ -25,7 +25,7 @@ use std::ffi::CStr; use std::os::raw; use std::path::{Path, PathBuf}; use std::ptr; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use std::thread; bitflags::bitflags! { @@ -64,7 +64,7 @@ pub struct FsEventWatcher { since_when: fs::FSEventStreamEventId, latency: cf::CFTimeInterval, flags: fs::FSEventStreamCreateFlags, - event_fn: Arc, + event_fn: Arc>, runloop: Option<(cf::CFRunLoopRef, Receiver<()>)>, recursive_info: HashMap, } @@ -225,7 +225,7 @@ fn translate_flags(flags: StreamFlags, precise: bool) -> Vec { } struct StreamContextInfo { - event_fn: Arc, + event_fn: Arc>, recursive_info: HashMap, } @@ -235,7 +235,7 @@ extern "C" { } impl FsEventWatcher { - fn from_event_fn(event_fn: Arc) -> Result { + fn from_event_fn(event_fn: Arc>) -> Result { Ok(FsEventWatcher { paths: unsafe { cf::CFArrayCreateMutable(cf::kCFAllocatorDefault, 0, &cf::kCFTypeArrayCallBacks) @@ -375,11 +375,14 @@ impl FsEventWatcher { }; // Unfortunately fsevents doesn't provide a mechanism for getting the context back from a - // stream after it's been created. So in order to avoid a memory leak, we need to box the - // pointer up, alias the pointer, then drop it when the stream has completed. + // stream after it's been created. So in order to avoid a memory leak in the normal case, + // we need to box the pointer up, alias the pointer, then drop it when the stream has + // completed. This box will be leaked if a panic is triggered before the inner thread has a + // chance to drop the box. let context = Box::into_raw(Box::new(info)); // Safety + // - StreamContextInfo is Send+Sync. // - This is safe to move into a thread because it will only be accessed when the stream is // completed and no longer accessing the context. struct ContextPtr(*mut StreamContextInfo); @@ -531,14 +534,14 @@ unsafe fn callback_impl( for ev in translate_flags(flag, true).into_iter() { // TODO: precise let ev = ev.add_path(path.clone()); - (*event_fn)(Ok(ev)); + (event_fn.lock().unwrap())(Ok(ev)); } } } impl Watcher for FsEventWatcher { fn new_immediate(event_fn: F) -> Result { - FsEventWatcher::from_event_fn(Arc::new(event_fn)) + FsEventWatcher::from_event_fn(Arc::new(Mutex::new(event_fn))) } fn watch>(&mut self, path: P, recursive_mode: RecursiveMode) -> Result<()> { @@ -595,3 +598,9 @@ fn test_fsevent_watcher_drop() { println!("in test: {} works", file!()); } + +#[test] +fn test_steam_context_info_send() { + fn check_send() {} + check_send::(); +} From 3bf6d4a6ff4a10a80a1dd71c5249b433e44ebd19 Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Wed, 2 Jun 2021 10:43:27 -0700 Subject: [PATCH 2/3] Fix a typo --- src/fsevent.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fsevent.rs b/src/fsevent.rs index 7bf4725a..b48860fa 100644 --- a/src/fsevent.rs +++ b/src/fsevent.rs @@ -457,7 +457,7 @@ impl FsEventWatcher { .send(()) .expect("error while signal run loop is done"); }); - // block until runloop has been set + // block until runloop has been sent self.runloop = Some((rl_rx.recv().unwrap().0, done_rx)); Ok(()) From 0c2b6415975e30bf82e529c4669296c5374403ac Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Wed, 2 Jun 2021 11:29:07 -0700 Subject: [PATCH 3/3] Address comments and add comment to changelog --- CHANGELOG.md | 4 ++++ src/fsevent.rs | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24984106..b1606aa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,10 @@ ## unreleased +- FIX: Make StreamContextInfo Send to fix soundness issue [#325] + +[#325]: https://github.com/notify-rs/notify/pull/325 + ## 5.0.0-pre.9 (2021-05-21) - DEPS: Upgrade fsevent-sys dependency to 4.0 [#322] diff --git a/src/fsevent.rs b/src/fsevent.rs index b48860fa..9018c8c1 100644 --- a/src/fsevent.rs +++ b/src/fsevent.rs @@ -534,7 +534,8 @@ unsafe fn callback_impl( for ev in translate_flags(flag, true).into_iter() { // TODO: precise let ev = ev.add_path(path.clone()); - (event_fn.lock().unwrap())(Ok(ev)); + let event_fn = event_fn.lock().expect("lock not to be poisoned"); + (event_fn)(Ok(ev)); } } }