Code review request for hexchat-unsafe-plugin

We already messed this up once by pushing 1.0.0 with uh Bad Code in it so we'd like to play it safe and ask around if this looks okay: commit

kinda annoyed at having to bump to 2.0.0 this soon but ah well.

It looks like you have the variance on some of the types mixed up. For instance, PluginHandle<'ph> is covariant over 'ph, but it allows mutation through a shared reference. As a minimal example, this plugin causes a segmentation fault once it is loaded by reading s after it is dropped:

use hexchat_unsafe_plugin::{hexchat_plugin, Plugin, PluginHandle};
use std::{mem, pin::Pin};

#[derive(Default)]
struct MyPlugin;

// SAFETY: No additional threads are spawned by this plugin.
unsafe impl<'ph> Plugin<'ph> for MyPlugin {
    fn init(
        self: Pin<&mut Self>,
        ph: &mut PluginHandle<'ph>,
        _filename: &str,
        _arg: Option<&str>,
    ) -> bool {
        let s = "Hello, world!".to_string();
        let timer = ph.hook_timer(0, |_| {
            println!("{s}");
            false
        });
        mem::forget(timer);
        true
    }
}

hexchat_plugin!('ph, MyPlugin);
1 Like

Ouch, we forgot you could leak HookHandles! :see_no_evil:

(we knew something looked funny about trying to let the hooks access local context but somehow we didn't connect that to pre-pooping your pants.)

Edit: Oh wait, huh, what's going on here...

Ohh. We think we overlooked that RawPh (and thus LtPhPtr) is effectively self-referential. A *mut RawPh<'ph> would've immediately shown the issue...

Alright, so that's fixed. What's next?

call_hook_protected() and the other two catch_unwind() callers aren't sufficiently careful with the panic payload. If the payload panics on drop, then that panic will propagate into C code. As a minimal example:

use hexchat_unsafe_plugin::{hexchat_plugin, HookHandle, Plugin, PluginHandle};
use std::{panic, pin::Pin};

struct PanicOnDrop;

impl Drop for PanicOnDrop {
    fn drop(&mut self) {
        panic!()
    }
}

#[derive(Default)]
struct MyPlugin<'ph> {
    timer: Option<HookHandle<'ph, 'ph>>,
}

unsafe impl<'ph> Plugin<'ph> for MyPlugin<'ph> {
    fn init(
        mut self: Pin<&mut Self>,
        ph: &mut PluginHandle<'ph>,
        _filename: &str,
        _arg: Option<&str>,
    ) -> bool {
        self.timer = Some(ph.hook_timer(0, |_| panic::panic_any(PanicOnDrop)));
        true
    }
}

hexchat_plugin!('ph, MyPlugin<'ph>);

As an aside, the logic of rc_clone_from_raw() is a bit weird. If I'd have written it, I'd have used Rc::increment_strong_count():

unsafe fn rc_clone_from_raw<T>(ptr: *const T) -> Rc<T> {
    Rc::increment_strong_count(ptr);
    Rc::from_raw(ptr)
}

Hmm...

What's the "standard"/most popular way of handling those? Leak unknown payloads? struct AbortGuard?

Also yeah increment_strong_count didn't exist back in the day. See also Why clonefromrawing and intorawing is better than refcount manipulation - libs - Rust Internals

Personally, I'd just drop the payload inside of another catch_unwind(), then abort() if that panics. There's not much one can do at that point. Alternatively, unconditionally abort() after handling the payload, since a plugin writer would likely expect a panic to end the program.

For reference, Rc::increment_strong_count() has been stable since Rust 1.53.0. Meanwhile, hexchat-unsafe-plugin doesn't compile on versions older than 1.58.0, since it depends on impl<T> RefUnwindSafe for Rc<T> where T: RefUnwindSafe.

