fix(event): own graph guard-condition registrations instead of raw pointers

Moving the guard-condition trigger out of the registry lock was necessary --
the trigger is rmw code that re-enters hiroz -- but it turned a deadlock into
a use-after-free.

`trigger_graph_change` snapshotted `Vec<usize>` of raw pointers, released the
lock, then dereferenced them. The lock was the only thing serialising that
against teardown: `rmw_destroy_node` calls
`unregister_graph_guard_condition` and then immediately
`rmw_destroy_guard_condition`, which frees the handle. Previously `unregister`
blocked until triggering finished, so the free could not land mid-trigger.
Afterwards it could, and the trigger wrote through freed memory. Concurrent
triggers also aliased `&mut GuardConditionImpl`.

Make registrations owned. hiroz gains a `GraphGuardCondition` trait and stores
`Arc<dyn GraphGuardCondition>`; the snapshot clones `Arc`s, so an in-flight
trigger keeps its target alive no matter what teardown does. `unregister` is
by `Arc::ptr_eq` and no longer implies "no trigger is running" -- it does not
need to, because the survivor keeps the object alive.

rmw-zenoh-rs splits `GuardConditionImpl` into a C-side handle and an
`Arc<GuardConditionState>` holding the notifier and an `AtomicBool`. The node
registers a clone of that state and keeps one itself for unregistration.
`triggered` becomes atomic because triggering no longer happens under any
lock. The process-wide `set_guard_condition_trigger` indirection is gone --
each registration now carries its own behaviour -- so
`GraphGuardConditionTrigger` is removed.

Covered by a unit test pinning the ownership contract: the registry keeps the
value alive after the registrant drops its handle, and releases it on
unregister. That property is what makes the destroy-during-trigger race
harmless, and unlike the race itself it is deterministic to assert.

Author: YuanYuYuan

crates/hiroz/src/event.rs

@@ -177,19 +177,37 @@ impl EventsManager {
     }
 }
 
-// Callback type for triggering graph guard conditions.
-//
-// `Arc` for the same reason as [`EventCallback`]: it is cloned out of its
-// `Mutex` before being called, so guard-condition triggers never run with the
-// graph-event registry locked.
-pub type GraphGuardConditionTrigger = Arc<dyn Fn(*mut std::ffi::c_void) + Send + Sync>;
+/// A graph guard condition this manager may trigger on a graph change.
+///
+/// Registrations are **owned**, not raw pointers, and that is the whole point.
+/// Triggering happens after the registry lock is released — it has to, because
+/// the trigger is rmw-side code that re-enters hiroz. But a raw pointer cloned
+/// out of the lock is only valid while something guarantees the target outlives
+/// the call, and nothing did: `rmw_destroy_node` unregisters and then
+/// immediately frees the guard condition, so a destroy landing between the
+/// snapshot and the call left the trigger dereferencing freed memory.
+///
+/// Holding an `Arc` for the duration of the call closes that window without
+/// reintroducing the lock: the implementation's state stays alive as long as
+/// this manager holds a reference, even if the C-side handle is destroyed
+/// concurrently. Implementors must therefore keep [`trigger`] valid after the
+/// owning C object is gone — the natural shape is state behind its own `Arc`,
+/// with the C handle holding one reference and this registry another.
+///
+/// [`trigger`]: GraphGuardCondition::trigger
+pub trait GraphGuardCondition: Send + Sync {
+    /// Wake whatever is waiting on this guard condition.
+    ///
+    /// Called with no hiroz lock held, possibly concurrently, and possibly
+    /// after the corresponding C handle has been destroyed.
+    fn trigger(&self);
+}
 
 // GraphCache event integration
 pub struct GraphEventManager {
     event_callbacks: TrackedMutex<HashMap<GidArray, HashMap<ZenohEventType, EventCallback>>>,
     entity_topics: TrackedMutex<HashMap<GidArray, String>>, // Topic name per registered entity
-    graph_guard_conditions: TrackedMutex<Vec<usize>>,       // Pointers as usize for Send
-    trigger_guard_condition: TrackedMutex<Option<GraphGuardConditionTrigger>>,
+    graph_guard_conditions: TrackedMutex<Vec<Arc<dyn GraphGuardCondition>>>,
 }
 
 impl Default for GraphEventManager {
@@ -204,14 +222,9 @@ impl GraphEventManager {
             event_callbacks: TrackedMutex::new(HashMap::new()),
             entity_topics: TrackedMutex::new(HashMap::new()),
             graph_guard_conditions: TrackedMutex::new(Vec::new()),
-            trigger_guard_condition: TrackedMutex::new(None),
         }
     }
 
