diff --git a/CHANGELOG.md b/CHANGELOG.md index e53f1f65..844d0704 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,12 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - Add back `Server::create_event()` with reordered and additional arguments. This new method replaces both `Server::create_event()` and `Server::trigger_event()` that have been removed. +### Fixed + +- Fix a memory leak triggered when using `with_*` builder methods on data types for array members + (e.g. `ua::RelativePath::with_elements()` would leak the elements that were previously owned by + the instance). + ## [0.12.0] - 2026-07-14 ### Changed diff --git a/Cargo.toml b/Cargo.toml index 67972cef..847f13a2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,6 +69,7 @@ rust_2024_compatibility = { level = "warn", priority = -1 } tail_expr_drop_order = { level = "allow" } trivial_casts = "warn" trivial_numeric_casts = "warn" +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(asan)'] } unreachable_pub = "warn" # Writing unsafe code is a necessity for FFI wrappers. unsafe_code = "allow" diff --git a/src/lib.rs b/src/lib.rs index fb88ee90..2d45e270 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -265,6 +265,7 @@ use self::{ client::ClientContext, data_type::{bitmask_ops, data_type, enum_variants}, monitored_item::create_monitored_items_callback, + ua::UaIterExt, value::{ArrayValue, NonScalarValue}, }; #[cfg(feature = "mbedtls")] diff --git a/src/server.rs b/src/server.rs index d204274f..400644b1 100644 --- a/src/server.rs +++ b/src/server.rs @@ -30,7 +30,7 @@ use parking_lot::{Condvar, Mutex, MutexGuard}; use crate::{ Attribute, Attributes, BrowseResult, DEFAULT_PORT_NUMBER, DataType, DataValue, Error, Result, - ua, + UaIterExt as _, ua, }; pub(crate) use self::node_context::NodeContext; @@ -184,10 +184,10 @@ impl ServerBuilder { #[must_use] pub fn server_urls(mut self, server_urls: &[&str]) -> Self { let config = self.config_mut(); - let server_urls = server_urls + server_urls .iter() - .map(|server_url| ua::String::new(server_url).unwrap()); - ua::Array::from_iter(server_urls) + .map(|server_url| ua::String::new(server_url).unwrap()) + .collect_ua_array() .move_into_raw(&mut config.serverUrlsSize, &mut config.serverUrls); self } diff --git a/src/ua.rs b/src/ua.rs index 4449772c..8ac526df 100644 --- a/src/ua.rs +++ b/src/ua.rs @@ -73,4 +73,6 @@ pub use self::{ unit_id::UnitId, user_identity_token::UserIdentityToken, }; -pub(crate) use self::{client_config::ClientConfig, logger::Logger, server_config::ServerConfig}; +pub(crate) use self::{ + array::UaIterExt, client_config::ClientConfig, logger::Logger, server_config::ServerConfig, +}; diff --git a/src/ua/array.rs b/src/ua/array.rs index dcd8edff..e0e3bc18 100644 --- a/src/ua/array.rs +++ b/src/ua/array.rs @@ -83,22 +83,6 @@ impl Array { Self(State::NonEmpty { ptr: array, size }) } - /// Creates new array from existing elements. - /// - /// This takes ownership of the elements from the given iterator. - /// - /// # Panics - /// - /// Enough memory must be available to allocate array. - pub(crate) fn from_iter>(iter: I) -> Self { - // This creates a temporary copy by first collecting all elements into a `Vec` and then once - // more by copying elements from the `Vec` into the new array from `UA_Array_new()`. - // - // TODO: Avoid temporary copy. How to deal with unknown size of iterator when initializing a - // new array to hold its elements? - Self::from_slice(&iter.collect::>()) - } - /// Creates new array from existing elements. /// /// This copies over the elements from the given slice. The array will own the copies, and clean @@ -401,8 +385,11 @@ impl Array { /// /// After this, it is the responsibility of `dst` to eventually clean up the data. pub(crate) fn move_into_raw(self, dst_size: &mut usize, dst: &mut *mut T::Inner) { - // Make sure to clean up any previous value in target. - let _unused = Self::from_raw_parts(*dst_size, *dst); + // Make sure to clean up any previous value in target. This must move out of (not copy) the + // existing array, otherwise its allocation would be leaked. + // + // SAFETY: `dst` is the sole owner of the previous array, which we take over and drop here. + let _unused = unsafe { Self::move_from_raw_parts(dst_size, dst) }; let (size, ptr) = self.into_raw_parts(); *dst_size = size; @@ -456,6 +443,37 @@ impl Array { } } +/// Internal-only helper for collection of iterators into [`Array`]s. +// +// Might be good to make this public as an impl of [`FromIterator`] once performance is optimized. +pub(crate) trait UaIterExt +where + T: DataType, +{ + /// Takes ownership of the elements from the given iterator. + /// + /// # Panics + /// + /// Enough memory must be available to allocate array. + fn collect_ua_array(self) -> Array; +} + +impl UaIterExt for I +where + I: Iterator, + T: DataType, +{ + fn collect_ua_array(self) -> Array { + // This creates a temporary copy by first collecting all elements into a `Vec` and then once + // more by copying elements from the `Vec` into the new array from `UA_Array_new()`. + // + // TODO: Avoid temporary copy. How to deal with unknown size of iterator when initializing a + // new array to hold its elements? (possible optimization opportunity with a specialized + // implementation for `ExactSizeIterator`) + Array::from_slice(&self.collect::>()) + } +} + // SAFETY: Arrays in `open62541` can be sent across thread boundaries: they may be dropped in other // threads as long as their items are `Send`. unsafe impl Send for Array {} @@ -679,4 +697,16 @@ mod tests { .join() .expect("join thread"); } + + // Simple probe to verify that the behavior of `Array::move_into_raw()` doesn't leak. + // Only valuable when run with `-Zsanitizer=address`, so it's being gated behind an explicit cfg. + #[cfg(asan)] + #[test] + fn move_into_raw_leak_probe() { + let array_src = ua::Array::::new(42); + let (mut size, mut ptr) = ua::Array::::new(24).into_raw_parts(); + // This would fail under ASAN with previous impl, now succeeds. + array_src.move_into_raw(&mut size, &mut ptr); + unsafe { ua::Array::::move_from_raw_parts(&mut size, &mut ptr) }.unwrap(); + } } diff --git a/src/ua/data_types/application_description.rs b/src/ua/data_types/application_description.rs index 10a8e316..27a28787 100644 --- a/src/ua/data_types/application_description.rs +++ b/src/ua/data_types/application_description.rs @@ -1,4 +1,4 @@ -use crate::{DataType as _, ua}; +use crate::{DataType as _, UaIterExt as _, ua}; crate::data_type!(ApplicationDescription); @@ -86,10 +86,10 @@ impl ApplicationDescription { /// The strings must not contain any NUL bytes. #[must_use] pub fn with_discovery_urls(mut self, discovery_urls: &[&str]) -> Self { - let discovery_urls = discovery_urls + discovery_urls .iter() - .map(|discovery_url| ua::String::new(discovery_url).unwrap()); - ua::Array::from_iter(discovery_urls) + .map(|discovery_url| ua::String::new(discovery_url).unwrap()) + .collect_ua_array() .move_into_raw(&mut self.0.discoveryUrlsSize, &mut self.0.discoveryUrls); self } diff --git a/src/ua/data_types/browse_next_request.rs b/src/ua/data_types/browse_next_request.rs index c2e89457..bfe7a09c 100644 --- a/src/ua/data_types/browse_next_request.rs +++ b/src/ua/data_types/browse_next_request.rs @@ -1,4 +1,4 @@ -use crate::{DataType as _, ServiceRequest, ua}; +use crate::{DataType as _, ServiceRequest, UaIterExt as _, ua}; crate::data_type!(BrowseNextRequest); @@ -8,15 +8,14 @@ impl BrowseNextRequest { mut self, continuation_points: &[ua::ContinuationPoint], ) -> Self { - let array = ua::Array::from_iter( - continuation_points - .iter() - .map(ua::ContinuationPoint::to_byte_string), - ); - array.move_into_raw( - &mut self.0.continuationPointsSize, - &mut self.0.continuationPoints, - ); + continuation_points + .iter() + .map(ua::ContinuationPoint::to_byte_string) + .collect_ua_array() + .move_into_raw( + &mut self.0.continuationPointsSize, + &mut self.0.continuationPoints, + ); self } diff --git a/src/ua/data_types/content_filter_element.rs b/src/ua/data_types/content_filter_element.rs index bae4d419..dbf5db28 100644 --- a/src/ua/data_types/content_filter_element.rs +++ b/src/ua/data_types/content_filter_element.rs @@ -1,4 +1,4 @@ -use crate::{DataType, FilterOperand, ua}; +use crate::{DataType, FilterOperand, UaIterExt as _, ua}; crate::data_type!(ContentFilterElement); @@ -11,12 +11,11 @@ impl ContentFilterElement { #[must_use] pub fn with_filter_operands(mut self, filter_operands: &[impl FilterOperand]) -> Self { - let array = ua::Array::from_iter( - filter_operands - .iter() - .map(FilterOperand::to_extension_object), - ); - array.move_into_raw(&mut self.0.filterOperandsSize, &mut self.0.filterOperands); + filter_operands + .iter() + .map(FilterOperand::to_extension_object) + .collect_ua_array() + .move_into_raw(&mut self.0.filterOperandsSize, &mut self.0.filterOperands); self } } diff --git a/src/ua/data_types/delete_monitored_items_request.rs b/src/ua/data_types/delete_monitored_items_request.rs index e94c74fc..56988417 100644 --- a/src/ua/data_types/delete_monitored_items_request.rs +++ b/src/ua/data_types/delete_monitored_items_request.rs @@ -1,4 +1,4 @@ -use crate::ua; +use crate::{UaIterExt as _, ua}; crate::data_type!(DeleteMonitoredItemsRequest); @@ -11,15 +11,14 @@ impl DeleteMonitoredItemsRequest { #[must_use] pub fn with_monitored_item_ids(mut self, monitored_item_ids: &[ua::MonitoredItemId]) -> Self { - let array = ua::Array::from_iter( - monitored_item_ids - .iter() - .map(|monitored_item_id| monitored_item_id.as_id().to_uint32()), - ); - array.move_into_raw( - &mut self.0.monitoredItemIdsSize, - &mut self.0.monitoredItemIds, - ); + monitored_item_ids + .iter() + .map(|monitored_item_id| monitored_item_id.as_id().to_uint32()) + .collect_ua_array() + .move_into_raw( + &mut self.0.monitoredItemIdsSize, + &mut self.0.monitoredItemIds, + ); self } } diff --git a/src/ua/data_types/delete_subscriptions_request.rs b/src/ua/data_types/delete_subscriptions_request.rs index 5fa46610..1a7466b6 100644 --- a/src/ua/data_types/delete_subscriptions_request.rs +++ b/src/ua/data_types/delete_subscriptions_request.rs @@ -1,16 +1,15 @@ -use crate::ua; +use crate::{UaIterExt as _, ua}; crate::data_type!(DeleteSubscriptionsRequest); impl DeleteSubscriptionsRequest { #[must_use] pub fn with_subscription_ids(mut self, subscription_ids: &[ua::SubscriptionId]) -> Self { - let array = ua::Array::from_iter( - subscription_ids - .iter() - .map(|subscription_id| subscription_id.as_id().to_uint32()), - ); - array.move_into_raw(&mut self.0.subscriptionIdsSize, &mut self.0.subscriptionIds); + subscription_ids + .iter() + .map(|subscription_id| subscription_id.as_id().to_uint32()) + .collect_ua_array() + .move_into_raw(&mut self.0.subscriptionIdsSize, &mut self.0.subscriptionIds); self } } diff --git a/src/ua/data_types/node_attributes/variable_attributes.rs b/src/ua/data_types/node_attributes/variable_attributes.rs index 392ae1f1..b4abeac9 100644 --- a/src/ua/data_types/node_attributes/variable_attributes.rs +++ b/src/ua/data_types/node_attributes/variable_attributes.rs @@ -1,4 +1,4 @@ -use crate::{DataType as _, ua}; +use crate::{DataType as _, UaIterExt as _, ua}; impl super::VariableAttributes { #[must_use] @@ -17,9 +17,11 @@ impl super::VariableAttributes { #[must_use] pub fn with_array_dimensions(mut self, array_dimensions: &[u32]) -> Self { - let array_dimensions = - ua::Array::from_iter(array_dimensions.iter().copied().map(ua::UInt32::new)); array_dimensions + .iter() + .copied() + .map(ua::UInt32::new) + .collect_ua_array() .move_into_raw(&mut self.0.arrayDimensionsSize, &mut self.0.arrayDimensions); self.0.specifiedAttributes |= ua::SpecifiedAttributes::ARRAYDIMENSIONS.as_u32(); self