From 5c3833b531c11505e96fa930fc82ce6945907165 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Fri, 1 Aug 2025 16:22:02 +0200 Subject: [PATCH 01/10] Switch to `master` branch of open62541 --- Cargo.lock | 33 +++++- Cargo.toml | 4 +- .../create_monitored_items.rs | 3 +- .../delete_monitored_items.rs | 3 +- src/async_subscription.rs | 6 +- src/client.rs | 5 +- src/server.rs | 109 ++++++++++++------ src/ua/certificate_verification.rs | 54 +++++---- src/ua/client.rs | 9 +- 9 files changed, 148 insertions(+), 78 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e038fc49..276970f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -270,6 +270,27 @@ dependencies = [ "serde", ] +[[package]] +name = "derive_more" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "093242cf7570c207c83073cf82f79706fe7b8317e98620a47d5be7c3d8497678" +dependencies = [ + "derive_more-impl", +] + +[[package]] +name = "derive_more-impl" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bda628edc44c4bb645fbe0f758797143e4e07926f7ebf4e9bdfbd3d2ce621df3" +dependencies = [ + "proc-macro2", + "quote", + "syn", + "unicode-xid", +] + [[package]] name = "either" version = "1.9.0" @@ -606,6 +627,7 @@ name = "open62541" version = "0.9.3" dependencies = [ "anyhow", + "derive_more", "env_logger", "futures", "futures-channel", @@ -628,9 +650,8 @@ dependencies = [ [[package]] name = "open62541-sys" -version = "0.4.17" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c098dee418e964c2515e3c0ba97052503a8a81ac1adb8ea3b259d09238ffc853" +version = "0.4.18" +source = "git+https://github.com/sgoll/open62541-sys.git?branch=open62541-1.5#5e1d073ad4ffc7e9964157e4715f89f5c3717f3f" dependencies = [ "bindgen", "cc", @@ -1031,6 +1052,12 @@ version = "1.0.12" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b" +[[package]] +name = "unicode-xid" +version = "0.2.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ebc1c04c71510c7f702b52b7c350734c9ff1295c464a03335b00bb84fc54f853" + [[package]] name = "untrusted" version = "0.9.0" diff --git a/Cargo.toml b/Cargo.toml index e71d4283..240f8ad4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,10 +17,12 @@ categories = ["network-programming", "embedded", "api-bindings"] include = ["src/", "README.md", "CHANGELOG.md"] [dependencies] +derive_more = { version = "2.0.1", features = ["debug"] } futures-channel = "0.3.30" futures-core = { version = "0.3.30", default-features = false } log = "0.4.20" -open62541-sys = "0.4.17" +# FIXME: Revert before merge. +open62541-sys = { git = "https://github.com/sgoll/open62541-sys.git", branch = "open62541-1.5" } parking_lot = "0.12.4" paste = "1.0.14" serde = { version = "1.0.194", optional = true } diff --git a/src/async_monitored_item/create_monitored_items.rs b/src/async_monitored_item/create_monitored_items.rs index 152fd5b7..fde88610 100644 --- a/src/async_monitored_item/create_monitored_items.rs +++ b/src/async_monitored_item/create_monitored_items.rs @@ -225,11 +225,10 @@ unsafe extern "C" fn callback_c( _client: *mut UA_Client, userdata: *mut c_void, _request_id: UA_UInt32, - response: *mut c_void, + response: *mut UA_CreateMonitoredItemsResponse, ) { log::debug!("MonitoredItems_createDataChanges() completed"); - let response = response.cast::(); // SAFETY: Incoming pointer is valid for access. // PANIC: We expect pointer to be valid when good. let response = unsafe { response.as_ref() }.expect("response should be set"); diff --git a/src/async_monitored_item/delete_monitored_items.rs b/src/async_monitored_item/delete_monitored_items.rs index c2984661..eea26b0b 100644 --- a/src/async_monitored_item/delete_monitored_items.rs +++ b/src/async_monitored_item/delete_monitored_items.rs @@ -34,11 +34,10 @@ unsafe extern "C" fn callback_c( _client: *mut UA_Client, _userdata: *mut c_void, _request_id: UA_UInt32, - response: *mut c_void, + response: *mut UA_DeleteMonitoredItemsResponse, ) { log::debug!("MonitoredItems_delete() completed"); - let response = response.cast::(); // SAFETY: Incoming pointer is valid for access. // PANIC: We expect pointer to be valid when good. let response = unsafe { response.as_ref() }.expect("response should be set"); diff --git a/src/async_subscription.rs b/src/async_subscription.rs index 2a18d71d..3ab3ef2c 100644 --- a/src/async_subscription.rs +++ b/src/async_subscription.rs @@ -230,11 +230,10 @@ async fn create_subscription( _client: *mut UA_Client, userdata: *mut c_void, _request_id: UA_UInt32, - response: *mut c_void, + response: *mut UA_CreateSubscriptionResponse, ) { log::debug!("Subscriptions_create() completed"); - let response = response.cast::(); // SAFETY: Incoming pointer is valid for access. // PANIC: We expect pointer to be valid when good. let response = unsafe { response.as_ref() }.expect("response should be set"); @@ -296,11 +295,10 @@ fn delete_subscriptions(client: &ua::Client, request: &ua::DeleteSubscriptionsRe _client: *mut UA_Client, _userdata: *mut c_void, _request_id: UA_UInt32, - response: *mut c_void, + response: *mut UA_DeleteSubscriptionsResponse, ) { log::debug!("Subscriptions_delete() completed"); - let response = response.cast::(); // SAFETY: Incoming pointer is valid for access. // PANIC: We expect pointer to be valid when good. let response = unsafe { response.as_ref() }.expect("response should be set"); diff --git a/src/client.rs b/src/client.rs index f88d4e8f..c2cc9106 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1,8 +1,7 @@ use std::{ffi::CString, ptr, time::Duration}; use open62541_sys::{ - UA_CertificateVerification_AcceptAll, UA_ClientConfig, UA_Client_connect, - UA_Client_getEndpoints, + UA_CertificateGroup_AcceptAll, UA_ClientConfig, UA_Client_connect, UA_Client_getEndpoints, }; use crate::{ua, DataType as _, Error, Result}; @@ -195,7 +194,7 @@ impl ClientBuilder { pub fn accept_all(mut self) -> Self { let config = self.config_mut(); unsafe { - UA_CertificateVerification_AcceptAll(&raw mut config.certificateVerification); + UA_CertificateGroup_AcceptAll(&raw mut config.certificateVerification); } self } diff --git a/src/server.rs b/src/server.rs index 9b677377..c0d11123 100644 --- a/src/server.rs +++ b/src/server.rs @@ -12,9 +12,11 @@ use std::{ time::Instant, }; +use derive_more::Debug; use open62541_sys::{ - UA_CertificateVerification_AcceptAll, UA_NodeId, UA_Server, UA_ServerConfig, - UA_Server_addDataSourceVariableNode, UA_Server_addMethodNodeEx, UA_Server_addNamespace, + UA_CertificateGroup_AcceptAll, UA_NodeId, UA_Server, UA_ServerConfig, + UA_Server_addCallbackValueSourceVariableNode, UA_Server_addMethodNodeEx, + UA_Server_addNamespace, UA_Server_addNode_begin, UA_Server_addNode_finish, UA_Server_addReference, UA_Server_browse, UA_Server_browseNext, UA_Server_browseRecursive, UA_Server_browseSimplifiedBrowsePath, UA_Server_createEvent, UA_Server_deleteNode, UA_Server_deleteReference, UA_Server_getConfig, UA_Server_getNamespaceByIndex, @@ -22,7 +24,7 @@ use open62541_sys::{ UA_Server_readObjectProperty, UA_Server_runUntilInterrupt, UA_Server_run_iterate, UA_Server_run_shutdown, UA_Server_run_startup, UA_Server_translateBrowsePathToNodeIds, UA_Server_triggerEvent, UA_Server_writeDataValue, UA_Server_writeObjectProperty, - UA_Server_writeValue, __UA_Server_addNode, UA_STATUSCODE_BADNOTFOUND, + UA_Server_writeValue, UA_STATUSCODE_BADNOTFOUND, }; use parking_lot::{Condvar, Mutex, MutexGuard}; @@ -194,8 +196,8 @@ impl ServerBuilder { pub fn accept_all(mut self) -> Self { let config = self.config_mut(); unsafe { - UA_CertificateVerification_AcceptAll(&raw mut config.secureChannelPKI); - UA_CertificateVerification_AcceptAll(&raw mut config.sessionPKI); + UA_CertificateGroup_AcceptAll(&raw mut config.secureChannelPKI); + UA_CertificateGroup_AcceptAll(&raw mut config.sessionPKI); } self } @@ -263,8 +265,8 @@ impl ServerBuilder { let config = self.config_mut(); // PANIC: We never set lifecycle hooks elsewhere in config. - debug_assert!(config.nodeLifecycle.destructor.is_none()); - config.nodeLifecycle.destructor = Some(destructor_c); + debug_assert!(unsafe { &*config.nodeLifecycle }.destructor.is_none()); + unsafe { &mut *config.nodeLifecycle }.destructor = Some(destructor_c); let Self { config, @@ -365,6 +367,7 @@ enum RunnerStateInner { #[derive(Debug)] struct ServerConfigGuard<'a> { + #[debug(skip)] config: &'a UA_ServerConfig, guard: RunnerStateGuard<'a>, } @@ -609,18 +612,20 @@ impl Server { let mut out_new_node_id = ua::NodeId::null(); let status_code = ua::StatusCode::new(unsafe { - __UA_Server_addNode( + UA_Server_addNode_begin( // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`. self.server.as_ptr().cast_mut(), // Passing ownership is trivial with primitive value (`u32`). attributes.node_class().clone().into_raw(), - requested_new_node_id.as_ptr(), - parent_node_id.as_ptr(), - reference_type_id.as_ptr(), - // TODO: Verify that `__UA_Server_addNode()` takes ownership. + // TODO: Verify that `UA_Server_addNode_begin()` does not take ownership. + DataType::to_raw_copy(&requested_new_node_id), + DataType::to_raw_copy(&parent_node_id), + DataType::to_raw_copy(&reference_type_id), + // TODO: Verify that `UA_Server_addNode_begin()` takes ownership. browse_name.clone().into_raw(), - type_definition.as_ptr(), - attributes.as_node_attributes().as_ptr(), + // TODO: Verify that `UA_Server_addNode_begin()` does not take ownership. + DataType::to_raw_copy(&type_definition), + attributes.as_node_attributes().as_ptr().cast(), attributes.attribute_type(), context.map_or(ptr::null_mut(), NodeContext::leak), out_new_node_id.as_mut_ptr(), @@ -628,6 +633,16 @@ impl Server { }); Error::verify_good(&status_code)?; + let status_code = ua::StatusCode::new(unsafe { + UA_Server_addNode_finish( + // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`. + self.server.as_ptr().cast_mut(), + // TODO: Verify that `UA_Server_addNode_finish()` does not take ownership. + DataType::to_raw_copy(&out_new_node_id), + ) + }); + Error::verify_good(&status_code)?; + Ok(out_new_node_id) } @@ -656,18 +671,20 @@ impl Server { let mut out_new_node_id = ua::NodeId::null(); let status_code = ua::StatusCode::new(unsafe { - __UA_Server_addNode( + UA_Server_addNode_begin( // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`. self.server.as_ptr().cast_mut(), // Passing ownership is trivial with primitive value (`u32`). ua::NodeClass::OBJECT.into_raw(), - requested_new_node_id.as_ptr(), - parent_node_id.as_ptr(), - reference_type_id.as_ptr(), - // TODO: Verify that `__UA_Server_addNode()` takes ownership. + // TODO: Verify that `UA_Server_addNode_begin()` does not take ownership. + DataType::to_raw_copy(&requested_new_node_id), + DataType::to_raw_copy(&parent_node_id), + DataType::to_raw_copy(&reference_type_id), + // TODO: Verify that `UA_Server_addNode_begin()` takes ownership. browse_name.into_raw(), - type_definition.as_ptr(), - attributes.as_node_attributes().as_ptr(), + // TODO: Verify that `UA_Server_addNode_begin()` does not take ownership. + DataType::to_raw_copy(&type_definition), + attributes.as_node_attributes().as_ptr().cast(), ua::ObjectAttributes::data_type(), ptr::null_mut(), out_new_node_id.as_mut_ptr(), @@ -675,6 +692,16 @@ impl Server { }); Error::verify_good(&status_code)?; + let status_code = ua::StatusCode::new(unsafe { + UA_Server_addNode_finish( + // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`. + self.server.as_ptr().cast_mut(), + // TODO: Verify that `UA_Server_addNode_finish()` does not take ownership. + DataType::to_raw_copy(&out_new_node_id), + ) + }); + Error::verify_good(&status_code)?; + Ok(out_new_node_id) } @@ -703,18 +730,20 @@ impl Server { let mut out_new_node_id = ua::NodeId::null(); let status_code = ua::StatusCode::new(unsafe { - __UA_Server_addNode( + UA_Server_addNode_begin( // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`. self.server.as_ptr().cast_mut(), // Passing ownership is trivial with primitive value (`u32`). ua::NodeClass::VARIABLE.into_raw(), - requested_new_node_id.as_ptr(), - parent_node_id.as_ptr(), - reference_type_id.as_ptr(), - // TODO: Verify that `__UA_Server_addNode()` takes ownership. + // TODO: Verify that `UA_Server_addNode_begin()` does not take ownership. + DataType::to_raw_copy(&requested_new_node_id), + DataType::to_raw_copy(&parent_node_id), + DataType::to_raw_copy(&reference_type_id), + // TODO: Verify that `UA_Server_addNode_begin()` takes ownership. browse_name.into_raw(), - type_definition.as_ptr(), - attributes.as_node_attributes().as_ptr(), + // TODO: Verify that `UA_Server_addNode_begin()` does not take ownership. + DataType::to_raw_copy(&type_definition), + attributes.as_node_attributes().as_ptr().cast(), ua::VariableAttributes::data_type(), ptr::null_mut(), out_new_node_id.as_mut_ptr(), @@ -722,6 +751,16 @@ impl Server { }); Error::verify_good(&status_code)?; + let status_code = ua::StatusCode::new(unsafe { + UA_Server_addNode_finish( + // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`. + self.server.as_ptr().cast_mut(), + // TODO: Verify that `UA_Server_addNode_finish()` does not take ownership. + DataType::to_raw_copy(&out_new_node_id), + ) + }); + Error::verify_good(&status_code)?; + Ok(out_new_node_id) } @@ -756,20 +795,20 @@ impl Server { // SAFETY: We store `node_context` inside the node to keep `data_source` alive. let (data_source, node_context) = unsafe { data_source::wrap_data_source(data_source) }; let status_code = ua::StatusCode::new(unsafe { - UA_Server_addDataSourceVariableNode( + UA_Server_addCallbackValueSourceVariableNode( // SAFETY: Cast to `mut` pointer, function is marked `UA_THREADSAFE`. self.server.as_ptr().cast_mut(), - // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership. + // TODO: Verify that `UA_Server_addCallbackValueSourceVariableNode()` takes ownership. requested_new_node_id.into_raw(), - // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership. + // TODO: Verify that `UA_Server_addCallbackValueSourceVariableNode()` takes ownership. parent_node_id.into_raw(), - // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership. + // TODO: Verify that `UA_Server_addCallbackValueSourceVariableNode()` takes ownership. reference_type_id.into_raw(), - // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership. + // TODO: Verify that `UA_Server_addCallbackValueSourceVariableNode()` takes ownership. browse_name.into_raw(), - // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership. + // TODO: Verify that `UA_Server_addCallbackValueSourceVariableNode()` takes ownership. type_definition.into_raw(), - // TODO: Verify that `UA_Server_addDataSourceVariableNode()` takes ownership. + // TODO: Verify that `UA_Server_addCallbackValueSourceVariableNode()` takes ownership. attributes.into_raw(), data_source, node_context.leak(), diff --git a/src/ua/certificate_verification.rs b/src/ua/certificate_verification.rs index dd6e6687..76a64d13 100644 --- a/src/ua/certificate_verification.rs +++ b/src/ua/certificate_verification.rs @@ -3,16 +3,16 @@ use std::{ ptr, }; +use derive_more::Debug; use open62541_sys::{ - UA_ByteString, UA_CertificateVerification, UA_CertificateVerification_AcceptAll, UA_StatusCode, - UA_String, + UA_ByteString, UA_CertificateGroup, UA_CertificateGroup_AcceptAll, UA_StatusCode, UA_String, }; use crate::{ua, CustomCertificateVerification, DataType, Userdata}; -/// Wrapper for [`UA_CertificateVerification`] from [`open62541_sys`]. +/// Wrapper for [`UA_CertificateGroup`] from [`open62541_sys`]. #[derive(Debug)] -pub struct CertificateVerification(UA_CertificateVerification); +pub struct CertificateVerification(#[debug(skip)] UA_CertificateGroup); impl CertificateVerification { /// Creates certificate verification with all checks disabled. @@ -24,17 +24,18 @@ impl CertificateVerification { let mut certificate_verification = Self::init(); // SAFETY: Certificate verification is null, but that is valid. unsafe { - UA_CertificateVerification_AcceptAll(certificate_verification.as_mut_ptr()); + UA_CertificateGroup_AcceptAll(certificate_verification.as_mut_ptr()); } certificate_verification } /// Creates certificate verification with custom callbacks. - pub fn custom(certificate_verification: impl CustomCertificateVerification + 'static) -> Self { + #[expect(dead_code, reason = "work in progress")] + pub fn custom(_certificate_verification: impl CustomCertificateVerification + 'static) -> Self { type Ud = Userdata>; unsafe extern "C" fn verify_certificate_c( - cv: *const UA_CertificateVerification, + cv: *const UA_CertificateGroup, certificate: *const UA_ByteString, ) -> UA_StatusCode { // SAFETY: Reference is used only for the remainder of this function. @@ -49,7 +50,7 @@ impl CertificateVerification { } unsafe extern "C" fn verify_application_uri_c( - cv: *const UA_CertificateVerification, + cv: *const UA_CertificateGroup, certificate: *const UA_ByteString, application_uri: *const UA_String, ) -> UA_StatusCode { @@ -70,23 +71,26 @@ impl CertificateVerification { status_code.into_raw() } - unsafe extern "C" fn clear_c(cv: *mut UA_CertificateVerification) { + unsafe extern "C" fn clear_c(cv: *mut UA_CertificateGroup) { // Reclaim ownership of certificate verification and drop it. // SAFETY: We use the user data only when it is still alive. let _unused = unsafe { Ud::consume((*cv).context) }; } - let inner = UA_CertificateVerification { - context: Ud::prepare(Box::new(certificate_verification)), - verifyCertificate: Some(verify_certificate_c), - verifyApplicationURI: Some(verify_application_uri_c), - getExpirationDate: None, - getSubjectName: None, - clear: Some(clear_c), - logging: ptr::null_mut(), - }; + // FIXME: Implement `UA_CertificateGroup`. + unimplemented!() - unsafe { Self::from_raw(inner) } + // let inner = UA_CertificateGroup { + // context: Ud::prepare(Box::new(certificate_verification)), + // verifyCertificate: Some(verify_certificate_c), + // verifyApplicationURI: Some(verify_application_uri_c), + // getExpirationDate: None, + // getSubjectName: None, + // clear: Some(clear_c), + // logging: ptr::null_mut(), + // }; + + // unsafe { Self::from_raw(inner) } } /// Creates wrapper by taking ownership of value. @@ -98,7 +102,7 @@ impl CertificateVerification { /// Ownership of the value passes to `Self`. This must only be used for values that are not /// contained within other values that may be dropped. #[must_use] - pub(crate) const unsafe fn from_raw(src: UA_CertificateVerification) -> Self { + pub(crate) const unsafe fn from_raw(src: UA_CertificateGroup) -> Self { Self(src) } @@ -112,7 +116,7 @@ impl CertificateVerification { #[expect(clippy::allow_attributes, reason = "non-static condition")] #[allow(clippy::missing_const_for_fn, reason = "unsupported before Rust 1.87")] #[must_use] - pub(crate) fn into_raw(self) -> UA_CertificateVerification { + pub(crate) fn into_raw(self) -> UA_CertificateGroup { // Use `ManuallyDrop` to avoid double-free even when added code might cause panic. See // documentation of `mem::forget()` for details. let this = ManuallyDrop::new(self); @@ -125,7 +129,7 @@ impl CertificateVerification { /// This initializes the value and makes all attributes well-defined. Additional attributes may /// need to be initialized for the value to be actually useful afterwards. pub(crate) const fn init() -> Self { - let inner = MaybeUninit::::zeroed(); + let inner = MaybeUninit::::zeroed(); // SAFETY: Zero-initialized memory is a valid certificate verification. let inner = unsafe { inner.assume_init() }; // SAFETY: We pass a value without pointers to it into `Self`. @@ -140,7 +144,7 @@ impl CertificateVerification { /// The logging reference will be transferred from the old to the new certificate verification. /// /// After this, it is the responsibility of `dst` to eventually clean up the data. - pub(crate) fn move_into_raw(self, dst: &mut UA_CertificateVerification) { + pub(crate) fn move_into_raw(self, dst: &mut UA_CertificateGroup) { // Move certificate verification into target, transferring ownership. let orig = mem::replace(dst, self.into_raw()); // Take ownership of previously set certificate verification in order to drop it. @@ -161,7 +165,7 @@ impl CertificateVerification { #[must_use] #[expect(clippy::allow_attributes, reason = "non-static condition")] #[allow(clippy::missing_const_for_fn, reason = "unsupported before Rust 1.87")] - pub(crate) unsafe fn as_mut(&mut self) -> &mut UA_CertificateVerification { + pub(crate) unsafe fn as_mut(&mut self) -> &mut UA_CertificateGroup { &mut self.0 } @@ -174,7 +178,7 @@ impl CertificateVerification { #[must_use] #[expect(clippy::allow_attributes, reason = "non-static condition")] #[allow(clippy::missing_const_for_fn, reason = "unsupported before Rust 1.87")] - pub(crate) unsafe fn as_mut_ptr(&mut self) -> *mut UA_CertificateVerification { + pub(crate) unsafe fn as_mut_ptr(&mut self) -> *mut UA_CertificateGroup { &raw mut self.0 } } diff --git a/src/ua/client.rs b/src/ua/client.rs index eb1cc30c..382bd1d9 100644 --- a/src/ua/client.rs +++ b/src/ua/client.rs @@ -1,8 +1,8 @@ use std::ptr::NonNull; use open62541_sys::{ - UA_Client, UA_Client_delete, UA_Client_disconnect, UA_Client_getConfig, UA_Client_getContext, - UA_Client_getState, UA_Client_new, UA_Client_newWithConfig, + UA_Client, UA_Client_delete, UA_Client_disconnect, UA_Client_getConfig, UA_Client_getState, + UA_Client_new, UA_Client_newWithConfig, }; use crate::{ua, ClientContext, DataType as _, Error}; @@ -118,7 +118,10 @@ impl Drop for Client { // Fetch context pointer before deleting client below, but free associated memory only after // client has completely shut down. - let context = unsafe { UA_Client_getContext(self.as_mut_ptr()) }.cast::(); + let context = unsafe { UA_Client_getConfig(self.as_mut_ptr()).as_ref() } + .expect("require client config") + .clientContext + .cast::(); // `UA_Client_delete()` matches `UA_Client_new()`. This may block (!) whenever the client is // still connected, for as long as it takes to take down the connection. This can be avoided From 248664fe4b68c20fd7d31366202085bca1aab23a Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Thu, 28 Aug 2025 18:12:28 +0200 Subject: [PATCH 02/10] Fix change in handling of variadic log messages --- src/ua/data_types/byte_string.rs | 11 ++++ src/ua/data_types/string.rs | 50 +++++++++++++++- src/ua/logger/rust_log.rs | 100 ++++++++++--------------------- 3 files changed, 93 insertions(+), 68 deletions(-) diff --git a/src/ua/data_types/byte_string.rs b/src/ua/data_types/byte_string.rs index 815bd240..189b321f 100644 --- a/src/ua/data_types/byte_string.rs +++ b/src/ua/data_types/byte_string.rs @@ -105,6 +105,17 @@ impl ByteString { unsafe { self.as_bytes().unwrap_unchecked() } } + /// Converts byte string into string. + /// + /// This is lossless because the underlying types of OPC UA strings and byte strings are exactly + /// the same, only with different semantics. + #[must_use] + pub(crate) fn into_string(self) -> ua::String { + let string = self.into_raw(); + // SAFETY: We still own this string. + unsafe { ua::String::from_raw(string) } + } + #[must_use] fn array_value(&self) -> ArrayValue { // Internally, `open62541` represents strings as `Byte` array and has the same special cases diff --git a/src/ua/data_types/string.rs b/src/ua/data_types/string.rs index 00cc8cc3..10c187c1 100644 --- a/src/ua/data_types/string.rs +++ b/src/ua/data_types/string.rs @@ -1,6 +1,6 @@ use std::{ffi::CString, fmt, ptr, slice, str}; -use open62541_sys::UA_String_fromChars; +use open62541_sys::{UA_ByteString_allocBuffer, UA_String_fromChars}; use crate::{ua, ArrayValue, DataType as _, Error}; @@ -24,6 +24,24 @@ impl String { Ok(Self(str)) } + /// Creates uninitialized string of specific length. + /// + /// This should be used with caution: the uninitialized contents may contain leaked data and may + /// not be valid UTF-8. + #[must_use] + pub(crate) fn uninit(len: usize) -> Self { + let mut str = ua::ByteString::init(); + // We let `UA_ByteString_allocBuffer()` do the string allocation. + let status_code = + ua::StatusCode::new(unsafe { UA_ByteString_allocBuffer(str.as_mut_ptr(), len) }); + // PANIC: The only possible errors here are out-of-memory. + assert!( + status_code.is_good(), + "byte string should have been created" + ); + str.into_string() + } + /// Creates invalid string (as defined by OPC UA). // TODO: The OPC UA specification calls invalid strings "null". Consider changing this to match. pub(crate) fn invalid() -> Self { @@ -37,6 +55,22 @@ impl String { Self::new("").unwrap() } + /// Gets string length. + /// + /// This may return [`None`] when the string itself is invalid (as defined by OPC UA). + #[expect(dead_code, reason = "unused for now")] + #[must_use] + pub(crate) fn len(&self) -> Option { + match self.array_value() { + ArrayValue::Invalid => None, + ArrayValue::Empty => Some(0), + ArrayValue::Valid(_) => { + // `self.0.data` is valid, so we may use `self.0.length` now. + Some(self.0.length) + } + } + } + /// Checks if string is invalid. /// /// The invalid state is defined by OPC UA. It is a third state which is distinct from empty and @@ -69,6 +103,20 @@ impl String { } } + #[must_use] + pub(crate) fn as_mut_bytes(&mut self) -> Option<&mut [u8]> { + // Internally, `open62541` represents strings as `Byte` array and has the same special cases + // as regular arrays, i.e. empty and invalid states. + match self.array_value() { + ArrayValue::Invalid => None, + ArrayValue::Empty => Some(&mut []), + ArrayValue::Valid(data) => { + // `self.0.data` is valid, so we may use `self.0.length` now. + Some(unsafe { slice::from_raw_parts_mut(data.as_ptr(), self.0.length) }) + } + } + } + /// Returns string contents as string slice. /// /// This may return [`None`] when the string itself is invalid (as defined by OPC UA) or when it diff --git a/src/ua/logger/rust_log.rs b/src/ua/logger/rust_log.rs index b2e83dfd..8ee2e515 100644 --- a/src/ua/logger/rust_log.rs +++ b/src/ua/logger/rust_log.rs @@ -1,11 +1,11 @@ use std::{ - ffi::{c_char, c_void, CStr}, + ffi::{c_char, c_void}, ptr, }; -use open62541_sys::{vsnprintf_va_copy, vsnprintf_va_end, UA_LogCategory, UA_LogLevel, UA_Logger}; +use open62541_sys::{UA_LogCategory, UA_LogLevel, UA_Logger, UA_String_vformat}; -use crate::ua; +use crate::{ua, DataType as _, Error, Result}; const LOG_TARGET: &str = "open62541_sys"; @@ -21,14 +21,15 @@ pub(crate) fn logger() -> ua::Logger { msg: *const c_char, args: open62541_sys::va_list_, ) { - let Some(msg) = format_message(msg, args) else { - log::error!(target: LOG_TARGET, "Unknown log message"); - return; + let msg = match format_message(msg, args) { + Ok(msg) => msg, + Err(error) => { + log::error!(target: LOG_TARGET, "Unknown log message: {error}"); + return; + } }; - let msg = CStr::from_bytes_with_nul(&msg) - .unwrap_or(c"Invalid log message") - .to_string_lossy(); + let msg = msg.as_str().unwrap_or("Invalid log message"); if level == UA_LogLevel::UA_LOGLEVEL_FATAL { // Without fatal level in `log`, fall back to error. @@ -89,70 +90,35 @@ pub(crate) fn logger() -> ua::Logger { unsafe { ua::Logger::from_raw(logger) } } -/// Initial buffer size when formatting messages. -const FORMAT_MESSAGE_DEFAULT_BUFFER_LEN: usize = 128; - -/// Maximum buffer size when formatting messages. -const FORMAT_MESSAGE_MAXIMUM_BUFFER_LEN: usize = 65536; +/// Buffer size when formatting messages. +// This matches the limit used by default implementations `ua_log_stdout.h` and `ua_log_syslog.h`. +const FORMAT_MESSAGE_BUFFER_LEN: usize = 512; /// Formats message with `vprintf` library calls. /// -/// This returns the formatted message with a trailing NUL byte, or `None` when formatting fails. A -/// long message may be truncated (see [`FORMAT_MESSAGE_MAXIMUM_BUFFER_LEN`] for details); its last -/// characters will be replaced with `...` to indicate this. -fn format_message(msg: *const c_char, args: open62541_sys::va_list_) -> Option> { - // Delegate string formatting to `vsnprintf()`, the length-checked string buffer variant of the - // variadic `vprintf` family. - // - // We use the custom `vsnprintf_va_copy()` provided by `open62541_sys`. This copies the va args - // and requires an explicit call to `vsnprintf_va_end()` afterwards. - - // Allocate default buffer first. Only when the message doesn't fit, we need to allocate larger - // buffer below. - let mut msg_buffer: Vec = vec![0; FORMAT_MESSAGE_DEFAULT_BUFFER_LEN]; - loop { - let result = unsafe { - vsnprintf_va_copy( - msg_buffer.as_mut_ptr().cast::(), - msg_buffer.len(), - msg, - args, - ) - }; - let Ok(msg_len) = usize::try_from(result) else { - // Negative result is an error in the format string. Nothing we can do. - debug_assert!(result < 0); - // Free the `va_list` argument that is no consumed by `vsnprintf()`! - unsafe { vsnprintf_va_end(args) } - return None; - }; - let buffer_len = msg_len + 1; - if buffer_len > msg_buffer.len() { - // Last byte must always be the NUL terminator, even if the message - // doesn't fit into the buffer. - debug_assert_eq!(msg_buffer.last(), Some(&0)); - if msg_buffer.len() < FORMAT_MESSAGE_MAXIMUM_BUFFER_LEN { - // Allocate larger buffer and try again. - msg_buffer.resize(FORMAT_MESSAGE_MAXIMUM_BUFFER_LEN, 0); - continue; - } - // Message is too large to format. Truncate the message by ending it with `...`. - for char in msg_buffer.iter_mut().rev().skip(1).take(3) { +/// This returns the formatted message as string, or `Err` when formatting fails. A long message is +/// truncated (see [`FORMAT_MESSAGE_BUFFER_LEN`]); its last characters will be replaced with `...`. +fn format_message(msg: *const c_char, args: open62541_sys::va_list_) -> Result { + // With non-zero length, `UA_String_vformat()` fills the given string directly. For zero length + // strings, the result would be dynamically allocated, but this risks handling incredibly large + // amounts of memory in the log handler which we want to avoid here. + let mut msg_buffer = ua::String::uninit(FORMAT_MESSAGE_BUFFER_LEN); + + let status_code = + ua::StatusCode::new(unsafe { UA_String_vformat(msg_buffer.as_mut_ptr(), msg, args) }); + if status_code == ua::StatusCode::BADENCODINGLIMITSEXCEEDED { + // Message is too large to format. We could try again with a larger buffer, but since we do + // not know the required length (`UA_String_vformat()` doesn't return it), we would have to + // guess (e.g., doubling the length until the message fits). Simply truncate the message by + // ending it with `...` instead to ensure constant-time operation. + if let Some(msg_buffer) = msg_buffer.as_mut_bytes() { + for char in msg_buffer.iter_mut().rev().take(3) { *char = b'.'; } - } else { - // Message fits into the buffer. Make sure that `from_bytes_with_nul()` - // sees the expected single NUL terminator in the final position. - msg_buffer.truncate(buffer_len); } - break; + } else { + Error::verify_good(&status_code)?; } - // Free the `va_list` argument that is not consumed by `vsnprintf()`! - unsafe { vsnprintf_va_end(args) } - - // Last byte must always be the NUL terminator. - debug_assert_eq!(msg_buffer.last(), Some(&0)); - - Some(msg_buffer) + Ok(msg_buffer) } From 9a794c5bf1a8d02510eb6f1ce52969744c6e07d2 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Fri, 29 Aug 2025 12:57:59 +0200 Subject: [PATCH 03/10] Add workaround for unset node lifecycle config --- src/server.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/server.rs b/src/server.rs index c0d11123..4f7ae83b 100644 --- a/src/server.rs +++ b/src/server.rs @@ -264,9 +264,12 @@ impl ServerBuilder { let config = self.config_mut(); - // PANIC: We never set lifecycle hooks elsewhere in config. - debug_assert!(unsafe { &*config.nodeLifecycle }.destructor.is_none()); - unsafe { &mut *config.nodeLifecycle }.destructor = Some(destructor_c); + // FIXME: Initialize lifecycle hooks object. + if !config.nodeLifecycle.is_null() { + // PANIC: We never set lifecycle hooks elsewhere in config. + debug_assert!(unsafe { &*config.nodeLifecycle }.destructor.is_none()); + unsafe { &mut *config.nodeLifecycle }.destructor = Some(destructor_c); + } let Self { config, From f270f6db6ff3bd9e58c31e9b9ac918a54bfbc23d Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Fri, 29 Aug 2025 15:52:44 +0200 Subject: [PATCH 04/10] Skip unnecessary log formatting, include log category --- src/ua/logger/rust_log.rs | 78 ++++++++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/src/ua/logger/rust_log.rs b/src/ua/logger/rust_log.rs index 8ee2e515..5f095023 100644 --- a/src/ua/logger/rust_log.rs +++ b/src/ua/logger/rust_log.rs @@ -3,51 +3,77 @@ use std::{ ptr, }; +use log::Level; use open62541_sys::{UA_LogCategory, UA_LogLevel, UA_Logger, UA_String_vformat}; use crate::{ua, DataType as _, Error, Result}; +// This matches the crate name. const LOG_TARGET: &str = "open62541_sys"; +// These match the category names from `ua_log_stdout.c` and `ua_log_syslog.c`. +const LOG_CATEGORY_NETWORK: &str = "network"; +const LOG_CATEGORY_SECURECHANNEL: &str = "channel"; +const LOG_CATEGORY_SESSION: &str = "session"; +const LOG_CATEGORY_SERVER: &str = "server"; +const LOG_CATEGORY_CLIENT: &str = "client"; +const LOG_CATEGORY_USERLAND: &str = "userland"; +const LOG_CATEGORY_SECURITYPOLICY: &str = "security"; +const LOG_CATEGORY_EVENTLOOP: &str = "eventloop"; +const LOG_CATEGORY_PUBSUB: &str = "pubsub"; +const LOG_CATEGORY_DISCOVERY: &str = "discovery"; +const LOG_CATEGORY_UNKNOWN: &str = "unknown"; + /// Creates logger that forwards to the `log` crate. /// -/// We can use this to prevent `open62541` from installing its own default logger (which outputs any +/// We can use this to prevent `open62541` from installing its own default logger (which outputs all /// logs to stdout/stderr directly). pub(crate) fn logger() -> ua::Logger { unsafe extern "C" fn log_c( _log_context: *mut c_void, level: UA_LogLevel, - _category: UA_LogCategory, + category: UA_LogCategory, msg: *const c_char, args: open62541_sys::va_list_, ) { - let msg = match format_message(msg, args) { - Ok(msg) => msg, - Err(error) => { - log::error!(target: LOG_TARGET, "Unknown log message: {error}"); - return; - } - }; - - let msg = msg.as_str().unwrap_or("Invalid log message"); - - if level == UA_LogLevel::UA_LOGLEVEL_FATAL { + let level = match level { // Without fatal level in `log`, fall back to error. - log::error!(target: LOG_TARGET, "{msg}"); - } else if level == UA_LogLevel::UA_LOGLEVEL_ERROR { - log::error!(target: LOG_TARGET, "{msg}"); - } else if level == UA_LogLevel::UA_LOGLEVEL_WARNING { - log::warn!(target: LOG_TARGET, "{msg}"); - } else if level == UA_LogLevel::UA_LOGLEVEL_INFO { - log::info!(target: LOG_TARGET, "{msg}"); - } else if level == UA_LogLevel::UA_LOGLEVEL_DEBUG { - log::debug!(target: LOG_TARGET, "{msg}"); - } else if level == UA_LogLevel::UA_LOGLEVEL_TRACE { - log::trace!(target: LOG_TARGET, "{msg}"); - } else { + UA_LogLevel::UA_LOGLEVEL_FATAL | UA_LogLevel::UA_LOGLEVEL_ERROR => Level::Error, + UA_LogLevel::UA_LOGLEVEL_WARNING => Level::Warn, + UA_LogLevel::UA_LOGLEVEL_INFO => Level::Info, + UA_LogLevel::UA_LOGLEVEL_DEBUG => Level::Debug, + UA_LogLevel::UA_LOGLEVEL_TRACE => Level::Trace, // Handle unexpected level by escalating to error. - log::error!(target: LOG_TARGET, "{msg}"); + #[expect(clippy::match_same_arms, reason = "distinction of cases")] + _ => Level::Error, + }; + + if !log::log_enabled!(target: LOG_TARGET, level) { + // Bail out early to skip formatting message. + return; } + + let msg = format_message(msg, args); + let msg = match msg { + Ok(ref msg) => msg.as_str().unwrap_or("Invalid log message"), + Err(_) => "Unknown log message", + }; + + let category = match category { + UA_LogCategory::UA_LOGCATEGORY_NETWORK => LOG_CATEGORY_NETWORK, + UA_LogCategory::UA_LOGCATEGORY_SECURECHANNEL => LOG_CATEGORY_SECURECHANNEL, + UA_LogCategory::UA_LOGCATEGORY_SESSION => LOG_CATEGORY_SESSION, + UA_LogCategory::UA_LOGCATEGORY_SERVER => LOG_CATEGORY_SERVER, + UA_LogCategory::UA_LOGCATEGORY_CLIENT => LOG_CATEGORY_CLIENT, + UA_LogCategory::UA_LOGCATEGORY_USERLAND => LOG_CATEGORY_USERLAND, + UA_LogCategory::UA_LOGCATEGORY_SECURITYPOLICY => LOG_CATEGORY_SECURITYPOLICY, + UA_LogCategory::UA_LOGCATEGORY_EVENTLOOP => LOG_CATEGORY_EVENTLOOP, + UA_LogCategory::UA_LOGCATEGORY_PUBSUB => LOG_CATEGORY_PUBSUB, + UA_LogCategory::UA_LOGCATEGORY_DISCOVERY => LOG_CATEGORY_DISCOVERY, + _ => LOG_CATEGORY_UNKNOWN, + }; + + log::log!(target: LOG_TARGET, level, "({category}) {msg}"); } unsafe extern "C" fn clear_c(logger: *mut UA_Logger) { From 358835ff02c840c8f202b4dbb94a1b384b11a947 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Mon, 1 Sep 2025 13:02:30 +0200 Subject: [PATCH 05/10] Fix handling of `config.nodeLifecycle` --- src/server.rs | 87 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 13 deletions(-) diff --git a/src/server.rs b/src/server.rs index 4f7ae83b..7b350a26 100644 --- a/src/server.rs +++ b/src/server.rs @@ -7,14 +7,14 @@ mod node_types; use std::{ any::Any, ffi::{c_void, CString}, - ptr, + mem, ptr, sync::Arc, time::Instant, }; use derive_more::Debug; use open62541_sys::{ - UA_CertificateGroup_AcceptAll, UA_NodeId, UA_Server, UA_ServerConfig, + UA_CertificateGroup_AcceptAll, UA_GlobalNodeLifecycle, UA_NodeId, UA_Server, UA_ServerConfig, UA_Server_addCallbackValueSourceVariableNode, UA_Server_addMethodNodeEx, UA_Server_addNamespace, UA_Server_addNode_begin, UA_Server_addNode_finish, UA_Server_addReference, UA_Server_browse, UA_Server_browseNext, UA_Server_browseRecursive, @@ -69,6 +69,9 @@ pub use self::{ pub struct ServerBuilder { config: ua::ServerConfig, + /// Heap-allocated set of callbacks in `config.nodeLifecycle`. + global_node_lifecycle: Box, + /// [`AccessControl`] instances may hold additional data that must be kept alive until server is /// shut down. The sentinel value cleans this up when it is dropped. access_control_sentinel: Option>, @@ -78,6 +81,7 @@ impl ServerBuilder { fn new(config: ua::ServerConfig) -> Self { Self { config, + global_node_lifecycle: Box::default(), access_control_sentinel: None, } } @@ -227,7 +231,7 @@ impl ServerBuilder { /// Builds OPC UA server. #[must_use] - pub fn build(mut self) -> (Server, ServerRunner) { + pub fn build(self) -> (Server, ServerRunner) { unsafe extern "C" fn destructor_c( _server: *mut UA_Server, _session_id: *const UA_NodeId, @@ -262,25 +266,25 @@ impl ServerBuilder { } } - let config = self.config_mut(); - - // FIXME: Initialize lifecycle hooks object. - if !config.nodeLifecycle.is_null() { - // PANIC: We never set lifecycle hooks elsewhere in config. - debug_assert!(unsafe { &*config.nodeLifecycle }.destructor.is_none()); - unsafe { &mut *config.nodeLifecycle }.destructor = Some(destructor_c); - } - let Self { - config, + mut config, + mut global_node_lifecycle, access_control_sentinel, } = self; + // PANIC: We never set lifecycle hooks elsewhere in config. + debug_assert!(global_node_lifecycle.destructor.is_none()); + global_node_lifecycle.destructor = Some(destructor_c); + + // SAFETY: We keep `global_node_lifecycle` alive until `clear_global_node_lifecycle()`. + unsafe { set_global_node_lifecycle(&mut config, &mut global_node_lifecycle) }; + let server = Arc::new(ua::Server::new_with_config(config)); let state = Arc::new(RunnerState::new()); let runner = ServerRunner::new( Arc::clone(&server), + global_node_lifecycle, access_control_sentinel, Arc::clone(&state), ); @@ -1695,6 +1699,9 @@ impl Server { pub struct ServerRunner { server: Arc, + /// Heap-allocated set of callbacks in `config.nodeLifecycle`. + global_node_lifecycle: Box, + /// [`AccessControl`] instances may hold additional data that must be kept alive until server is /// shut down. The sentinel value cleans this up when it is dropped. access_control_sentinel: Option>, @@ -1706,11 +1713,13 @@ impl ServerRunner { #[must_use] fn new( server: Arc, + global_node_lifecycle: Box, access_control_sentinel: Option>, state: Arc, ) -> Self { Self { server, + global_node_lifecycle, access_control_sentinel, state, } @@ -1737,9 +1746,11 @@ impl ServerRunner { pub fn run_until_interrupt(mut self) -> Result<()> { let Self { server, + global_node_lifecycle, access_control_sentinel, state, } = &mut self; + let global_node_lifecycle = mem::take(global_node_lifecycle); let access_control_sentinel = access_control_sentinel.take(); let mut state_guard = state.lock(); @@ -1767,6 +1778,8 @@ impl ServerRunner { } } + unsafe { clear_global_node_lifecycle(server, global_node_lifecycle) }; + // Compile-time assertion to make sure that the sentinel value was still around for the call // above (including any branches that exit early with `?` or `return`): only when the server // has finished shutting down, we are allowed to drop sentinel values. @@ -1786,9 +1799,11 @@ impl ServerRunner { pub fn run_until_cancelled(mut self, mut is_cancelled: impl FnMut() -> bool) -> Result<()> { let Self { server, + global_node_lifecycle, access_control_sentinel, state, } = &mut self; + let global_node_lifecycle = mem::take(global_node_lifecycle); let access_control_sentinel = access_control_sentinel.take(); let mut state_guard = state.lock(); @@ -1863,6 +1878,8 @@ impl ServerRunner { } } + unsafe { clear_global_node_lifecycle(server, global_node_lifecycle) }; + // Compile-time assertion to make sure that the sentinel value was still around for the call // above (including any branches that exit early with `?` or `return`): only when the server // has finished shutting down, we are allowed to drop sentinel values. @@ -1895,3 +1912,47 @@ fn to_browse_result(result: &ua::BrowseResult) -> BrowseResult { Ok((references.into_vec(), result.continuation_point())) } + +/// Sets `config.nodeLifecycle` to the given value. +/// +/// # Safety +/// +/// This must be called before the server accesses `config.nodeLifecycle`. +/// +/// The boxed value `global_node_lifecycle` must be kept alive until `clear_global_node_lifecycle()` +/// has been called. +unsafe fn set_global_node_lifecycle( + config: &mut ua::ServerConfig, + global_node_lifecycle: &mut Box, +) { + let config = unsafe { config.as_mut() }; + + // PANIC: Nobody else has initialized the node lifecycle callbacks before. + debug_assert!(config.nodeLifecycle.is_null()); + + // This stores the pointer to the `Box`-allocated `global_node_lifecycle` in the config; it must + // be kept alive elsewhere (e.g., in `ServerRunner`) until the server no longer accesses it. + config.nodeLifecycle = &raw mut *global_node_lifecycle.as_mut(); +} + +/// Clears `config.nodeLifecycle`. +/// +/// # Safety +/// +/// This must be called after the server has finished accessing `config.nodeLifecycle`. +unsafe fn clear_global_node_lifecycle( + server: &ua::Server, + mut global_node_lifecycle: Box, +) { + // SAFETY: The server has finished execution and we can mutate the configuration. + let config = unsafe { &mut *UA_Server_getConfig(server.as_ptr().cast_mut()) }; + + debug_assert_eq!( + config.nodeLifecycle, + &raw mut *global_node_lifecycle.as_mut() + ); + + // Remove pointer to prevent server from making any more calls. `global_node_lifecycle` goes out + // of scope at the end of this function and gets freed. + config.nodeLifecycle = ptr::null_mut(); +} From 6bced2ea8fe4e6f4ae6eb0a09f8cbe4f5bb93748 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Mon, 1 Sep 2025 14:51:22 +0200 Subject: [PATCH 06/10] Skip broken builds on Ubuntu 22.04 with GNU libc --- .github/workflows/latest-dependencies.yaml | 12 ++++++++++-- .github/workflows/test.yaml | 12 ++++++++++-- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/.github/workflows/latest-dependencies.yaml b/.github/workflows/latest-dependencies.yaml index 5c1ed049..71bdf0a5 100644 --- a/.github/workflows/latest-dependencies.yaml +++ b/.github/workflows/latest-dependencies.yaml @@ -99,14 +99,18 @@ jobs: cargo update --verbose - name: Build with feature combinations + # Disable build for GNU targets on Ubuntu 22.04 (unsupported `atomic_uintptr_t`). + # TODO: Try to re-enable this step regardless of the build target in the future. + if: ${{ matrix.runner_os != 'ubuntu-22.04' || endsWith(matrix.target, '-musl') }} run: >- cargo hack --each-feature build --locked - name: Run tests (bins/lib/tests/examples) with feature combinations + # Disable build for GNU targets on Ubuntu 22.04 (unsupported `atomic_uintptr_t`). # Disable tests for musl libc target(s) due to build failure for unknown reasons. # TODO: Try to re-enable this step regardless of the build target in the future. - if: ${{ !endsWith(matrix.target, '-musl') }} + if: ${{ matrix.runner_os != 'ubuntu-22.04' && !endsWith(matrix.target, '-musl') }} run: >- cargo hack --each-feature test --locked @@ -119,15 +123,19 @@ jobs: # certain features only for some doctests, so we run them without # `cargo-hack`. - name: Run doctests with all features enabled + # Disable build for GNU targets on Ubuntu 22.04 (unsupported `atomic_uintptr_t`). # Disable tests for musl libc target(s) due to build failure for unknown reasons. # TODO: Try to re-enable this step regardless of the build target in the future. - if: ${{ !endsWith(matrix.target, '-musl') }} + if: ${{ matrix.runner_os != 'ubuntu-22.04' && !endsWith(matrix.target, '-musl') }} run: >- cargo test --locked --all-features --doc - name: Build package with all features enabled + # Disable build for GNU targets on Ubuntu 22.04 (unsupported `atomic_uintptr_t`). + # TODO: Try to re-enable this step regardless of the build target in the future. + if: ${{ matrix.runner_os != 'ubuntu-22.04' || endsWith(matrix.target, '-musl') }} # We allow dirty state here because it is only expected after update. run: >- cargo package diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 78f63c8b..47bebe2f 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -113,14 +113,18 @@ jobs: shared-key: ${{ github.workflow }}-${{ github.job }}-${{ matrix.target }}-${{ matrix.runner_os }} - name: Build with feature combinations + # Disable build for GNU targets on Ubuntu 22.04 (unsupported `atomic_uintptr_t`). + # TODO: Try to re-enable this step regardless of the build target in the future. + if: ${{ matrix.runner_os != 'ubuntu-22.04' || endsWith(matrix.target, '-musl') }} run: >- cargo hack --each-feature build --locked - name: Run tests (bins/lib/tests/examples) with feature combinations + # Disable build for GNU targets on Ubuntu 22.04 (unsupported `atomic_uintptr_t`). # Disable tests for musl libc target(s) due to build failure for unknown reasons. # TODO: Try to re-enable this step regardless of the build target in the future. - if: ${{ !endsWith(matrix.target, '-musl') }} + if: ${{ matrix.runner_os != 'ubuntu-22.04' && !endsWith(matrix.target, '-musl') }} run: >- cargo hack --each-feature test --locked @@ -133,15 +137,19 @@ jobs: # certain features only for some doctests, so we run them without # `cargo-hack`. - name: Run doctests with all features enabled + # Disable build for GNU targets on Ubuntu 22.04 (unsupported `atomic_uintptr_t`). # Disable tests for musl libc target(s) due to build failure for unknown reasons. # TODO: Try to re-enable this step regardless of the build target in the future. - if: ${{ !endsWith(matrix.target, '-musl') }} + if: ${{ matrix.runner_os != 'ubuntu-22.04' && !endsWith(matrix.target, '-musl') }} run: >- cargo test --locked --all-features --doc - name: Build package with all features enabled + # Disable build for GNU targets on Ubuntu 22.04 (unsupported `atomic_uintptr_t`). + # TODO: Try to re-enable this step regardless of the build target in the future. + if: ${{ matrix.runner_os != 'ubuntu-22.04' || endsWith(matrix.target, '-musl') }} run: >- cargo package --locked --all-features From 044b2026c3bf867fd26cc7ab0ce9903b914dab0a Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Mon, 1 Sep 2025 14:58:15 +0200 Subject: [PATCH 07/10] Remove unsupported custom certificate verification for now --- src/lib.rs | 5 +-- src/traits.rs | 13 ------ src/ua/certificate_verification.rs | 70 +----------------------------- 3 files changed, 2 insertions(+), 86 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index d3a6c2a3..774a7ecc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -254,10 +254,7 @@ pub use self::{ MethodNode, Node, ObjectNode, Server, ServerBuilder, ServerRunner, VariableNode, }, service::{ServiceRequest, ServiceResponse}, - traits::{ - Attribute, Attributes, CustomCertificateVerification, DataTypeExt, FilterOperand, - MonitoringFilter, - }, + traits::{Attribute, Attributes, DataTypeExt, FilterOperand, MonitoringFilter}, userdata::{Userdata, UserdataSentinel}, value::{ScalarValue, ValueType, VariantValue}, }; diff --git a/src/traits.rs b/src/traits.rs index 41f889ca..ddc3fcc6 100644 --- a/src/traits.rs +++ b/src/traits.rs @@ -77,19 +77,6 @@ pub trait Attributes: DataType { fn as_node_attributes(&self) -> &ua::NodeAttributes; } -/// Custom certificate verification. -/// -/// This is used to implement custom callbacks in [`ua::CertificateVerification::custom()`]. -pub trait CustomCertificateVerification { - fn verify_certificate(&self, certificate: &ua::ByteString) -> ua::StatusCode; - - fn verify_application_uri( - &self, - certificate: &ua::ByteString, - application_uri: &ua::String, - ) -> ua::StatusCode; -} - /// Private-key password callback. /// /// This is used to fetch the password for a given client private key when establishing a connection diff --git a/src/ua/certificate_verification.rs b/src/ua/certificate_verification.rs index 76a64d13..65a2f4df 100644 --- a/src/ua/certificate_verification.rs +++ b/src/ua/certificate_verification.rs @@ -4,11 +4,7 @@ use std::{ }; use derive_more::Debug; -use open62541_sys::{ - UA_ByteString, UA_CertificateGroup, UA_CertificateGroup_AcceptAll, UA_StatusCode, UA_String, -}; - -use crate::{ua, CustomCertificateVerification, DataType, Userdata}; +use open62541_sys::{UA_CertificateGroup, UA_CertificateGroup_AcceptAll}; /// Wrapper for [`UA_CertificateGroup`] from [`open62541_sys`]. #[derive(Debug)] @@ -29,70 +25,6 @@ impl CertificateVerification { certificate_verification } - /// Creates certificate verification with custom callbacks. - #[expect(dead_code, reason = "work in progress")] - pub fn custom(_certificate_verification: impl CustomCertificateVerification + 'static) -> Self { - type Ud = Userdata>; - - unsafe extern "C" fn verify_certificate_c( - cv: *const UA_CertificateGroup, - certificate: *const UA_ByteString, - ) -> UA_StatusCode { - // SAFETY: Reference is used only for the remainder of this function. - let certificate = ua::ByteString::raw_ref(unsafe { - certificate.as_ref().expect("certificate should be set") - }); - - // SAFETY: We use the user data only when it is still alive. - let certificate_verification = unsafe { Ud::peek_at((*cv).context) }; - let status_code = certificate_verification.verify_certificate(certificate); - status_code.into_raw() - } - - unsafe extern "C" fn verify_application_uri_c( - cv: *const UA_CertificateGroup, - certificate: *const UA_ByteString, - application_uri: *const UA_String, - ) -> UA_StatusCode { - // SAFETY: References are used only for the remainder of this function. - let certificate = ua::ByteString::raw_ref(unsafe { - certificate.as_ref().expect("certificate should be set") - }); - let application_uri = ua::String::raw_ref(unsafe { - application_uri - .as_ref() - .expect("application URI should be set") - }); - - // SAFETY: We use the user data only when it is still alive. - let certificate_verification = unsafe { Ud::peek_at((*cv).context) }; - let status_code = - certificate_verification.verify_application_uri(certificate, application_uri); - status_code.into_raw() - } - - unsafe extern "C" fn clear_c(cv: *mut UA_CertificateGroup) { - // Reclaim ownership of certificate verification and drop it. - // SAFETY: We use the user data only when it is still alive. - let _unused = unsafe { Ud::consume((*cv).context) }; - } - - // FIXME: Implement `UA_CertificateGroup`. - unimplemented!() - - // let inner = UA_CertificateGroup { - // context: Ud::prepare(Box::new(certificate_verification)), - // verifyCertificate: Some(verify_certificate_c), - // verifyApplicationURI: Some(verify_application_uri_c), - // getExpirationDate: None, - // getSubjectName: None, - // clear: Some(clear_c), - // logging: ptr::null_mut(), - // }; - - // unsafe { Self::from_raw(inner) } - } - /// Creates wrapper by taking ownership of value. /// /// When `Self` is dropped, allocations held by the inner type are cleaned up. From 1b00e19ff61d6619ac416980239a5ab496bbf45f Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Mon, 1 Sep 2025 14:58:26 +0200 Subject: [PATCH 08/10] Add changelog messages --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35db552a..04cca40b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Changed +- Breaking: Upgrade to open62541 version + [1.5.0](https://github.com/open62541/open62541/releases/tag/v1.5.0). +- Breaking: Remove method `ua::CertificateVerification::custom()` and trait + `CustomCertificateVerification`. - Include category ("network", "channel", ..., "discovery") in log messages. - Skip overhead of formatting log message when corresponding log level has been disabled. From 9add1df7f514d45dcf363afb9924e5654baadf66 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Mon, 1 Sep 2025 15:46:10 +0200 Subject: [PATCH 09/10] Add `ua::CertificateVerification::memory_store()` stub --- CHANGELOG.md | 4 +++ src/ua/certificate_verification.rs | 32 ++++++++++++++++- src/ua/data_types.rs | 2 ++ src/ua/data_types/trust_list_data_type.rs | 44 +++++++++++++++++++++++ 4 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 src/ua/data_types/trust_list_data_type.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 04cca40b..5eb4a579 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added + +- Add method `ua::CertificateVerification::memory_store()` and `ua::TrustListDataType`. + ### Changed - Breaking: Upgrade to open62541 version diff --git a/src/ua/certificate_verification.rs b/src/ua/certificate_verification.rs index 65a2f4df..244af89e 100644 --- a/src/ua/certificate_verification.rs +++ b/src/ua/certificate_verification.rs @@ -4,8 +4,13 @@ use std::{ }; use derive_more::Debug; +#[cfg(feature = "mbedtls")] +use open62541_sys::UA_CertificateGroup_Memorystore; use open62541_sys::{UA_CertificateGroup, UA_CertificateGroup_AcceptAll}; +#[cfg(feature = "mbedtls")] +use crate::{ua, DataType as _}; + /// Wrapper for [`UA_CertificateGroup`] from [`open62541_sys`]. #[derive(Debug)] pub struct CertificateVerification(#[debug(skip)] UA_CertificateGroup); @@ -18,13 +23,38 @@ impl CertificateVerification { #[must_use] pub fn accept_all() -> Self { let mut certificate_verification = Self::init(); - // SAFETY: Certificate verification is null, but that is valid. + // SAFETY: Certificate verification is uninitialized, but that is expected. unsafe { UA_CertificateGroup_AcceptAll(certificate_verification.as_mut_ptr()); } certificate_verification } + #[cfg(feature = "mbedtls")] + #[must_use] + pub fn memory_store( + certificate_group_id: &ua::NodeId, + trust_list: Option<&ua::TrustListDataType>, + ) -> Self { + let mut certificate_verification = Self::init(); + // SAFETY: Certificate verification is uninitialized, but that is expected. + unsafe { + UA_CertificateGroup_Memorystore( + certificate_verification.as_mut_ptr(), + // SAFETY: Argument is only read, not modified or returned. + certificate_group_id.as_ptr().cast_mut(), + trust_list.map_or(ptr::null(), |trust_list| trust_list.as_ptr()), + // FIXME: Initialize logger. + ptr::null(), + // TODO: Allow parameters. Valid parameters are: + // - "max-trust-listsize" + // - "max-rejected-listsize" + ptr::null(), + ); + } + certificate_verification + } + /// Creates wrapper by taking ownership of value. /// /// When `Self` is dropped, allocations held by the inner type are cleaned up. diff --git a/src/ua/data_types.rs b/src/ua/data_types.rs index b00d9f4e..55d16284 100644 --- a/src/ua/data_types.rs +++ b/src/ua/data_types.rs @@ -76,6 +76,7 @@ mod status_code; mod string; mod structure_definition; mod timestamps_to_return; +mod trust_list_data_type; mod user_name_identity_token; mod variant; mod write_request; @@ -164,6 +165,7 @@ pub use self::{ string::String, structure_definition::StructureDefinition, timestamps_to_return::TimestampsToReturn, + trust_list_data_type::TrustListDataType, user_name_identity_token::UserNameIdentityToken, variant::Variant, write_request::WriteRequest, diff --git a/src/ua/data_types/trust_list_data_type.rs b/src/ua/data_types/trust_list_data_type.rs new file mode 100644 index 00000000..f9412aa8 --- /dev/null +++ b/src/ua/data_types/trust_list_data_type.rs @@ -0,0 +1,44 @@ +use crate::ua; + +crate::data_type!(TrustListDataType); + +impl TrustListDataType { + #[must_use] + pub fn with_trusted_certificates(mut self, trusted_certificates: &[ua::String]) -> Self { + let array = ua::Array::from_slice(trusted_certificates); + array.move_into_raw( + &mut self.0.trustedCertificatesSize, + &mut self.0.trustedCertificates, + ); + // FIXME: How to set masks? + // self.0.specifiedLists |= UA_TRUSTLISTMASKS_TRUSTEDCERTIFICATES; + self + } + + #[must_use] + pub fn with_trusted_crls(mut self, trusted_crls: &[ua::String]) -> Self { + let array = ua::Array::from_slice(trusted_crls); + array.move_into_raw(&mut self.0.trustedCrlsSize, &mut self.0.trustedCrls); + // FIXME: How to set masks? + self + } + + #[must_use] + pub fn with_issuer_certificates(mut self, issuer_certificates: &[ua::String]) -> Self { + let array = ua::Array::from_slice(issuer_certificates); + array.move_into_raw( + &mut self.0.issuerCertificatesSize, + &mut self.0.issuerCertificates, + ); + // FIXME: How to set masks? + self + } + + #[must_use] + pub fn with_issuer_crls(mut self, issuer_crls: &[ua::String]) -> Self { + let array = ua::Array::from_slice(issuer_crls); + array.move_into_raw(&mut self.0.issuerCrlsSize, &mut self.0.issuerCrls); + // FIXME: How to set masks? + self + } +} From 531393826fd9cc45e376f80bfe8a185f5670a562 Mon Sep 17 00:00:00 2001 From: Sebastian Goll Date: Mon, 1 Sep 2025 15:58:05 +0200 Subject: [PATCH 10/10] Set trust list masks --- src/ua.rs | 2 + src/ua/data_types/trust_list_data_type.rs | 25 +++++++-- src/ua/trust_list_masks.rs | 67 +++++++++++++++++++++++ 3 files changed, 89 insertions(+), 5 deletions(-) create mode 100644 src/ua/trust_list_masks.rs diff --git a/src/ua.rs b/src/ua.rs index c5d61fc8..5f5003c5 100644 --- a/src/ua.rs +++ b/src/ua.rs @@ -31,6 +31,7 @@ mod session_state; mod session_statistics; mod specified_attributes; mod subscription_id; +mod trust_list_masks; mod unit_id; mod user_identity_token; @@ -64,6 +65,7 @@ pub use self::{ session_statistics::SessionStatistics, specified_attributes::SpecifiedAttributes, subscription_id::SubscriptionId, + trust_list_masks::TrustListMasks, unit_id::UnitId, user_identity_token::UserIdentityToken, }; diff --git a/src/ua/data_types/trust_list_data_type.rs b/src/ua/data_types/trust_list_data_type.rs index f9412aa8..823c8c81 100644 --- a/src/ua/data_types/trust_list_data_type.rs +++ b/src/ua/data_types/trust_list_data_type.rs @@ -6,39 +6,54 @@ impl TrustListDataType { #[must_use] pub fn with_trusted_certificates(mut self, trusted_certificates: &[ua::String]) -> Self { let array = ua::Array::from_slice(trusted_certificates); + if array.is_empty() { + self.0.specifiedLists &= !ua::TrustListMasks::TRUSTEDCERTIFICATES.as_u32(); + } else { + self.0.specifiedLists |= ua::TrustListMasks::TRUSTEDCERTIFICATES.as_u32(); + } array.move_into_raw( &mut self.0.trustedCertificatesSize, &mut self.0.trustedCertificates, ); - // FIXME: How to set masks? - // self.0.specifiedLists |= UA_TRUSTLISTMASKS_TRUSTEDCERTIFICATES; self } #[must_use] pub fn with_trusted_crls(mut self, trusted_crls: &[ua::String]) -> Self { let array = ua::Array::from_slice(trusted_crls); + if array.is_empty() { + self.0.specifiedLists &= !ua::TrustListMasks::TRUSTEDCRLS.as_u32(); + } else { + self.0.specifiedLists |= ua::TrustListMasks::TRUSTEDCRLS.as_u32(); + } array.move_into_raw(&mut self.0.trustedCrlsSize, &mut self.0.trustedCrls); - // FIXME: How to set masks? self } #[must_use] pub fn with_issuer_certificates(mut self, issuer_certificates: &[ua::String]) -> Self { let array = ua::Array::from_slice(issuer_certificates); + if array.is_empty() { + self.0.specifiedLists &= !ua::TrustListMasks::ISSUERCERTIFICATES.as_u32(); + } else { + self.0.specifiedLists |= ua::TrustListMasks::ISSUERCERTIFICATES.as_u32(); + } array.move_into_raw( &mut self.0.issuerCertificatesSize, &mut self.0.issuerCertificates, ); - // FIXME: How to set masks? self } #[must_use] pub fn with_issuer_crls(mut self, issuer_crls: &[ua::String]) -> Self { let array = ua::Array::from_slice(issuer_crls); + if array.is_empty() { + self.0.specifiedLists &= !ua::TrustListMasks::ISSUERCRLS.as_u32(); + } else { + self.0.specifiedLists |= ua::TrustListMasks::ISSUERCRLS.as_u32(); + } array.move_into_raw(&mut self.0.issuerCrlsSize, &mut self.0.issuerCrls); - // FIXME: How to set masks? self } } diff --git a/src/ua/trust_list_masks.rs b/src/ua/trust_list_masks.rs new file mode 100644 index 00000000..adf4e65d --- /dev/null +++ b/src/ua/trust_list_masks.rs @@ -0,0 +1,67 @@ +#[expect(unreachable_pub, reason = "hidden inner enum")] +// We do not expose the inner enum. We want to use a proper `u32` for bit operations on the mask and +// we want to be clear about what is an initial (const, enum-like) value and what is a derived mask; +// specifically, the bitmask type is _not_ an enum even though declared so in `open62541-sys`. +mod inner { + crate::data_type!(TrustListMasks); + + crate::enum_variants!( + TrustListMasks, + UA_TrustListMasks, + [ + NONE, + TRUSTEDCERTIFICATES, + TRUSTEDCRLS, + ISSUERCERTIFICATES, + ISSUERCRLS, + ALL, + ], + ); +} + +/// Wrapper for trust list masks from [`open62541_sys`]. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct TrustListMasks(u32); + +crate::bitmask_ops!(TrustListMasks); + +impl TrustListMasks { + pub const NONE: Self = Self(inner::TrustListMasks::NONE_U32); + pub const TRUSTEDCERTIFICATES: Self = Self(inner::TrustListMasks::TRUSTEDCERTIFICATES_U32); + pub const TRUSTEDCRLS: Self = Self(inner::TrustListMasks::TRUSTEDCRLS_U32); + pub const ISSUERCERTIFICATES: Self = Self(inner::TrustListMasks::ISSUERCERTIFICATES_U32); + pub const ISSUERCRLS: Self = Self(inner::TrustListMasks::ISSUERCRLS_U32); + pub const ALL: Self = Self(inner::TrustListMasks::ALL_U32); + + pub(crate) const fn from_u32(mask: u32) -> Self { + Self(mask) + } + + pub(crate) const fn as_u32(&self) -> u32 { + self.0 + } +} + +#[cfg(test)] +mod tests { + use crate::ua; + + #[test] + fn combine_masks() { + // We support mask combinators in `const` expressions. + const LHS: ua::TrustListMasks = ua::TrustListMasks::TRUSTEDCERTIFICATES + .or(&ua::TrustListMasks::TRUSTEDCRLS) + .or(&ua::TrustListMasks::ISSUERCERTIFICATES) + .or(&ua::TrustListMasks::ISSUERCRLS); + const RHS: ua::TrustListMasks = ua::TrustListMasks::ALL; + assert_eq!(LHS, RHS); + + // We support mask combinators with `|` shorthand notation. + let lhs = ua::TrustListMasks::TRUSTEDCERTIFICATES + | ua::TrustListMasks::TRUSTEDCRLS + | ua::TrustListMasks::ISSUERCERTIFICATES + | ua::TrustListMasks::ISSUERCRLS; + let rhs = ua::TrustListMasks::ALL; + assert_eq!(lhs, rhs); + } +}