From bcfb301da070d43c2fff42e8da33b397a4806d11 Mon Sep 17 00:00:00 2001 From: Sympatron GmbH <35803463+Sympatron@users.noreply.github.com> Date: Sat, 28 Sep 2024 16:42:56 +0200 Subject: [PATCH 1/4] Fix unsoundness in PdInfo::as_struct() --- libosdp/src/pdinfo.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/libosdp/src/pdinfo.rs b/libosdp/src/pdinfo.rs index cb4ba29..98b5697 100644 --- a/libosdp/src/pdinfo.rs +++ b/libosdp/src/pdinfo.rs @@ -250,19 +250,21 @@ impl PdInfoBuilder { impl PdInfo { /// Get a C-repr struct for PdInfo that LibOSDP can operate on. pub fn as_struct(&mut self) -> libosdp_sys::osdp_pd_info_t { - let scbk; - if let Some(key) = self.scbk.as_mut() { - scbk = key.as_mut_ptr(); + let scbk = if let Some(key) = self.scbk { + Box::into_raw(Box::new(key)) as *mut _ } else { - scbk = std::ptr::null_mut::(); - } + std::ptr::null_mut::() + }; + let mut cap = self.cap.clone(); + let cap_ptr = cap.as_mut_ptr(); + core::mem::forget(cap); libosdp_sys::osdp_pd_info_t { - name: self.name.as_ptr(), + name: self.name.clone().into_raw(), baud_rate: self.baud_rate, address: self.address, flags: self.flags.bits() as i32, id: self.id.into(), - cap: self.cap.as_ptr(), + cap: cap_ptr, channel: self.channel.take().unwrap().into(), scbk, } From aae2afa87f4c5f1806322da0eda99ff2554524e9 Mon Sep 17 00:00:00 2001 From: Sympatron GmbH <35803463+Sympatron@users.noreply.github.com> Date: Sat, 28 Sep 2024 18:34:17 +0200 Subject: [PATCH 2/4] Always add sentinel to capability list before passing it to C --- libosdp/src/pdinfo.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/libosdp/src/pdinfo.rs b/libosdp/src/pdinfo.rs index 98b5697..30f1fa3 100644 --- a/libosdp/src/pdinfo.rs +++ b/libosdp/src/pdinfo.rs @@ -5,7 +5,7 @@ use alloc::ffi::CString; -use crate::{Channel, OsdpError, OsdpFlag, PdCapability, PdId}; +use crate::{Box, Channel, OsdpError, OsdpFlag, PdCapability, PdId}; /// OSDP PD Information. This struct is used to describe a PD to LibOSDP #[derive(Debug, Default)] @@ -253,18 +253,27 @@ impl PdInfo { let scbk = if let Some(key) = self.scbk { Box::into_raw(Box::new(key)) as *mut _ } else { - std::ptr::null_mut::() + core::ptr::null_mut::() + }; + let cap = if !self.cap.is_empty() { + let mut cap = self.cap.clone(); + cap.reserve(1); + cap.push(libosdp_sys::osdp_pd_cap { + function_code: -1i8 as u8, + compliance_level: 0, + num_items: 0, + }); + Box::into_raw(cap.into_boxed_slice()) as *mut _ + } else { + core::ptr::null_mut::() }; - let mut cap = self.cap.clone(); - let cap_ptr = cap.as_mut_ptr(); - core::mem::forget(cap); libosdp_sys::osdp_pd_info_t { name: self.name.clone().into_raw(), baud_rate: self.baud_rate, address: self.address, flags: self.flags.bits() as i32, id: self.id.into(), - cap: cap_ptr, + cap: cap as *mut _, channel: self.channel.take().unwrap().into(), scbk, } From 909fd418c24d971a4808904110a0c7fd6394aa9a Mon Sep 17 00:00:00 2001 From: Sympatron GmbH <35803463+Sympatron@users.noreply.github.com> Date: Wed, 2 Oct 2024 09:45:46 +0200 Subject: [PATCH 3/4] Manually drop osdp_pd_info_t to prevent leaks --- libosdp/src/cp.rs | 3 +++ libosdp/src/pd.rs | 4 +++- libosdp/src/pdinfo.rs | 24 ++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/libosdp/src/cp.rs b/libosdp/src/cp.rs index 0a611b8..4e5b2a6 100644 --- a/libosdp/src/cp.rs +++ b/libosdp/src/cp.rs @@ -57,6 +57,9 @@ where fn cp_setup(info: Vec) -> Result<*mut c_void> { let ctx = unsafe { libosdp_sys::osdp_cp_setup(info.len() as i32, info.as_ptr()) }; + for pd in info.into_iter() { + crate::drop_osdp_pd_info(pd); + } if ctx.is_null() { Err(OsdpError::Setup) } else { diff --git a/libosdp/src/pd.rs b/libosdp/src/pd.rs index e341788..a3bec3c 100644 --- a/libosdp/src/pd.rs +++ b/libosdp/src/pd.rs @@ -58,7 +58,9 @@ where } fn pd_setup(mut info: PdInfo) -> Result<*mut c_void> { - let ctx = unsafe { libosdp_sys::osdp_pd_setup(&info.as_struct()) }; + let info_struct = info.as_struct(); + let ctx = unsafe { libosdp_sys::osdp_pd_setup(&info_struct) }; + crate::drop_osdp_pd_info(info_struct); if ctx.is_null() { Err(OsdpError::Setup) } else { diff --git a/libosdp/src/pdinfo.rs b/libosdp/src/pdinfo.rs index 30f1fa3..cff3449 100644 --- a/libosdp/src/pdinfo.rs +++ b/libosdp/src/pdinfo.rs @@ -279,3 +279,27 @@ impl PdInfo { } } } + +pub(crate) fn drop_osdp_pd_info(info: libosdp_sys::osdp_pd_info_t) { + unsafe { + // The name is not copied by LibOSDP, so we cannot drop it here + // if !info.name.is_null() { + // drop(CString::from_raw(info.name as *mut _)); + // } + if !info.cap.is_null() { + let mut cap = info.cap as *mut libosdp_sys::osdp_pd_cap; + while (*cap).function_code != -1i8 as u8 { + cap = cap.add(1); + } + let len = (cap.offset_from(info.cap) + 1) as usize; + drop(Vec::from_raw_parts( + info.cap as *mut libosdp_sys::osdp_pd_cap, + len, + len, + )); + } + if !info.scbk.is_null() { + drop(Box::from_raw(info.scbk as *mut [u8; 16])); + } + } +} From 3c36def735cb56a5ffea996db646aca2c1b17b2a Mon Sep 17 00:00:00 2001 From: Sympatron GmbH <35803463+Sympatron@users.noreply.github.com> Date: Wed, 2 Oct 2024 12:13:44 +0200 Subject: [PATCH 4/4] Wrapper for osdp_pd_info_t to handle dropping of it's fields after use automatically --- libosdp/src/cp.rs | 12 +++---- libosdp/src/pd.rs | 7 ++-- libosdp/src/pdinfo.rs | 81 +++++++++++++++++++++++++------------------ 3 files changed, 55 insertions(+), 45 deletions(-) diff --git a/libosdp/src/cp.rs b/libosdp/src/cp.rs index 4e5b2a6..def2197 100644 --- a/libosdp/src/cp.rs +++ b/libosdp/src/cp.rs @@ -55,11 +55,8 @@ where trampoline:: } -fn cp_setup(info: Vec) -> Result<*mut c_void> { - let ctx = unsafe { libosdp_sys::osdp_cp_setup(info.len() as i32, info.as_ptr()) }; - for pd in info.into_iter() { - crate::drop_osdp_pd_info(pd); - } +fn cp_setup(info: Vec) -> Result<*mut c_void> { + let ctx = unsafe { libosdp_sys::osdp_cp_setup(info.len() as i32, info.as_ptr() as *const _) }; if ctx.is_null() { Err(OsdpError::Setup) } else { @@ -77,12 +74,11 @@ unsafe impl Send for ControlPanel {} impl ControlPanel { /// Create a new CP context for the list of PDs described by the [`PdInfo`] vector. - pub fn new(mut pd_info: Vec) -> Result { + pub fn new(pd_info: Vec) -> Result { if pd_info.len() > 126 { return Err(OsdpError::PdInfo("max PD count exceeded")); } - let info: Vec = - pd_info.iter_mut().map(|i| i.as_struct()).collect(); + let info: Vec = pd_info.into_iter().map(|i| i.into()).collect(); unsafe { libosdp_sys::osdp_set_log_callback(Some(log_handler)) }; Ok(Self { ctx: cp_setup(info)?, diff --git a/libosdp/src/pd.rs b/libosdp/src/pd.rs index a3bec3c..b6985b4 100644 --- a/libosdp/src/pd.rs +++ b/libosdp/src/pd.rs @@ -57,10 +57,9 @@ where trampoline:: } -fn pd_setup(mut info: PdInfo) -> Result<*mut c_void> { - let info_struct = info.as_struct(); - let ctx = unsafe { libosdp_sys::osdp_pd_setup(&info_struct) }; - crate::drop_osdp_pd_info(info_struct); +fn pd_setup(info: PdInfo) -> Result<*mut c_void> { + let info: crate::OsdpPdInfoHandle = info.into(); + let ctx = unsafe { libosdp_sys::osdp_pd_setup(&*info) }; if ctx.is_null() { Err(OsdpError::Setup) } else { diff --git a/libosdp/src/pdinfo.rs b/libosdp/src/pdinfo.rs index cff3449..f8f3f71 100644 --- a/libosdp/src/pdinfo.rs +++ b/libosdp/src/pdinfo.rs @@ -3,6 +3,8 @@ // // SPDX-License-Identifier: Apache-2.0 +use core::ops::Deref; + use alloc::ffi::CString; use crate::{Box, Channel, OsdpError, OsdpFlag, PdCapability, PdId}; @@ -247,16 +249,26 @@ impl PdInfoBuilder { } } -impl PdInfo { - /// Get a C-repr struct for PdInfo that LibOSDP can operate on. - pub fn as_struct(&mut self) -> libosdp_sys::osdp_pd_info_t { - let scbk = if let Some(key) = self.scbk { +#[repr(transparent)] +pub(crate) struct OsdpPdInfoHandle(pub libosdp_sys::osdp_pd_info_t); + +impl Deref for OsdpPdInfoHandle { + type Target = libosdp_sys::osdp_pd_info_t; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl From for OsdpPdInfoHandle { + fn from(info: PdInfo) -> OsdpPdInfoHandle { + let scbk = if let Some(key) = info.scbk { Box::into_raw(Box::new(key)) as *mut _ } else { core::ptr::null_mut::() }; - let cap = if !self.cap.is_empty() { - let mut cap = self.cap.clone(); + let cap = if !info.cap.is_empty() { + let mut cap = info.cap.clone(); cap.reserve(1); cap.push(libosdp_sys::osdp_pd_cap { function_code: -1i8 as u8, @@ -267,39 +279,42 @@ impl PdInfo { } else { core::ptr::null_mut::() }; - libosdp_sys::osdp_pd_info_t { - name: self.name.clone().into_raw(), - baud_rate: self.baud_rate, - address: self.address, - flags: self.flags.bits() as i32, - id: self.id.into(), + OsdpPdInfoHandle(libosdp_sys::osdp_pd_info_t { + name: info.name.clone().into_raw(), + baud_rate: info.baud_rate, + address: info.address, + flags: info.flags.bits() as i32, + id: info.id.into(), cap: cap as *mut _, - channel: self.channel.take().unwrap().into(), + channel: info.channel.unwrap().into(), scbk, - } + }) } } -pub(crate) fn drop_osdp_pd_info(info: libosdp_sys::osdp_pd_info_t) { - unsafe { - // The name is not copied by LibOSDP, so we cannot drop it here - // if !info.name.is_null() { - // drop(CString::from_raw(info.name as *mut _)); - // } - if !info.cap.is_null() { - let mut cap = info.cap as *mut libosdp_sys::osdp_pd_cap; - while (*cap).function_code != -1i8 as u8 { - cap = cap.add(1); +impl Drop for OsdpPdInfoHandle { + fn drop(&mut self) { + unsafe { + let info = self.0; + // LibOSDP versions <= 3.0.6 do not copy the name, so this is only valid for >= 3.0.7 + if !info.name.is_null() { + drop(CString::from_raw(info.name as *mut _)); + } + if !info.cap.is_null() { + let mut cap = info.cap as *mut libosdp_sys::osdp_pd_cap; + while (*cap).function_code != -1i8 as u8 { + cap = cap.add(1); + } + let len = (cap.offset_from(info.cap) + 1) as usize; + drop(Vec::from_raw_parts( + info.cap as *mut libosdp_sys::osdp_pd_cap, + len, + len, + )); + } + if !info.scbk.is_null() { + drop(Box::from_raw(info.scbk as *mut [u8; 16])); } - let len = (cap.offset_from(info.cap) + 1) as usize; - drop(Vec::from_raw_parts( - info.cap as *mut libosdp_sys::osdp_pd_cap, - len, - len, - )); - } - if !info.scbk.is_null() { - drop(Box::from_raw(info.scbk as *mut [u8; 16])); } } }