no they would not(!), at least not hexchat plugin authors. (well, depends on if they're the kind of dev who tests on their production hexchat instance (hi) but yeah.) but yeah we guess we can use an abort guard of some sort...

we mean, yeah, tho we forked from hexchat-plugin which did a lot of unsound things but this wasn't one of them :sweat_smile:

we don't particularly care about it either way, but this feels like one of those "don't fix what isn't broken" kinda situations. (tho we do still wish we didn't have to roll our own rc_clone_from_raw...)

Alright, we think we have that one fixed unless we did something really wrong. What's next?

(Also, how would you like to be credited in our crate?)

I haven't found anything soundness-related yet (I've been somewhat busy), but looking at HookHandle, I don't see why it needs an 'f lifetime at all. A HookHandle<'ph, 'f> can never be accessed outside of 'ph, and the type is covariant over 'f; therefore, it is always be replaced with a HookHandle<'ph, 'ph> in all circumstances. As an experiment, I replaced all instances of 'f with 'ph, and all tests pass with no issues. Here's the relevant diff:

diff --git a/src/lib.rs b/src/lib.rs
index 3585031..9a2ffc3 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -40,7 +40,7 @@
 //!
 //! #[derive(Default)]
 //! struct MyPlugin<'ph> {
-//!     cmd: Mutex<Option<HookHandle<'ph, 'ph>>>
+//!     cmd: Mutex<Option<HookHandle<'ph>>>
 //! }
 //!
 //! unsafe impl<'ph> Plugin<'ph> for MyPlugin<'ph> {
@@ -359,12 +359,12 @@ pub struct EventAttrs<'a> {
 
 /// A hook handle, used to enable unhooking.
 #[must_use = "Hooks must be stored somewhere and are automatically unhooked on Drop"]
-pub struct HookHandle<'ph, 'f> where 'f: 'ph {
+pub struct HookHandle<'ph> {
     ph: LtPhPtr<'ph>,
     hh: *const internals::HexchatHook,
     freed: Rc<Cell<bool>>,
     // this does actually store an Rc<...>, but on the other side of the FFI.
-    _f: PhantomData<Rc<HookUd<'f>>>,
+    _f: PhantomData<Rc<HookUd<'ph>>>,
 }
 
 /// A virtual plugin.
@@ -399,7 +399,7 @@ impl<F> InvalidContextError<F> {
     }
 }
 