-    pub fn set_guard_condition_trigger(&self, trigger: GraphGuardConditionTrigger) {
-        *self.trigger_guard_condition.lock().unwrap() = Some(trigger);
-    }
-
     pub fn register_event_callback<F>(
         &self,
         entity_gid: GidArray,
@@ -244,15 +257,27 @@ impl GraphEventManager {
         topics.remove(entity_gid);
     }
 
-    pub fn register_graph_guard_condition(&self, guard_condition: *mut std::ffi::c_void) {
+    /// Register a guard condition to be triggered on every graph change.
+    ///
+    /// The manager keeps the `Arc` alive for as long as it is registered, and
+    /// for the duration of any trigger already in flight — see
+    /// [`GraphGuardCondition`] for why that ownership is load-bearing.
+    pub fn register_graph_guard_condition(&self, guard_condition: Arc<dyn GraphGuardCondition>) {
         let mut conditions = self.graph_guard_conditions.lock().unwrap();
-        conditions.push(guard_condition as usize);
+        conditions.push(guard_condition);
     }
 
-    pub fn unregister_graph_guard_condition(&self, guard_condition: *mut std::ffi::c_void) {
+    /// Stop triggering `guard_condition`.
+    ///
+    /// Identity is `Arc::ptr_eq`, so the caller must pass the same allocation it
+    /// registered. Returning does **not** mean no trigger is in flight: a
+    /// concurrent [`Self::trigger_graph_change`] may already hold its own clone
+    /// and be calling into it. That is exactly why the registration is owned —
+    /// the in-flight call keeps the target alive, so a caller that frees its own
+    /// handle immediately after this returns is still safe.
+    pub fn unregister_graph_guard_condition(&self, guard_condition: &Arc<dyn GraphGuardCondition>) {
         let mut conditions = self.graph_guard_conditions.lock().unwrap();
-        let gc_usize = guard_condition as usize;
-        conditions.retain(|&gc| gc != gc_usize);
+        conditions.retain(|gc| !Arc::ptr_eq(gc, guard_condition));
     }
 
     pub fn trigger_event(&self, entity_gid: &GidArray, event_type: ZenohEventType, change: i32) {
@@ -309,15 +334,16 @@ impl GraphEventManager {
         let change = if appeared { 1 } else { -1 };
 
         // Trigger graph guard conditions for ALL graph changes (local and remote).
-        // Snapshot the trigger and the pointer list, then release both locks before
-        // calling out — the trigger is rmw-side code and may re-enter.
-        let trigger = self.trigger_guard_condition.lock().unwrap().clone();
-        if let Some(trigger) = trigger {
-            let guard_conditions = self.graph_guard_conditions.lock().unwrap().clone();
-            for gc_usize in guard_conditions {
-                let gc = gc_usize as *mut std::ffi::c_void;
-                trigger(gc);
-            }
+        //
+        // Snapshot, release the lock, then call — the trigger is rmw-side code
+        // and may re-enter this manager, so it must not run under the guard.
+        // The snapshot clones `Arc`s rather than raw pointers, which is what
+        // makes releasing the lock safe: a concurrent `rmw_destroy_node` can
+        // unregister and free its C handle here, and each in-flight trigger
+        // still holds the target alive until it returns.
+        let guard_conditions = self.graph_guard_conditions.lock().unwrap().clone();
+        for gc in guard_conditions {
+            gc.trigger();
         }
 
         // Determine which event type based on entity kind
@@ -696,4 +722,55 @@ mod tests {
             .update_event_status(ZenohEventType::LivelinessChanged, 3);
         assert_eq!(*fired.lock().unwrap(), 3);
     }
+
+    /// The graph-guard-condition registry must **own** what it registers.
+    ///
+    /// This is the invariant that makes triggering outside the lock safe.
+    /// `trigger_graph_change` snapshots the registrations, releases the lock,
+    /// and only then calls them; meanwhile `rmw_destroy_node` may unregister
+    /// and free its C handle. When the registry held raw pointers, that window
+    /// was a use-after-free. Holding an `Arc` closes it — an in-flight trigger
+    /// keeps the target alive regardless of what the registrant does.
+    ///
+    /// The test pins the ownership half of that contract, which is the part
+    /// that is deterministic: the registry keeps the value alive after the
+    /// registrant drops its handle, and releases it on unregister. It does not
+    /// attempt to schedule the destroy-during-trigger race itself — that would
+    /// be a timing test, and the ownership property is what makes the race
+    /// harmless in the first place.
+    #[test]
+    fn graph_guard_condition_registration_is_owned_by_the_manager() {
+        use std::sync::atomic::{AtomicUsize, Ordering};
+
+        struct CountingGc(Arc<AtomicUsize>);
+        impl GraphGuardCondition for CountingGc {
+            fn trigger(&self) {
+                self.0.fetch_add(1, Ordering::SeqCst);
+            }
+        }
+
+        let mgr = GraphEventManager::new();
+        let hits = Arc::new(AtomicUsize::new(0));
+        let gc: Arc<dyn GraphGuardCondition> = Arc::new(CountingGc(hits.clone()));
+        let weak = Arc::downgrade(&gc);
+
+        mgr.register_graph_guard_condition(gc.clone());
+        drop(gc);
+        let held = weak.upgrade().expect(
+            "the manager must keep the registration alive after the registrant drops its handle; \
+             otherwise a trigger issued outside the lock dereferences freed memory",
+        );
+
+        // Still reachable and callable through the registry's own reference.
+        held.trigger();
+        assert_eq!(hits.load(Ordering::SeqCst), 1);
+
+        mgr.unregister_graph_guard_condition(&held);
+        drop(held);
+        assert!(
+            weak.upgrade().is_none(),
+            "unregister must release the registry's reference, or registrations leak for the \
+             life of the process",
+        );
+    }
 }

crates/rmw-zenoh-rs/src/context.rs

@@ -95,18 +95,6 @@ impl ContextImpl {
             .build()
             .map_err(|e| format!("Failed to create ZContext: {}", e))?;
 
-        // Set up the guard condition trigger function for graph events
-        // This allows graph changes to trigger RMW guard conditions
-        let trigger_fn: hiroz::event::GraphGuardConditionTrigger = Arc::new(|gc_ptr| {
-            crate::guard_condition::rmw_trigger_guard_condition(
-                gc_ptr as *const crate::ros::rmw_guard_condition_t,
-            );
-        });
-        zcontext
-            .graph()
-            .event_manager
-            .set_guard_condition_trigger(trigger_fn);
-
         Ok(Self {
             zcontext: Arc::new(zcontext),
             enclave,

crates/rmw-zenoh-rs/src/guard_condition.rs

@@ -3,30 +3,76 @@ use crate::ros::*;
 use crate::traits::*;
 use crate::utils::Notifier;
 use std::sync::Arc;
+use std::sync::atomic::{AtomicBool, Ordering};
+
+/// The triggerable state of a guard condition, separated from the C handle.
+///
+/// This lives behind an `Arc` so it can outlive `rmw_destroy_guard_condition`.
+/// hiroz's graph-event manager registers a clone (as
+/// `Arc<dyn GraphGuardCondition>`) and triggers it **after** dropping its
+/// registry lock; without shared ownership, a `rmw_destroy_node` racing that
+/// window would free the target and the trigger would write through dangling
+/// memory. The C handle holds one reference and the registry another, so
+/// whichever outlives the other, the state is valid for the whole call.
+///
+/// `triggered` is atomic because triggering no longer happens under any lock:
+/// a graph-event thread can set it while `rmw_wait` reads it.
+#[derive(Debug, Default)]
+pub struct GuardConditionState {
+    pub(crate) notifier: Option<Arc<Notifier>>,
+    pub(crate) triggered: AtomicBool,
+}
+
+impl GuardConditionState {
+    pub(crate) fn fire(&self) -> Result<(), ()> {
+        let notifier = self.notifier.as_ref().ok_or(())?;
+        self.triggered.store(true, Ordering::SeqCst);
+        notifier.notify_all();
+        Ok(())
+    }
+
+    pub(crate) fn reset(&self) {
+        self.triggered.store(false, Ordering::SeqCst);
+    }
+
+    pub(crate) fn is_triggered(&self) -> bool {
+        self.triggered.load(Ordering::SeqCst)
+    }
+}
+
+impl hiroz::event::GraphGuardCondition for GuardConditionState {
+    fn trigger(&self) {
+        // A guard condition with no notifier cannot wake anyone; that is not an
+        // error worth propagating across the registry.
+        let _ = self.fire();
+    }
+}
 
 /// Guard condition implementation for RMW
 #[derive(Debug, Default)]
 pub struct GuardConditionImpl {
-    pub(crate) notifier: Option<Arc<Notifier>>,
-    pub(crate) triggered: bool,
+    pub(crate) state: Arc<GuardConditionState>,
 }
 
 impl GuardConditionImpl {
     pub(crate) fn trigger(&mut self) -> Result<(), ()> {
-        let notifier = self.notifier.as_ref().ok_or(())?;
-        self.triggered = true;
-        notifier.notify_all();
-        Ok(())
+        self.state.fire()
     }
 
     pub fn reset(&mut self) {
-        self.triggered = false;
+        self.state.reset();
+    }
+
+    /// A shared handle to this guard condition's state, for registration with
+    /// hiroz's graph-event manager.
+    pub(crate) fn share_state(&self) -> Arc<GuardConditionState> {
+        self.state.clone()
     }
 }
 
 impl crate::traits::Waitable for GuardConditionImpl {
     fn is_ready(&self) -> bool {
-        self.triggered
+        self.state.is_triggered()
     }
 }
 
@@ -52,8 +98,10 @@ pub extern "C" fn rmw_create_guard_condition(
 
     let notifier = Some(context_impl.share_notifier());
     let gc_impl = GuardConditionImpl {
-        notifier,
-        triggered: false,
+        state: Arc::new(GuardConditionState {
+            notifier,
+            triggered: AtomicBool::new(false),
+        }),
     };
     let gc = Box::new(rmw_guard_condition_t {
         implementation_identifier: crate::RMW_ZENOH_IDENTIFIER.as_ptr() as *const _,

crates/rmw-zenoh-rs/src/node.rs

@@ -12,6 +12,14 @@ pub struct NodeImpl {
     pub namespace: CString,
     pub fq_name: CString,
     pub graph_guard_condition: *mut rmw_guard_condition_t,
+    /// The shared state registered with hiroz's graph-event manager.
+    ///
+    /// Kept so teardown can unregister the exact allocation it registered
+    /// (identity is `Arc::ptr_eq`). Holding it here also means the state
+    /// survives `rmw_destroy_guard_condition` below, which is what makes it safe
+    /// to free the C handle while a graph-change trigger may still be in flight.
+    pub graph_guard_condition_state:
+        Option<std::sync::Arc<dyn hiroz::event::GraphGuardCondition>>,
 }
 
 impl NodeImpl {
@@ -44,6 +52,7 @@ impl NodeImpl {
             namespace: namespace_cstr,
             fq_name: fq_name_cstr,
             graph_guard_condition: std::ptr::null_mut(),
+            graph_guard_condition_state: None,
         })
     }
 }
@@ -122,12 +131,23 @@ pub extern "C" fn rmw_create_node(
     }
     node_impl.graph_guard_condition = graph_guard_condition;
 
-    // Register the graph guard condition with the graph event manager
+    // Register the graph guard condition with the graph event manager.
+    //
+    // Register the *shared state*, not the C pointer: the manager triggers with
+    // its registry lock released, so a raw pointer could be freed by a
+    // concurrent `rmw_destroy_node` between snapshot and call. Handing over an
+    // `Arc` keeps the target alive for the duration of any in-flight trigger.
+    let gc_state: std::sync::Arc<dyn hiroz::event::GraphGuardCondition> =
+        match graph_guard_condition.borrow_data() {
+            Ok(gc_impl) => gc_impl.share_state(),
+            Err(_) => return std::ptr::null_mut(),
+        };
+    node_impl.graph_guard_condition_state = Some(gc_state.clone());
     node_impl
         .inner
         .graph()
         .event_manager
-        .register_graph_guard_condition(graph_guard_condition as *mut std::ffi::c_void);
+        .register_graph_guard_condition(gc_state);
 
     // Add node to local graph for immediate discovery
     if let Err(e) = node_impl
@@ -175,7 +195,7 @@ pub extern "C" fn rmw_destroy_node(node: *mut rmw_node_t) -> rmw_ret_t {
     }
 
     // Remove node from local graph and destroy the graph guard condition
-    if let Ok(node_impl) = node.borrow_data() {
+    if let Ok(node_impl) = node.borrow_mut_data() {
         // Remove node from local graph
         if let Err(e) = node_impl
             .inner
@@ -188,14 +208,17 @@ pub extern "C" fn rmw_destroy_node(node: *mut rmw_node_t) -> rmw_ret_t {
         }
 
         if !node_impl.graph_guard_condition.is_null() {
-            // Unregister from graph event manager
-            node_impl
-                .inner
-                .graph()
-                .event_manager
-                .unregister_graph_guard_condition(
-                    node_impl.graph_guard_condition as *mut std::ffi::c_void,
-                );
+            // Unregister from graph event manager, by the same allocation we
+            // registered. Destroying the C handle immediately afterwards is
+            // safe even if a graph-change trigger is in flight: that trigger
+            // holds its own `Arc` to the state, which outlives this handle.
+            if let Some(gc_state) = node_impl.graph_guard_condition_state.take() {
+                node_impl
+                    .inner
+                    .graph()
+                    .event_manager
+                    .unregister_graph_guard_condition(&gc_state);
+            }
             crate::guard_condition::rmw_destroy_guard_condition(node_impl.graph_guard_condition);
         }
     }