From d0206b4e346aa844ddc62bc107d73946708e9aaf Mon Sep 17 00:00:00 2001 From: Hugo Hakim Damer Date: Thu, 28 Nov 2024 15:22:55 +0100 Subject: [PATCH] fix(context): don't explicitly free endpoints when dropping context Explicitly freeing the endpoints before calling coap_free_context() may cause a SIGABRT if sessions are still referenced somewhere (e.g., due to the session still being observed). This change decomposes the CoapEndpoint instances into their raw variant without calling coap_free_endpoint() on dropping the context so that libcoap can perform its own cleanup before freeing the endpoints itself in coap_free_context(). --- libcoap/src/context.rs | 28 +++++++++++++++++++--------- libcoap/src/transport.rs | 12 +++++++++++- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/libcoap/src/context.rs b/libcoap/src/context.rs index 48dcd1ec..1fc72f5b 100644 --- a/libcoap/src/context.rs +++ b/libcoap/src/context.rs @@ -13,7 +13,7 @@ use std::ffi::CString; #[cfg(dtls)] use std::ptr::NonNull; -use std::{any::Any, ffi::c_void, fmt::Debug, net::SocketAddr, ops::Sub, sync::Once, time::Duration}; +use std::{any::Any, ffi::c_void, fmt::Debug, net::SocketAddr, sync::Once, time::Duration}; #[cfg(all(feature = "dtls-pki", unix))] use std::{os::unix::ffi::OsStrExt, path::Path}; @@ -25,9 +25,10 @@ use libcoap_sys::{ coap_context_get_max_handshake_sessions, coap_context_get_max_idle_sessions, coap_context_get_session_timeout, coap_context_set_block_mode, coap_context_set_csm_max_message_size, coap_context_set_csm_timeout, coap_context_set_keepalive, coap_context_set_max_handshake_sessions, coap_context_set_max_idle_sessions, - coap_context_set_session_timeout, coap_context_t, coap_event_t, coap_free_context, coap_get_app_data, - coap_io_process, coap_new_context, coap_proto_t, coap_register_event_handler, coap_register_response_handler, - coap_set_app_data, coap_startup_with_feature_checks, COAP_BLOCK_SINGLE_BODY, COAP_BLOCK_USE_LIBCOAP, COAP_IO_WAIT, + coap_context_set_session_timeout, coap_context_t, coap_endpoint_t, coap_event_t, coap_free_context, + coap_get_app_data, coap_io_process, coap_new_context, coap_proto_t, coap_register_event_handler, + coap_register_response_handler, coap_set_app_data, coap_startup_with_feature_checks, COAP_BLOCK_SINGLE_BODY, + COAP_BLOCK_USE_LIBCOAP, COAP_IO_WAIT, }; #[cfg(any(feature = "dtls-rpk", feature = "dtls-pki"))] @@ -346,14 +347,19 @@ impl CoapContext<'_> { /// Performs a controlled shutdown of the CoAP context. /// /// This will perform all still outstanding IO operations until [coap_can_exit()] confirms that - /// the context has no more outstanding IO and can be dropped without interrupting sessions. + /// the context has no more outstanding IO and can be dropped. pub fn shutdown(mut self, exit_wait_timeout: Option) -> Result<(), IoProcessError> { let mut remaining_time = exit_wait_timeout; // Send remaining packets until we can cleanly shutdown. // SAFETY: Provided context is always valid as an invariant of this struct. - while unsafe { coap_can_exit(self.inner.borrow_mut().raw_context) } == 0 { + let raw_context = self.inner.borrow_mut().raw_context; + + while unsafe { coap_can_exit(raw_context) } == 0 { let spent_time = self.do_io(remaining_time)?; - remaining_time = remaining_time.map(|v| v.sub(spent_time)); + remaining_time = remaining_time.map(|v| v.saturating_sub(spent_time)); + if remaining_time.is_some_and(|v| v.is_zero()) { + break; + } } Ok(()) } @@ -652,8 +658,12 @@ impl Drop for CoapContextInner<'_> { for session in std::mem::take(&mut self.server_sessions).into_iter() { session.drop_exclusively(); } - // Clear endpoints because coap_free_context() would free their underlying raw structs. - self.endpoints.clear(); + // Decompose endpoint wrappers into raw values (can't drop them directly, as doing so might + // not work as long as server-side sessions are still active). + let _endpoints: Vec<*mut coap_endpoint_t> = std::mem::take(&mut self.endpoints) + .into_iter() + .map(|v| v.into_raw()) + .collect(); // Extract reference to CoapContextInner from raw context and drop it. // SAFETY: Value is set upon construction of the inner context and never deleted. unsafe { diff --git a/libcoap/src/transport.rs b/libcoap/src/transport.rs index 23e26aa3..0137a413 100644 --- a/libcoap/src/transport.rs +++ b/libcoap/src/transport.rs @@ -56,11 +56,21 @@ impl CoapEndpoint { Ok(Self { raw_endpoint: endpoint }) } } + + /// Decomposes this endpoint into its raw version. + /// + /// Note that the caller is now responsible for freeing the endpoint (either by calling + /// coap_free_endpoint() or calling coap_free_context() on the associated context). + pub(crate) fn into_raw(mut self) -> *mut coap_endpoint_t { + std::mem::replace(&mut self.raw_endpoint, std::ptr::null_mut()) + } } impl Drop for CoapEndpoint { fn drop(&mut self) { // SAFETY: Raw endpoint is guaranteed to exist for as long as the container exists. - unsafe { coap_free_endpoint(self.raw_endpoint) } + if !self.raw_endpoint.is_null() { + unsafe { coap_free_endpoint(self.raw_endpoint) } + } } }