From 0f8c5fe79656282e19a023d4943f2fe4481091e9 Mon Sep 17 00:00:00 2001 From: ragarnoy Date: Fri, 8 Nov 2024 18:33:51 +0100 Subject: [PATCH] Add debug assertions for null pointers, enhance doc --- src/config.rs | 2 + src/detector/distance.rs | 1 + src/detector/distance/config.rs | 1 + src/detector/presence.rs | 1 + src/detector/presence/config.rs | 4 ++ src/num.rs | 15 ++++-- src/processing.rs | 1 + src/processing/metadata.rs | 7 ++- src/radar.rs | 28 ++++++++++- src/sensor.rs | 87 ++++++++++++++------------------- 10 files changed, 89 insertions(+), 58 deletions(-) diff --git a/src/config.rs b/src/config.rs index 4da6789..0266081 100644 --- a/src/config.rs +++ b/src/config.rs @@ -73,6 +73,7 @@ impl Default for RadarConfig { impl Drop for RadarConfig { /// Destroys the radar configuration instance, freeing any allocated resources. fn drop(&mut self) { + debug_assert!(!self.inner.is_null(), "Radar configuration is null"); unsafe { acc_config_destroy(self.inner) }; } } @@ -95,6 +96,7 @@ impl RadarConfig { /// # Safety /// This function is unsafe because it returns a raw pointer. pub unsafe fn mut_ptr(&mut self) -> *mut acc_config_t { + debug_assert!(!self.inner.is_null(), "Radar configuration is null"); self.inner } diff --git a/src/detector/distance.rs b/src/detector/distance.rs index 4a2b1a5..ec56fb8 100644 --- a/src/detector/distance.rs +++ b/src/detector/distance.rs @@ -35,6 +35,7 @@ impl InnerRadarDistanceDetector { impl Drop for InnerRadarDistanceDetector { fn drop(&mut self) { + debug_assert!(!self.inner.is_null(), "Inner detector handle is null"); unsafe { acc_detector_distance_destroy(self.inner) } } } diff --git a/src/detector/distance/config.rs b/src/detector/distance/config.rs index f589792..8ae6911 100644 --- a/src/detector/distance/config.rs +++ b/src/detector/distance/config.rs @@ -94,6 +94,7 @@ pub struct RadarDistanceConfig { impl Drop for RadarDistanceConfig { fn drop(&mut self) { + debug_assert!(self.inner.is_null(), "Dropping a non-null pointer"); unsafe { acc_detector_distance_config_destroy(self.inner) } } } diff --git a/src/detector/presence.rs b/src/detector/presence.rs index c341349..70a3503 100644 --- a/src/detector/presence.rs +++ b/src/detector/presence.rs @@ -43,6 +43,7 @@ impl InnerPresenceDetector { impl Drop for InnerPresenceDetector { fn drop(&mut self) { + debug_assert!(!self.inner.is_null(), "Detector handle is null"); unsafe { acc_detector_presence_destroy(self.inner) } } } diff --git a/src/detector/presence/config.rs b/src/detector/presence/config.rs index bee992f..e74f124 100644 --- a/src/detector/presence/config.rs +++ b/src/detector/presence/config.rs @@ -23,6 +23,10 @@ pub struct PresenceConfig { impl Drop for PresenceConfig { fn drop(&mut self) { + debug_assert!( + !self.inner.is_null(), + "Presence detector configuration is null" + ); unsafe { acc_detector_presence_config_destroy(self.inner) } } } diff --git a/src/num.rs b/src/num.rs index 61c84a9..0222f7a 100644 --- a/src/num.rs +++ b/src/num.rs @@ -14,18 +14,23 @@ impl AccComplex { Self::default() } - /// Creates a new `AccComplex` instance from a raw pointer. + /// Creates a new AccComplex from a raw pointer. /// /// # Safety - /// This function is unsafe because it dereferences a raw pointer. - /// The caller must ensure that the pointer is valid and points to a properly initialized `acc_int16_complex_t` struct. + /// - ptr must be a valid pointer to an initialized acc_int16_complex_t + /// - The pointed-to data must be valid for the lifetime of the returned AccComplex + /// - The pointer must be properly aligned pub unsafe fn from_ptr(ptr: *const acc_int16_complex_t) -> Self { + debug_assert!(!ptr.is_null(), "Pointer is null"); Self { inner: *ptr } } - /// Returns a mutable pointer to the inner `acc_int16_complex_t` struct. + /// Returns a mutable pointer to the inner complex number data. + /// /// # Safety - /// This function is unsafe because it returns a raw pointer. + /// - The pointer is only valid for the lifetime of this AccComplex instance + /// - The caller must ensure no other references to the inner data exist + /// - The data must not be modified in a way that violates AccComplex invariants pub unsafe fn mut_ptr(&mut self) -> *mut acc_int16_complex_t { &mut self.inner } diff --git a/src/processing.rs b/src/processing.rs index 1c5de4f..9c1ea66 100644 --- a/src/processing.rs +++ b/src/processing.rs @@ -79,6 +79,7 @@ impl Processing { impl Drop for Processing { fn drop(&mut self) { + debug_assert!(!self.inner.is_null(), "Processing is null"); unsafe { acc_processing_destroy(self.inner); } diff --git a/src/processing/metadata.rs b/src/processing/metadata.rs index cc99531..d03afc1 100644 --- a/src/processing/metadata.rs +++ b/src/processing/metadata.rs @@ -24,9 +24,12 @@ impl ProcessingMetaData { Self::default() } - /// Get a mutable reference to the metadata + /// Gets a mutable pointer to the underlying metadata structure. + /// /// # Safety - /// This function is unsafe because it returns a mutable reference to the metadata, which is a raw pointer + /// - The caller must ensure the pointer is only used while ProcessingMetaData exists + /// - The pointer must only be used for passing to Acconeer API functions + /// - No other references to the data can exist while the pointer is in use pub unsafe fn mut_ptr(&mut self) -> *mut acc_processing_metadata_t { &mut self.inner } diff --git a/src/radar.rs b/src/radar.rs index 46aa92b..2b7c12a 100644 --- a/src/radar.rs +++ b/src/radar.rs @@ -211,6 +211,25 @@ where ENABLE: OutputPin, DLY: DelayNs, { + /// Starts a radar measurement with a previously prepared configuration. + /// + /// This function initiates a radar measurement based on a configuration that must have been + /// set up and prepared in advance. Ensure the sensor is powered on and calibration and + /// preparation steps have been completed before calling this function. + /// + /// # Preconditions + /// + /// - The sensor must be powered on. + /// - `calibrate` must have been successfully called. + /// - `prepare` must have been successfully called. + /// + /// # Arguments + /// + /// * `sensor` - The sensor instance to use for measurement. + /// + /// # Returns + /// + /// `Ok(())` if the measurement was successfully started, `Err(SensorError)` otherwise. pub async fn measure<'a>(&mut self, data: &mut [u8]) -> Result<(), SensorError> { if (self.sensor.measure(&mut self.interrupt).await).is_ok() { if self.sensor.read(data).is_ok() { @@ -284,10 +303,15 @@ where self.sensor.check_status(); } - /// Get a mutable reference to the sensor + /// Gets a mutable pointer to the underlying sensor. + /// /// # Safety - /// This function is unsafe because it returns a mutable reference to the sensor, which is a raw pointer + /// - The returned pointer is only valid while the Radar instance exists + /// - The pointer must not be used after the sensor is disabled or reset + /// - Only one mutable reference can exist at a time + /// - The caller must not violate the state transition requirements pub unsafe fn inner_sensor(&self) -> *mut acc_sensor_t { + debug_assert!(!self.sensor.inner().is_null(), "Sensor pointer is null"); self.sensor.inner() } } diff --git a/src/sensor.rs b/src/sensor.rs index 4752614..7e9c27f 100644 --- a/src/sensor.rs +++ b/src/sensor.rs @@ -15,23 +15,35 @@ use a121_sys::*; pub mod calibration; pub mod error; -struct InnerSensor { +/// Safety-critical wrapper around raw sensor pointer +pub struct InnerSensor { + /// Raw pointer to acc_sensor_t. Must never be null once initialized. + /// Managed exclusively by this type to maintain memory safety. inner: *mut acc_sensor_t, } impl InnerSensor { + /// Creates a new sensor instance from a sensor ID. + /// + /// # Safety + /// - The returned pointer must be valid and non-null + /// - The sensor must be powered on before calling this function + /// - The sensor must not be used in another sensor instance without a power/reset cycle pub fn new(sensor_id: u32) -> Option { let sensor_ptr = unsafe { acc_sensor_create(sensor_id as acc_sensor_id_t) }; + + // Runtime safety check for null pointer if sensor_ptr.is_null() { - None - } else { - Some(Self { inner: sensor_ptr }) + return None; } + + Some(Self { inner: sensor_ptr }) } } impl Drop for InnerSensor { fn drop(&mut self) { + debug_assert!(!self.inner.is_null(), "Sensor pointer is null in drop"); unsafe { acc_sensor_destroy(self.inner); } @@ -41,12 +53,22 @@ impl Drop for InnerSensor { impl Deref for InnerSensor { type Target = acc_sensor_t; + /// # Safety + /// Dereference is safe because: + /// - inner is guaranteed non-null by constructor + /// - exclusive access is maintained by Rust ownership rules + /// - pointer remains valid until drop fn deref(&self) -> &Self::Target { unsafe { &*self.inner } } } impl DerefMut for InnerSensor { + /// # Safety + /// Dereference is safe because: + /// - inner is guaranteed non-null by constructor + /// - exclusive access is maintained by Rust ownership rules + /// - pointer remains valid until drop fn deref_mut(&mut self) -> &mut Self::Target { unsafe { &mut *self.inner } } @@ -246,37 +268,6 @@ where } } - /// Starts a radar measurement with a previously prepared configuration. - /// - /// This function initiates a radar measurement based on a configuration that must have been - /// set up and prepared in advance. Ensure the sensor is powered on and calibration and - /// preparation steps have been completed before calling this function. - /// - /// # Preconditions - /// - /// - The sensor must be powered on. - /// - `calibrate` must have been successfully called. - /// - `prepare` must have been successfully called. - /// - /// # Arguments - /// - /// * `sensor` - The sensor instance to use for measurement. - /// - /// # Returns - /// - /// `Ok(())` if the measurement was successfully started, `Err(SensorError)` otherwise. - /// - /// # Example - /// - /// ```no_run - /// # use embedded_hal_async::digital::Wait; - /// use rad_hard_sys::sensor::*; - /// use rad_hard_sys::sensor::error::SensorError; - /// async fn foo(sensor: &mut Sensor) -> Result<(), SensorError> { - /// sensor.measure().await?; - /// # Ok(()) - /// # } - /// ``` pub async fn measure(&mut self, mut interrupt: SINT) -> Result<(), SensorError> { // Implementation to start the radar measurement let success = unsafe { acc_sensor_measure(self.inner.deref_mut()) }; @@ -307,20 +298,6 @@ where /// /// # Returns /// `Ok(())` if data was successfully read into the buffer, `Err(SensorError)` otherwise. - /// - /// # Example - /// - /// ```no_run - /// # use embedded_hal_async::digital::Wait; - /// use rad_hard_sys::sensor::*; - /// use rad_hard_sys::sensor::data::RadarData; - /// use rad_hard_sys::sensor::error::SensorError; - /// async fn foo(sensor: &mut Sensor) -> Result<(), SensorError> { - /// let mut data_buffer = RadarData::default(); - /// sensor.read(&mut data_buffer)?; - /// # Ok(()) - /// # } - /// ``` pub fn read(&self, buffer: &mut [u8]) -> Result<(), SensorError> { // Implementation to read the radar data let success = unsafe { @@ -337,7 +314,19 @@ where } } + /// Returns a mutable pointer to the underlying sensor. + /// + /// # Safety + /// This function is unsafe because: + /// - The caller must ensure the sensor has been properly initialized + /// - The pointer remains valid only for the lifetime of this Sensor instance + /// - The caller must not use this pointer after the Sensor is dropped + /// - Multiple mutable references to the same sensor must not be created pub unsafe fn inner(&self) -> *mut acc_sensor_t { + debug_assert!( + !self.inner.inner.is_null(), + "Sensor has not been initialized" + ); self.inner.inner } }