-impl<'ph, 'f> HookHandle<'ph, 'f> where 'f: 'ph {
+impl<'ph> HookHandle<'ph> {
     /// If this is a timer hook, returns whether the hook has expired.
     ///
     /// Otherwise, returns false.
@@ -410,7 +410,7 @@ impl<'ph, 'f> HookHandle<'ph, 'f> where 'f: 'ph {
     /// use hexchat_unsafe_plugin::{HookHandle};
     ///
     /// /// Remove timers that have expired.
-    /// fn clean_up_timers(timers: &mut Vec<HookHandle<'_, '_>>) {
+    /// fn clean_up_timers(timers: &mut Vec<HookHandle<'_>>) {
     ///     timers.retain(|timer| {
     ///         !timer.expired()
     ///     });
@@ -421,7 +421,7 @@ impl<'ph, 'f> HookHandle<'ph, 'f> where 'f: 'ph {
     }
 }
 
-impl<'ph, 'f> Drop for HookHandle<'ph, 'f> where 'f: 'ph {
+impl<'ph> Drop for HookHandle<'ph> {
     fn drop(&mut self) {
         if self.freed.get() {
             // already free'd.
@@ -429,7 +429,7 @@ impl<'ph, 'f> Drop for HookHandle<'ph, 'f> where 'f: 'ph {
         }
         self.freed.set(true);
         unsafe {
-            let b = ((*self.ph.ph).hexchat_unhook)(self.ph.ph, self.hh) as *mut HookUd<'f>;
+            let b = ((*self.ph.ph).hexchat_unhook)(self.ph.ph, self.hh) as *mut HookUd<'ph>;
             // we assume b is not null. this should be safe.
             // just drop it
             drop(Rc::from_raw(b));
@@ -711,7 +711,7 @@ impl<'ph> PluginHandle<'ph> {
     /// ```no_run
     /// use hexchat_unsafe_plugin::{PluginHandle, HookHandle};
     ///
-    /// fn register_commands<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph, 'ph>> {
+    /// fn register_commands<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph>> {
     ///     vec![
     ///     ph.hook_command("hello-world", hexchat_unsafe_plugin::PRI_NORM, Some("prints 'Hello, World!'"), |ph, arg, arg_eol| {
     ///         ph.print("Hello, World!");
@@ -720,7 +720,7 @@ impl<'ph> PluginHandle<'ph> {
     ///     ]
     /// }
     /// ```
-    pub fn hook_command<'f, F>(&self, cmd: &str, pri: i32, help: Option<&str>, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_command<F>(&self, cmd: &str, pri: i32, help: Option<&str>, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol) -> Eat + 'ph + RefUnwindSafe {
         assert_eq!(self.data.ph, self.plugin, "PluginEntryHandle can't have hooks");
         unsafe extern "C" fn callback(word: *const *const c_char, word_eol: *const *const c_char, ud: *mut c_void) -> c_int {
             let f: Rc<HookUd> = rc_clone_from_raw(ud as *const HookUd);
@@ -759,7 +759,7 @@ impl<'ph> PluginHandle<'ph> {
     /// ```no_run
     /// use hexchat_unsafe_plugin::{PluginHandle, HookHandle};
     ///
-    /// fn register_server_hooks<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph, 'ph>> {
+    /// fn register_server_hooks<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph>> {
     ///     vec![
     ///     ph.hook_server("PRIVMSG", hexchat_unsafe_plugin::PRI_NORM, |ph, word, word_eol| {
     ///         if word.len() > 0 && word[0].starts_with('@') {
@@ -770,12 +770,12 @@ impl<'ph> PluginHandle<'ph> {
     ///     ]
     /// }
     /// ```
-    pub fn hook_server<'f, F>(&self, cmd: &str, pri: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_server<F>(&self, cmd: &str, pri: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol) -> Eat + 'ph + RefUnwindSafe {
         assert_eq!(self.data.ph, self.plugin, "PluginEntryHandle can't have hooks");
         self.hook_server_attrs(cmd, pri, move |ph, w, we, _| cb(ph, w, we))
     }
     /// Sets a server hook, with attributes.
-    pub fn hook_server_attrs<'f, F>(&self, cmd: &str, pri: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol, EventAttrs) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_server_attrs<F>(&self, cmd: &str, pri: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol, EventAttrs) -> Eat + 'ph + RefUnwindSafe {
         assert_eq!(self.data.ph, self.plugin, "PluginEntryHandle can't have hooks");
         unsafe extern "C" fn callback(word: *const *const c_char, word_eol: *const *const c_char, attrs: *const RawAttrs, ud: *mut c_void) -> c_int {
             let f: Rc<HookUd> = rc_clone_from_raw(ud as *const HookUd);
@@ -814,7 +814,7 @@ impl<'ph> PluginHandle<'ph> {
     /// ```no_run
     /// use hexchat_unsafe_plugin::{PluginHandle, HookHandle};
     ///
-    /// fn register_print_hooks<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph, 'ph>> {
+    /// fn register_print_hooks<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph>> {
     ///     vec![
     ///     ph.hook_print("Channel Message", hexchat_unsafe_plugin::PRI_NORM, |ph, arg| {
     ///         if let Some(nick) = arg.get(0) {
@@ -827,7 +827,7 @@ impl<'ph> PluginHandle<'ph> {
     ///     ]
     /// }
     /// ```
-    pub fn hook_print<'f, F>(&self, name: &str, pri: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_print<F>(&self, name: &str, pri: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word) -> Eat + 'ph + RefUnwindSafe {
         assert_eq!(self.data.ph, self.plugin, "PluginEntryHandle can't have hooks");
         // hmm, is there any way to avoid this code duplication?
         // hook_print is special because dummy prints (keypresses, Close Context) are handled
@@ -867,7 +867,7 @@ impl<'ph> PluginHandle<'ph> {
     /// ```no_run
     /// use hexchat_unsafe_plugin::{PluginHandle, HookHandle};
     ///
-    /// fn register_print_hooks<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph, 'ph>> {
+    /// fn register_print_hooks<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph>> {
     ///     vec![
     ///     ph.hook_print_attrs("Channel Message", hexchat_unsafe_plugin::PRI_NORM, |ph, arg, attrs| {
     ///         if let Some(nick) = arg.get(0) {
@@ -880,7 +880,7 @@ impl<'ph> PluginHandle<'ph> {
     ///     ]
     /// }
     /// ```
-    pub fn hook_print_attrs<'f, F>(&self, name: &str, pri: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word, EventAttrs) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_print_attrs<F>(&self, name: &str, pri: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word, EventAttrs) -> Eat + 'ph + RefUnwindSafe {
         assert_eq!(self.data.ph, self.plugin, "PluginEntryHandle can't have hooks");
         unsafe extern "C" fn callback(word: *const *const c_char, attrs: *const RawAttrs, ud: *mut c_void) -> c_int {
             let f: Rc<HookUd> = rc_clone_from_raw(ud as *const HookUd);
@@ -918,7 +918,7 @@ impl<'ph> PluginHandle<'ph> {
     /// ```no_run
     /// use hexchat_unsafe_plugin::{PluginHandle, HookHandle};
     ///
-    /// fn register_timers<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph, 'ph>> {
+    /// fn register_timers<'ph>(ph: &mut PluginHandle<'ph>) -> Vec<HookHandle<'ph>> {
     ///     vec![
     ///     ph.hook_timer(2000, |ph| {
     ///         ph.print("timer up!");
@@ -927,7 +927,7 @@ impl<'ph> PluginHandle<'ph> {
     ///     ]
     /// }
     /// ```
-    pub fn hook_timer<'f, F>(&self, timeout: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>) -> bool + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_timer<F>(&self, timeout: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>) -> bool + 'ph + RefUnwindSafe {
         assert_eq!(self.data.ph, self.plugin, "PluginEntryHandle can't have hooks");
         unsafe extern "C" fn callback(ud: *mut c_void) -> c_int {
             let f: Rc<HookUd> = rc_clone_from_raw(ud as *const HookUd);
@@ -1431,37 +1431,37 @@ impl<'a, 'ph: 'a> ValidContext<'a, 'ph> {
     /// Sets a command hook.
     ///
     /// See [`PluginHandle::hook_command`].
-    pub fn hook_command<'f, F>(&self, cmd: &str, pri: i32, help: Option<&str>, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_command<F>(&self, cmd: &str, pri: i32, help: Option<&str>, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol) -> Eat + 'ph + RefUnwindSafe {
         self.ph.hook_command(cmd, pri, help, cb)
     }
     /// Sets a server hook.
     ///
     /// See [`PluginHandle::hook_server`].
-    pub fn hook_server<'f, F>(&self, cmd: &str, pri: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_server<F>(&self, cmd: &str, pri: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol) -> Eat + 'ph + RefUnwindSafe {
         self.ph.hook_server(cmd, pri, cb)
     }
     /// Sets a server hook with attributes.
     ///
     /// See [`PluginHandle::hook_server_attrs`].
-    pub fn hook_server_attrs<'f, F>(&self, cmd: &str, pri: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol, EventAttrs) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_server_attrs<F>(&self, cmd: &str, pri: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word, WordEol, EventAttrs) -> Eat + 'ph + RefUnwindSafe {
         self.ph.hook_server_attrs(cmd, pri, cb)
     }
     /// Sets a print hook.
     ///
     /// See [`PluginHandle::hook_print`].
-    pub fn hook_print<'f, F>(&self, name: &str, pri: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_print<F>(&self, name: &str, pri: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word) -> Eat + 'ph + RefUnwindSafe {
         self.ph.hook_print(name, pri, cb)
     }
     /// Sets a print hook with attributes.
     ///
     /// See [`PluginHandle::hook_print_attrs`].
-    pub fn hook_print_attrs<'f, F>(&self, name: &str, pri: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>, Word, EventAttrs) -> Eat + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_print_attrs<F>(&self, name: &str, pri: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>, Word, EventAttrs) -> Eat + 'ph + RefUnwindSafe {
         self.ph.hook_print_attrs(name, pri, cb)
     }
     /// Sets a timer hook.
     ///
     /// See [`PluginHandle::hook_timer`].
-    pub fn hook_timer<'f, F>(&self, timeout: i32, cb: F) -> HookHandle<'ph, 'f> where F: Fn(&mut PluginHandle<'ph>) -> bool + 'f + RefUnwindSafe, 'f: 'ph {
+    pub fn hook_timer<F>(&self, timeout: i32, cb: F) -> HookHandle<'ph> where F: Fn(&mut PluginHandle<'ph>) -> bool + 'ph + RefUnwindSafe {
         self.ph.hook_timer(timeout, cb)
     }
     /// Returns context and client information.
@@ -1503,7 +1503,7 @@ impl<'a, 'ph: 'a> ValidContext<'a, 'ph> {
 // /// Userdata type used by a timer hook.
 // type TimerHookUd = Box<dyn Fn() -> bool + std::panic::RefUnwindSafe>;
 /// Userdata type used by a hook
-type HookUd<'f> = Box<dyn Fn(*const *const c_char, *const *const c_char, *const RawAttrs) -> Eat + RefUnwindSafe + 'f>;
+type HookUd<'ph> = Box<dyn Fn(*const *const c_char, *const *const c_char, *const RawAttrs) -> Eat + RefUnwindSafe + 'ph>;
 /// Contexts
 type Contexts = Rc<AssertUnwindSafe<RefCell<HashSet<Rc<*const internals::HexchatContext>>>>>;
 
@@ -1619,7 +1619,7 @@ struct PhUserdata<'ph> {
     plug: Pin<Box<dyn Plugin<'ph> + 'ph>>,
     contexts: Contexts,
     // this is never read, but we need to not drop it until we can drop it
-    _context_hook: HookHandle<'ph, 'ph>,
+    _context_hook: HookHandle<'ph>,
     pluginfo: PluginInfo,
 }
 

I wouldn't like any credit, I haven't written any of the code, I just like to examine interfaces.

In general, a panic should be treated as an unrecoverable error. The problem is that currently, if a hook panics, it simply returns a HEXCHAT_EAT_NONE, and any hooks triggered after it observe an inconsistent state. This can introduce many headaches, since each hook must check that the plugin's state is still sane; it must assume that any operation in any other hook that can panic may have just done so. So instead, I'd expect any one of these behaviors following a panic being caught:

  1. The HexChat process is immediately terminated.
  2. A suitable error message is printed, and the plugin is immediately unloaded. (This one might not be possible.)
  3. A suitable error message is printed, and all hooks are immediately unregistered.
  4. A suitable error message is printed, and all hooks only print an error message when triggered.
  5. Any other solution that prevents user callbacks from being run following the panic.

It's true. And indeed, this is what we had originally. Then we tried to let the hook borrow outer context, which was a mistake (see: pre-pooping your pants). Ah well.

Somehow we feel like leaving the 'f there is a strong reminder of our failures...

We mean you are helping with QA. QA is important! And we do think it's important to credit QA just as much as developers.

(2) would be nice, but would require changes to hexchat. The requirement that hooks be RefUnwindSafe is intended to lead to (4) - in particular we expect one to use Arc<Mutex<_>>, or any equivalent that implements poisoning.

In general, errors in hooks have always been a headache for hexchat plugin developers and users alike. However:

  • (1) is a terrible solution.
  • (2) would probably be okay.
  • (3) conflicts with our crate's architecture/design (and we're not willing to change it).
  • (4) can be annoying but unlikely to be harmful, but by making it the developer's responsibility we gain some flexibility in what parts of the plugin break when something goes wrong. indeed, a plugin could use a RwLock (or equivalent) to manage access to user configuration values, and panics in hooks that use the config (but don't set it) wouldn't prevent the user from setting the config. so if you had a bug with certain config values, the user could still intervene and not lose the whole plugin (as with (2)) or the whole client (as with (1)).
  • (5) is too unspecific.

Honestly the main things we'd be worried about at this point are:

  1. Send/Sync (we have pointers everywhere and we don't explicitly impl Send/Sync so we assume it should be fine.)
  2. LocalKey/thread_local! (we didn't always have 'ph everywhere, but we added it because of this.)
  3. anything else that's weird or something one wouldn't immediately think of. (exception: env::set_var is unsound on some platforms but eh not worried about it tbh.)

Rustdoc confirms that all public types are correctly !Send and !Sync, except for the trivial types Eat, EventAttrs<'_>, InvalidContextError, Strip, Word<'_>, WordEol<'_>, and InfoId<'_>. All of the functions seem reasonable, even though they necessarily depend on HexChat's internal behavior not documented as part of its API.

Something I'm still concerned about is UnwindSafe. Currently:

  • Eat, EventAttrs<'_>, PluginEntryHandle<'_>, PluginHandle<'_>, Strip, Word<'_>, WordEol<'_>, and InfoId<'_> are UnwindSafe and RefUnwindSafe
  • ValidContext<'_, '_> is !UnwindSafe and RefUnwindSafe
  • Context<'_> and HookHandle<'_, '_> are !UnwindSafe and !RefUnwindSafe
  • InvalidContextError<F> is UnwindSafe iff F: UnwindSafe and RefUnwindSafe iff F: RefUnwindSafe

Some of these seem suspicious to me, especially the difference between Context<'_> and ValidContext<'_, '_>. However, I'm really not sure what the correct traits should be, since the documentation is so vague about implementing them on one's own types. In any case, this has no effect on memory safety. (Regarding unwinding, though, it would be best to at least clarify somewhere in the documentation that hooks and deinit will continue to run following a panic.)

Heh. ValidContext<'_, '_> used to be called EnsureValidContext<'_, '_>. In reality, it is not a Context-like, but a PluginHandle<'_>-like (or rather &mut PluginHandle<'_>-like). Indeed it's literally a typestate wrapper around &mut PluginHandle<'_>. So whatever is inferred for it is probably fine. However...

Hmm... PluginHandle<'_> does indeed act like an &UnsafeCell of sorts... We're not sure what the appropriate unwind safety should be here. Tho user code can never observe a PluginHandle except as a &mut PluginHandle (unless you use take_mut but we're pretty sure that's unsound).

We think a more useful question to ask is whether trait Plugin should require UnwindSafe and/or RefUnwindSafe.

I probably wouldn't worry too much about unwind safety. It doesn't actually have anything to do with unsafe, and doing it "incorrectly" does not result in UB.

2 Likes

We do know that. Tho we also know having the correct bounds tends to be extremely helpful.

In this particular case, unwind safety is more important than usual, since the program proceeds normally after catching a panic; all future callbacks in the plugin will still be executed. So whenever your callback begins, any other operation in your plugin could have just panicked. Therefore, to prevent this implicit-control-flow explosion, callbacks must be RefUnwindSafe, and any shared data should be accessed through a Mutex (or another RefUnwindSafe interface) to ensure that it is not poisoned. This is my understanding of the library author's intent.

Sometimes we kinda wish we could do an fn deinit(...) where &mut Self: UnwindSafe but unfortunately we can't do that.

Unwind safety works really well for closures and, generally, structs. But when you're making the user implement a trait it kinda just falls apart...

Eh, we guess we can just make a 3.0.0 if we change our mind about the unwind safety stuff. At least it's not unsound?

But we also had to filter out the return value on Close Context hooks, because that was unsound.

thanks @LegionMammal978 ! here's hoping there are no more issues because we already cargo published it ^^

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.