From a31428e34732db3b5f4b4722a3f06a0f8d07d1e3 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 10:37:58 +0200 Subject: [PATCH 1/4] Do conversion between CString <-> raw pointer closer to FFI boundary --- rust-uci/src/lib.rs | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/rust-uci/src/lib.rs b/rust-uci/src/lib.rs index b5a0f1f..33dbc67 100644 --- a/rust-uci/src/lib.rs +++ b/rust-uci/src/lib.rs @@ -141,27 +141,23 @@ unsafe impl Sync for Uci {} /// - [`uci_context`] can be freed safely from another thread. unsafe impl Send for Uci {} -/// Contains the native `uci_ptr` and it's raw `CString` key -/// this is done so the raw `CString` stays alive until the `uci_ptr` is dropped -struct UciPtr(uci_ptr, *mut std::os::raw::c_char); +/// Contains the native `uci_ptr` and it's associated key. +struct UciPtr { + ptr: uci_ptr, + _key: CString, +} impl Deref for UciPtr { type Target = uci_ptr; fn deref(&self) -> &Self::Target { - &self.0 + &self.ptr } } impl DerefMut for UciPtr { fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 - } -} - -impl Drop for UciPtr { - fn drop(&mut self) { - drop(unsafe { CString::from_raw(self.1) }); + &mut self.ptr } } @@ -298,7 +294,7 @@ impl Uci { pub fn delete(&mut self, identifier: &str) -> Result<()> { let mut ptr = self.get_ptr(identifier)?; libuci_locked!(self, { - let result = unsafe { uci_delete(self.ctx, &mut ptr.0) }; + let result = unsafe { uci_delete(self.ctx, ptr.deref_mut()) }; if result != UCI_OK { return Err(Error::Message(format!( "Could not delete uci key: {}, {}, {}", @@ -333,7 +329,7 @@ impl Uci { pub fn revert(&mut self, identifier: &str) -> Result<()> { libuci_locked!(self, { let mut ptr = self.get_ptr(identifier)?; - let result = unsafe { uci_revert(self.ctx, &mut ptr.0) }; + let result = unsafe { uci_revert(self.ctx, ptr.deref_mut()) }; if result != UCI_OK { return Err(Error::Message(format!( "Could not revert uci key: {}, {}, {}", @@ -379,7 +375,7 @@ impl Uci { identifier, val ))); } - let result = unsafe { uci_set(self.ctx, &mut ptr.0) }; + let result = unsafe { uci_set(self.ctx, ptr.deref_mut()) }; if result != UCI_OK { return Err(Error::Message(format!( "Could not set uci key: {}={}, {}, {}", @@ -515,11 +511,14 @@ impl Uci { option: ptr::null(), value: ptr::null(), }; - let raw = libuci_locked!(self, { + let key = libuci_locked!(self, { let raw = CString::new(identifier)?.into_raw(); let result = unsafe { uci_lookup_ptr(self.ctx, &mut ptr, raw, true) }; match result { - UCI_OK => (), + // Safety: raw was created from CString::into_raw. + // raw is not aliased / referenced from anywhere else for the lifetime of the + // reconstructed CString. + UCI_OK => unsafe { CString::from_raw(raw) }, UCI_ERR_NOTFOUND => { return Err(Error::EntryNotFound { entry_identifier: identifier.to_string(), @@ -535,11 +534,10 @@ impl Uci { ))); } } - raw }); debug!("{:?}", ptr); if !ptr.last.is_null() { - Ok(UciPtr(ptr, raw)) + Ok(UciPtr { ptr, _key: key }) } else { Err(Error::Message(format!( "Cannot access null value: {}", From beda50ecee7cd7d0918507f6b7689ee524e8b801 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 11:11:51 +0200 Subject: [PATCH 2/4] Prefer CString::as_ptr over CString::as_bytes_with_nul + pointer cast --- rust-uci/src/lib.rs | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/rust-uci/src/lib.rs b/rust-uci/src/lib.rs index 33dbc67..78bc1ed 100644 --- a/rust-uci/src/lib.rs +++ b/rust-uci/src/lib.rs @@ -201,14 +201,12 @@ impl Uci { /// Sets the config directory of UCI, this is `/etc/config` by default. pub fn set_config_dir(&mut self, config_dir: &str) -> Result<()> { libuci_locked!(self, { - let result = unsafe { - let raw = CString::new(config_dir)?; - uci_set_confdir( - self.ctx, - raw.as_bytes_with_nul() - .as_ptr() - .cast::(), - ) + let result = { + let config_dir = CString::new(config_dir)?; + // Safety: + // * self.ctx points to a valid UCI context. + // * save_dir is a valid, null-terminated C-string. + unsafe { uci_set_confdir(self.ctx, config_dir.as_ptr()) } }; if result == UCI_OK { debug!("Set config dir to: {}", config_dir); @@ -243,15 +241,13 @@ impl Uci { /// Sets the save directory of UCI, this is `/tmp/.uci` by default. pub fn set_save_dir(&mut self, save_dir: &str) -> Result<()> { - let raw = CString::new(save_dir)?; libuci_locked!(self, { - let result = unsafe { - uci_set_savedir( - self.ctx, - raw.as_bytes_with_nul() - .as_ptr() - .cast::(), - ) + let result = { + let save_dir = CString::new(save_dir)?; + // Safety: + // * self.ctx points to a valid UCI context. + // * save_dir is a valid, null-terminated C-string. + unsafe { uci_set_savedir(self.ctx, save_dir.as_ptr()) } }; if result == UCI_OK { debug!("Set save dir to: {}", save_dir); From 88527f7b6373a87263854c0795ca0074cf5c1ad2 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 11:15:54 +0200 Subject: [PATCH 3/4] Fix error message assuming call site --- rust-uci/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-uci/src/lib.rs b/rust-uci/src/lib.rs index 78bc1ed..698bb89 100644 --- a/rust-uci/src/lib.rs +++ b/rust-uci/src/lib.rs @@ -175,7 +175,7 @@ impl DerefMut for UciPtr { /// and returns an `Err` otherwise. unsafe fn char_ptr_to_str<'a>(ptr: *mut c_char) -> Result<&'a str> { if ptr.is_null() { - return Err(Error::Message("config dir was nullptr".into())); + return Err(Error::Message("char* was nullptr".into())); } // Safety: the ptr is not null. // The safety assumption is that ptr points to From 753a3173a04ca1e76f8c1750f15896a4dfe8a64c Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 12:33:56 +0200 Subject: [PATCH 4/4] Make set_config_dir + set_save_dir accept AsRef instead of &str It was assumed that the &str argument containing the directory was indeed a valid directory, which is likely but not enforced by the API. The corresponding libuci functions are sparsely documented, but the also assume a valid directory name. --- rust-uci/src/lib.rs | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/rust-uci/src/lib.rs b/rust-uci/src/lib.rs index 698bb89..023ee61 100644 --- a/rust-uci/src/lib.rs +++ b/rust-uci/src/lib.rs @@ -66,6 +66,8 @@ use std::sync::Mutex; use std::{ ffi::{CStr, CString}, ops::{Deref, DerefMut}, + os::unix::ffi::OsStrExt, + path::Path, }; use crate::error::{Error, Result}; @@ -199,22 +201,24 @@ impl Uci { } /// Sets the config directory of UCI, this is `/etc/config` by default. - pub fn set_config_dir(&mut self, config_dir: &str) -> Result<()> { + pub fn set_config_dir(&mut self, config_dir: impl AsRef) -> Result<()> { libuci_locked!(self, { let result = { - let config_dir = CString::new(config_dir)?; + let config_dir = config_dir.as_ref().as_os_str(); + let config_dir = CString::new(config_dir.as_bytes())?; // Safety: // * self.ctx points to a valid UCI context. - // * save_dir is a valid, null-terminated C-string. + // * config_dir is a valid, null-terminated C-string. + // * config_dir is a valid directory path. unsafe { uci_set_confdir(self.ctx, config_dir.as_ptr()) } }; if result == UCI_OK { - debug!("Set config dir to: {}", config_dir); + debug!("Set config dir to: {}", config_dir.as_ref().display()); Ok(()) } else { Err(Error::Message(format!( "Cannot set config dir: {}, {}", - config_dir, + config_dir.as_ref().display(), self.get_last_error() .unwrap_or_else(|_| String::from("Unknown")) ))) @@ -240,22 +244,24 @@ impl Uci { } /// Sets the save directory of UCI, this is `/tmp/.uci` by default. - pub fn set_save_dir(&mut self, save_dir: &str) -> Result<()> { + pub fn set_save_dir(&mut self, save_dir: impl AsRef) -> Result<()> { libuci_locked!(self, { let result = { - let save_dir = CString::new(save_dir)?; + let save_dir = save_dir.as_ref().as_os_str(); + let save_dir = CString::new(save_dir.as_bytes())?; // Safety: // * self.ctx points to a valid UCI context. // * save_dir is a valid, null-terminated C-string. + // * save_dir is a valid directory path. unsafe { uci_set_savedir(self.ctx, save_dir.as_ptr()) } }; if result == UCI_OK { - debug!("Set save dir to: {}", save_dir); + debug!("Set save dir to: {}", save_dir.as_ref().display()); Ok(()) } else { Err(Error::Message(format!( "Cannot set save dir: {}, {}", - save_dir, + save_dir.as_ref().display(), self.get_last_error() .unwrap_or_else(|_| String::from("Unknown")) ))) @@ -623,8 +629,8 @@ mod tests { std::fs::create_dir_all(&config_dir).unwrap(); std::fs::create_dir_all(&save_dir).unwrap(); - uci.set_config_dir(config_dir.as_os_str().to_str().unwrap())?; - uci.set_save_dir(save_dir.as_os_str().to_str().unwrap())?; + uci.set_config_dir(&config_dir)?; + uci.set_save_dir(&save_dir)?; Ok((uci, tmp)) }