From a31428e34732db3b5f4b4722a3f06a0f8d07d1e3 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 10:37:58 +0200 Subject: [PATCH 1/9] 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/9] 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/9] 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/9] 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)) } From af9bcdc7c8fa9bd6e2461294d9a9a1787e81886c Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 14:47:56 +0200 Subject: [PATCH 5/9] Add safe wrapper for uci_add_list Add common helper methods for working with the unsafe uci C-ffi. --- rust-uci/src/lib.rs | 83 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 79 insertions(+), 4 deletions(-) diff --git a/rust-uci/src/lib.rs b/rust-uci/src/lib.rs index 023ee61..6c09c2b 100644 --- a/rust-uci/src/lib.rs +++ b/rust-uci/src/lib.rs @@ -54,10 +54,10 @@ pub mod error; use core::ptr; use libuci_sys::{ - uci_alloc_context, uci_commit, uci_context, uci_delete, uci_element, uci_foreach_element, - uci_free_context, uci_get_errorstr, uci_lookup_ptr, uci_option_type_UCI_TYPE_STRING, - uci_package, uci_ptr, uci_ptr_UCI_LOOKUP_COMPLETE, uci_revert, uci_save, uci_set, - uci_set_confdir, uci_set_savedir, uci_to_section, uci_type_UCI_TYPE_OPTION, + uci_add_list, uci_alloc_context, uci_commit, uci_context, uci_delete, uci_element, + uci_foreach_element, uci_free_context, uci_get_errorstr, uci_lookup_ptr, + uci_option_type_UCI_TYPE_STRING, uci_package, uci_ptr, uci_ptr_UCI_LOOKUP_COMPLETE, uci_revert, + uci_save, uci_set, uci_set_confdir, uci_set_savedir, uci_to_section, uci_type_UCI_TYPE_OPTION, uci_type_UCI_TYPE_SECTION, uci_unload, }; use log::debug; @@ -149,6 +149,19 @@ struct UciPtr { _key: CString, } +impl UciPtr { + /// Assert that self.ptr.value is set (non-null), returning self if the value is set. + fn assert_value_set(self) -> Result { + if self.ptr.value.is_null() { + return Err(Error::Message(format!( + "uci_ptr.value is null: {}", + self._key.to_string_lossy(), + ))); + } + Ok(self) + } +} + impl Deref for UciPtr { type Target = uci_ptr; @@ -404,6 +417,68 @@ impl Uci { }) } + /// Append a string to an element list in UCI. identifier and value will be concatenated to the + /// following assignment: `{identifier}={val}`. + /// + /// If the given option already contains a string value, it will be converted to an 1-element-list before appending the next element. + /// + /// If the addition failed an `Err` is returned. + pub fn add_list(&mut self, identifier: &str, val: &str) -> Result<()> { + if val.contains('\'') { + return Err(Error::Message(format!( + "Values may not contain quotes: {}={}", + identifier, val + ))); + } + libuci_locked!(self, { + let assignment = format!("{}={}", identifier, val); + let mut ptr = self.get_ptr(identifier)?.assert_value_set()?; + // Safety: + // * self.ctx points to a valid UCI context. + // * ptr is valid because self.get_ptr returned Ok(ptr). + let result = unsafe { uci_add_list(self.ctx, ptr.deref_mut()) }; + if result != UCI_OK { + return Err(Error::Message(format!( + "Could not append string to element list: {}, {}, {}", + assignment, + result, + self.get_last_error() + .unwrap_or_else(|_| String::from("Unknown")) + ))); + } + self.save(&ptr)?; + }); + Ok(()) + } + + /// Save change delta for the specified package referenced by `ptr`. See [Self::get_ptr]. + /// + /// UCI will keep the delta changes in a temporary location until `uci_commit()` or `uci_revert()` is called. + /// The change delta may be applied (and flushed) by calling [Self::commit]. + fn save(&mut self, ptr: &UciPtr) -> Result<()> { + let identifier = ptr._key.to_string_lossy().to_string(); + libuci_locked!(self, { + // Safety: + // * self.ctx points to a valid UCI context. + // * ptr was constructed using Self::get_ptr, which guarantees that the pointer is + // valid. + let result = unsafe { uci_save(self.ctx, ptr.p) }; + match result { + UCI_OK => Ok(()), + UCI_ERR_NOTFOUND => Err(Error::EntryNotFound { + entry_identifier: identifier, + }), + _ => Err(Error::Message(format!( + "Could not save uci package: {}, {}, {}", + identifier, + result, + self.get_last_error() + .unwrap_or_else(|_| String::from("Unknown")) + ))), + } + }) + } + /// Commit all changes to the specified package /// writing the temporary delta to the config file pub fn commit(&mut self, package: &str) -> Result<()> { From 3bc966140a50110bca357c0b374cce2d48bf71b7 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 15:03:17 +0200 Subject: [PATCH 6/9] Use safe `save` in `set` wrapper --- rust-uci/src/lib.rs | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/rust-uci/src/lib.rs b/rust-uci/src/lib.rs index 6c09c2b..cd1753f 100644 --- a/rust-uci/src/lib.rs +++ b/rust-uci/src/lib.rs @@ -383,7 +383,8 @@ impl Uci { ))); } libuci_locked!(self, { - let mut ptr = self.get_ptr(format!("{}={}", identifier, val).as_ref())?; + let assignment = format!("{}={}", identifier, val); + let mut ptr = self.get_ptr(&assignment)?; if ptr.value.is_null() { return Err(Error::Message(format!( "parsed value is null: {}={}", @@ -401,19 +402,7 @@ impl Uci { .unwrap_or_else(|_| String::from("Unknown")) ))); } - let result = unsafe { uci_save(self.ctx, ptr.p) }; - if result == UCI_OK { - Ok(()) - } else { - Err(Error::Message(format!( - "Could not save uci key: {}={}, {}, {}", - identifier, - val, - result, - self.get_last_error() - .unwrap_or_else(|_| String::from("Unknown")) - ))) - } + self.save(&ptr) }) } From 303636b77b16c27233d6c4636321c613e6a6802a Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 15:10:37 +0200 Subject: [PATCH 7/9] Use safe `save` in `delete` wrapper --- rust-uci/src/lib.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/rust-uci/src/lib.rs b/rust-uci/src/lib.rs index cd1753f..0c8ec10 100644 --- a/rust-uci/src/lib.rs +++ b/rust-uci/src/lib.rs @@ -319,21 +319,9 @@ impl Uci { .unwrap_or_else(|_| String::from("Unknown")) ))); } - let result = unsafe { uci_save(self.ctx, ptr.p) }; - match result { - UCI_OK => Ok(()), - UCI_ERR_NOTFOUND => Err(Error::EntryNotFound { - entry_identifier: identifier.to_string(), - }), - _ => Err(Error::Message(format!( - "Could not save uci key: {}, {}, {}", - identifier, - result, - self.get_last_error() - .unwrap_or_else(|_| String::from("Unknown")) - ))), - } - }) + self.save(&ptr)?; + }); + Ok(()) } /// Revert changes to an option, section or package From a793d29ef902ea6c2ee17eb7d28fccce314fba61 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 15:12:59 +0200 Subject: [PATCH 8/9] Use safe `save` in `revert` wrapper --- rust-uci/src/lib.rs | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/rust-uci/src/lib.rs b/rust-uci/src/lib.rs index 0c8ec10..76eb4cd 100644 --- a/rust-uci/src/lib.rs +++ b/rust-uci/src/lib.rs @@ -342,18 +342,7 @@ impl Uci { .unwrap_or_else(|_| String::from("Unknown")) ))); } - let result = unsafe { uci_save(self.ctx, ptr.p) }; - if result == UCI_OK { - Ok(()) - } else { - Err(Error::Message(format!( - "Could not save uci key: {}, {}, {}", - identifier, - result, - self.get_last_error() - .unwrap_or_else(|_| String::from("Unknown")) - ))) - } + self.save(&ptr) }) } From 3e96e8d83b98f4ca471f565f13064ea8b9d8007d Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Sat, 25 Apr 2026 15:13:24 +0200 Subject: [PATCH 9/9] Fix potential TOCTOU race when constructing a pointer and calling delete Hold onto the global mutex for the entire duration of Uci::delete to prevent a potential race between calling get_ptr and uci_delete. --- 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 76eb4cd..f6be1af 100644 --- a/rust-uci/src/lib.rs +++ b/rust-uci/src/lib.rs @@ -307,8 +307,8 @@ impl Uci { /// If the deletion failed an `Err` is returned. /// If the entry does not exist before deletion, an `Err` with [`Error::EntryNotFound`] is returned. pub fn delete(&mut self, identifier: &str) -> Result<()> { - let mut ptr = self.get_ptr(identifier)?; libuci_locked!(self, { + let mut ptr = self.get_ptr(identifier)?; let result = unsafe { uci_delete(self.ctx, ptr.deref_mut()) }; if result != UCI_OK { return Err(Error::Message(format!(