From d4a618bdfca63781fceca9813a7d166ee282c811 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 10 Jun 2026 20:59:01 +0000 Subject: [PATCH] Harden OPC UA layer: reconnect races, handle allocation, interval/security wiring, status semantics - ReconnectAsync now works against a locally captured CancellationTokenSource; the field swap in DisposeReconnectCts happens under a lock, closing the TOCTOU where a concurrent Disconnect could double-dispose or NRE, and a session recreated after the user disconnected is closed instead of living on as a ghost connection. - AddNodeAsync allocates the client handle inside the dedup lock; the unsynchronized increment let two concurrent adds collide on one handle and silently overwrite each other's dictionary entries. - The last-resort subscription restore path now raises VariableRemoved for every lost node so the UI no longer shows rows stuck at (reconnecting...) with dangling handles. - settings.samplingIntervalMs is finally honored: SubscriptionManager gains a SamplingInterval property used for monitored items, ConnectionManager stores both intervals from ConnectAsync and reuses them when subscriptions are restored after reconnect (previously restores fell back to 250 ms). - Config securityMode/securityPolicy and samplingIntervalMs are passed from MainWindow.LoadConfigurationAsync to ConnectAsync, closing the declared loose end from PR #160; a warning is logged when credentials are about to be sent over a SecurityMode=None endpoint. - MonitoredNode treats Good-with-info-bits status codes (e.g. GoodClamped) as Good: severity is the top two bits, not == 0. https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie --- App/MainWindow.cs | 10 ++- OpcUa/ConnectionManager.cs | 31 +++++++-- OpcUa/Models/MonitoredNode.cs | 13 ++-- OpcUa/OpcUaClientWrapper.cs | 65 ++++++++++++++----- OpcUa/SubscriptionManager.cs | 21 +++++- .../OpcUa/Models/MonitoredNodeTests.cs | 32 +++++++++ .../OpcUa/SubscriptionManagerTests.cs | 17 +++++ 7 files changed, 160 insertions(+), 29 deletions(-) diff --git a/App/MainWindow.cs b/App/MainWindow.cs index 9519285..d363a3e 100644 --- a/App/MainWindow.cs +++ b/App/MainWindow.cs @@ -1251,7 +1251,15 @@ private async Task LoadConfigurationAsync(string filePath) // old server while (or after) the new connection is attempted. await DisconnectAsync(); - var connected = await _connectionManager.ConnectAsync(config.Server.EndpointUrl, config.Settings.PublishingIntervalMs, credentials); + // Honor the config's security and sampling settings (the connect dialog has + // no UI for these, so the config file is their only source). + var connected = await _connectionManager.ConnectAsync( + config.Server.EndpointUrl, + config.Settings.PublishingIntervalMs, + credentials, + config.Server.SecurityMode, + config.Server.SecurityPolicy, + config.Settings.SamplingIntervalMs); if (connected) { diff --git a/OpcUa/ConnectionManager.cs b/OpcUa/ConnectionManager.cs index b958538..2dd5871 100644 --- a/OpcUa/ConnectionManager.cs +++ b/OpcUa/ConnectionManager.cs @@ -17,6 +17,11 @@ public sealed class ConnectionManager : IDisposable private bool _disposed; private int _isReconnecting; + // Intervals from the most recent ConnectAsync, so subscription restoration + // after a reconnect does not silently fall back to the defaults. + private int _publishingInterval = 250; + private int _samplingInterval = 250; + // Stored event handler references for proper unsubscription private Action? _valueChangedHandler; private Action? _variableAddedHandler; @@ -115,13 +120,15 @@ public ConnectionManager(Logger logger, bool? allowInsecure = null) /// Authentication credentials (defaults to anonymous). /// Requested message security mode (e.g. None, Sign, SignAndEncrypt). When null/None, an unsecured endpoint is selected. /// Requested security policy URI or shorthand (e.g. Basic256Sha256). Honored when a matching endpoint exists. + /// Sampling interval in milliseconds for monitored items. /// True if connection succeeded, false otherwise. public async Task ConnectAsync( string endpoint, int publishingInterval = 250, ConnectionCredentials? credentials = null, string? securityMode = null, - string? securityPolicy = null) + string? securityPolicy = null, + int samplingInterval = 250) { // Async teardown: the synchronous Disconnect() blocks on the OPC UA close // round-trip (up to the transport timeout against a dead server), which froze @@ -130,6 +137,8 @@ public async Task ConnectAsync( _lastEndpoint = endpoint; _credentials = credentials ?? ConnectionCredentials.Anonymous; + _publishingInterval = publishingInterval; + _samplingInterval = samplingInterval; StateChanged?.Invoke(ConnectionState.Connecting); try @@ -138,7 +147,7 @@ public async Task ConnectAsync( if (success) { - if (!await InitializeSubscriptionAsync(publishingInterval)) + if (!await InitializeSubscriptionAsync()) { // Without a subscription the session is useless for monitoring; // fail the connect rather than reporting Connected. @@ -271,9 +280,20 @@ private async Task RestoreSubscriptionsAsync() if (!recreated) { _logger.Warning("Failed to recreate subscriptions - initializing fresh"); - // Last resort: start fresh (will lose monitored nodes) + // Last resort: start fresh. The monitored nodes are gone, so tell the UI - + // otherwise their rows sit at "(reconnecting...)" forever with handles the + // new subscription manager knows nothing about. + var lostHandles = _subscriptionManager.MonitoredVariables + .Select(v => v.ClientHandle) + .ToList(); + DisposeSubscription(); await InitializeSubscriptionAsync(); + + foreach (var handle in lostHandles) + { + VariableRemoved?.Invoke(handle); + } } } @@ -313,10 +333,11 @@ public Task UnsubscribeAsync(uint clientHandle) return _client.WriteValueAsync(nodeId, value); } - private async Task InitializeSubscriptionAsync(int publishingInterval = 250) + private async Task InitializeSubscriptionAsync() { _subscriptionManager = new SubscriptionManager(_client, _logger); - _subscriptionManager.PublishingInterval = publishingInterval; + _subscriptionManager.PublishingInterval = _publishingInterval; + _subscriptionManager.SamplingInterval = _samplingInterval; var initialized = await _subscriptionManager.InitializeAsync(); // Store handler references for proper unsubscription diff --git a/OpcUa/Models/MonitoredNode.cs b/OpcUa/Models/MonitoredNode.cs index d468c0d..4832a9c 100644 --- a/OpcUa/Models/MonitoredNode.cs +++ b/OpcUa/Models/MonitoredNode.cs @@ -24,8 +24,11 @@ public class MonitoredNode public DateTime? Timestamp { get; set; } public uint StatusCode { get; set; } - public bool IsGood => StatusCode == 0; // StatusCode.Good = 0 - public bool IsUncertain => (StatusCode & 0x40000000) != 0; + // OPC UA status severity lives in the top two bits: 00 = Good, 01 = Uncertain, + // 10 = Bad. Good codes with info bits set (e.g. GoodClamped 0x00300000) are + // still Good, so testing for == 0 would misclassify them. + public bool IsGood => (StatusCode & 0xC0000000) == 0; + public bool IsUncertain => (StatusCode & 0xC0000000) == 0x40000000; public bool IsBad => (StatusCode & 0x80000000) != 0; public DateTime LastChangeTime { get; set; } = DateTime.MinValue; public bool RecentlyChanged => (DateTime.Now - LastChangeTime).TotalMilliseconds < 500; @@ -72,9 +75,9 @@ public string StatusString get { if (StatusCode == 0) return "Good"; - if ((StatusCode & 0x80000000) != 0) return $"Bad (0x{StatusCode:X8})"; - if ((StatusCode & 0x40000000) != 0) return $"Uncertain (0x{StatusCode:X8})"; - return $"0x{StatusCode:X8}"; + if (IsBad) return $"Bad (0x{StatusCode:X8})"; + if (IsUncertain) return $"Uncertain (0x{StatusCode:X8})"; + return $"Good (0x{StatusCode:X8})"; } } diff --git a/OpcUa/OpcUaClientWrapper.cs b/OpcUa/OpcUaClientWrapper.cs index a68c12c..af77369 100644 --- a/OpcUa/OpcUaClientWrapper.cs +++ b/OpcUa/OpcUaClientWrapper.cs @@ -15,6 +15,7 @@ public class OpcUaClientWrapper : IDisposable private readonly Logger _logger; private string? _currentEndpoint; private CancellationTokenSource? _reconnectCts; + private readonly object _reconnectCtsLock = new(); private bool _disposed; private ApplicationConfiguration? _appConfig; private ConfiguredEndpoint? _lastConfiguredEndpoint; @@ -287,15 +288,23 @@ public void Disconnect() /// /// Cancels and disposes the reconnect cancellation token source, if any. + /// Safe to race with : the field swap happens + /// under a lock, so two callers can never dispose the same instance twice. /// private void DisposeReconnectCts() { - if (_reconnectCts != null) + CancellationTokenSource? cts; + lock (_reconnectCtsLock) { - try { _reconnectCts.Cancel(); } catch (ObjectDisposedException) { } - _reconnectCts.Dispose(); + cts = _reconnectCts; _reconnectCts = null; } + + if (cts != null) + { + try { cts.Cancel(); } catch (ObjectDisposedException) { } + cts.Dispose(); + } } /// @@ -313,14 +322,23 @@ public async Task ReconnectAsync() } DisposeReconnectCts(); - _reconnectCts = new CancellationTokenSource(); + + // Work with a local reference throughout: a concurrent Disconnect() can null + // and dispose the field at any time, so re-reading it mid-loop would NRE or + // touch a disposed CTS. + var cts = new CancellationTokenSource(); + lock (_reconnectCtsLock) + { + _reconnectCts = cts; + } + var token = cts.Token; // Exponential backoff: 1s, 2s, 4s, 8s int[] delays = { 1000, 2000, 4000, 8000 }; for (int attempt = 0; attempt < delays.Length; attempt++) { - if (_reconnectCts.Token.IsCancellationRequested) + if (token.IsCancellationRequested) return false; _logger.Info($"Reconnection attempt {attempt + 1}/{delays.Length}..."); @@ -330,8 +348,8 @@ public async Task ReconnectAsync() // Strategy 1: Try to reconnect the existing session (preserves subscriptions automatically) if (_session != null) { - var reconnectResult = await TrySessionReconnectAsync(); - if (reconnectResult) + var reconnectResult = await TrySessionReconnectAsync(token); + if (reconnectResult && !token.IsCancellationRequested) { _logger.Info("Session reconnected successfully (subscriptions preserved)"); Connected?.Invoke(); @@ -340,9 +358,18 @@ public async Task ReconnectAsync() } // Strategy 2: Recreate session and transfer subscriptions - var recreateResult = await TryRecreateSessionAsync(); + var recreateResult = await TryRecreateSessionAsync(token); if (recreateResult) { + if (token.IsCancellationRequested) + { + // The user disconnected while the session was being recreated; + // don't resurrect a connection they asked to close. + _logger.Info("Reconnect cancelled after session recreation - closing the new session"); + await DisconnectAsync(); + return false; + } + _logger.Info("Session recreated successfully (subscriptions transferred)"); Connected?.Invoke(); return true; @@ -356,7 +383,7 @@ public async Task ReconnectAsync() // Wait before next attempt try { - await Task.Delay(delays[attempt], _reconnectCts.Token); + await Task.Delay(delays[attempt], token); } catch (OperationCanceledException) { @@ -373,7 +400,7 @@ public async Task ReconnectAsync() /// Tries to reconnect the existing session using OPC UA Reconnect service. /// This preserves the session ID and all subscriptions automatically. /// - private async Task TrySessionReconnectAsync() + private async Task TrySessionReconnectAsync(CancellationToken cancellationToken) { if (_session == null) return false; @@ -381,7 +408,7 @@ private async Task TrySessionReconnectAsync() try { _logger.Info("Attempting session reconnect..."); - await _session.ReconnectAsync(_reconnectCts?.Token ?? CancellationToken.None); + await _session.ReconnectAsync(cancellationToken); return _session.Connected; } catch (ServiceResultException ex) @@ -395,7 +422,7 @@ private async Task TrySessionReconnectAsync() /// Recreates the session and transfers existing subscriptions to it. /// Used when direct reconnect fails (e.g., session timed out on server). /// - private async Task TryRecreateSessionAsync() + private async Task TryRecreateSessionAsync(CancellationToken cancellationToken) { if (_lastConfiguredEndpoint == null || string.IsNullOrEmpty(_currentEndpoint)) return false; @@ -450,7 +477,7 @@ private async Task TryRecreateSessionAsync() { if (subscriptionsToTransfer != null && subscriptionsToTransfer.Count > 0) { - var transferred = await TransferSubscriptionsAsync(subscriptionsToTransfer); + var transferred = await TransferSubscriptionsAsync(subscriptionsToTransfer, cancellationToken); _logger.Info($"Transferred {transferred} of {subscriptionsToTransfer.Count} subscription(s)"); } } @@ -487,7 +514,7 @@ private async Task TryRecreateSessionAsync() /// /// Transfers subscriptions from a previous session to the current session. /// - private async Task TransferSubscriptionsAsync(SubscriptionCollection subscriptions) + private async Task TransferSubscriptionsAsync(SubscriptionCollection subscriptions, CancellationToken cancellationToken) { if (_session == null || subscriptions == null) return 0; @@ -500,7 +527,7 @@ private async Task TransferSubscriptionsAsync(SubscriptionCollection subscr var success = await _session.TransferSubscriptionsAsync( subscriptions, sendInitialValues: true, - _reconnectCts?.Token ?? CancellationToken.None); + cancellationToken); if (success) { @@ -743,6 +770,14 @@ private async Task DiscoverAndSelectEndpointAsync( _logger.Info($"Selected endpoint: {selectedEndpoint.SecurityMode} / {selectedEndpoint.SecurityPolicyUri}"); + if (_credentials.Type != AuthenticationType.Anonymous + && selectedEndpoint.SecurityMode == MessageSecurityMode.None) + { + _logger.Warning( + "Credentials will be sent over an UNENCRYPTED channel: the selected endpoint uses SecurityMode=None. " + + "Anyone on the network can read the username and password. Prefer a server endpoint with Sign or SignAndEncrypt."); + } + // Update the endpoint URL to use the requested host if different // (handles cases where server returns localhost but we connected via IP/hostname) var selectedUri = new Uri(selectedEndpoint.EndpointUrl); diff --git a/OpcUa/SubscriptionManager.cs b/OpcUa/SubscriptionManager.cs index b35534a..e39b0bf 100644 --- a/OpcUa/SubscriptionManager.cs +++ b/OpcUa/SubscriptionManager.cs @@ -21,6 +21,7 @@ public class SubscriptionManager : IDisposable, IAsyncDisposable private readonly Dictionary _opcHandleToClientHandle = new(); private uint _nextClientHandle = 1; private int _publishingInterval = 250; + private int _samplingInterval = 250; private bool _isInitialized; private readonly object _lock = new(); @@ -45,6 +46,16 @@ public int PublishingInterval set => _publishingInterval = Math.Max(100, Math.Min(10000, value)); } + /// + /// Sampling interval in milliseconds applied to monitored items. + /// 0 requests the server's fastest practical rate. + /// + public int SamplingInterval + { + get => _samplingInterval; + set => _samplingInterval = Math.Max(0, Math.Min(60000, value)); + } + public IReadOnlyCollection MonitoredVariables { get @@ -107,6 +118,7 @@ public async Task InitializeAsync() return null; } + uint clientHandle; lock (_lock) { // Check if already monitoring this node @@ -115,11 +127,14 @@ public async Task InitializeAsync() _logger.Warning($"Node {displayName} is already being monitored"); return null; } + + // Allocate the handle under the lock: it keys three dictionaries, and an + // unsynchronized increment lets two concurrent adds collide on one handle. + clientHandle = _nextClientHandle++; } try { - var clientHandle = _nextClientHandle++; // Create OPC UA monitored item var monitoredItem = new MonitoredItem(_subscription.DefaultItem) @@ -127,7 +142,7 @@ public async Task InitializeAsync() DisplayName = displayName, StartNodeId = nodeId, AttributeId = Attributes.Value, - SamplingInterval = 250, + SamplingInterval = _samplingInterval, QueueSize = 10, DiscardOldest = true }; @@ -589,7 +604,7 @@ public async Task RecreateSubscriptionsAsync() DisplayName = displayName, StartNodeId = nodeId, AttributeId = Attributes.Value, - SamplingInterval = 250, + SamplingInterval = _samplingInterval, QueueSize = 10, DiscardOldest = true }; diff --git a/Tests/Opcilloscope.Tests/OpcUa/Models/MonitoredNodeTests.cs b/Tests/Opcilloscope.Tests/OpcUa/Models/MonitoredNodeTests.cs index 1a549c4..1df43ca 100644 --- a/Tests/Opcilloscope.Tests/OpcUa/Models/MonitoredNodeTests.cs +++ b/Tests/Opcilloscope.Tests/OpcUa/Models/MonitoredNodeTests.cs @@ -22,6 +22,38 @@ public void MonitoredNode_IsGood_ReturnsTrueWhenStatusCodeIsZero() Assert.False(node.IsBad); } + [Fact] + public void MonitoredNode_IsGood_ReturnsTrueForGoodWithInfoBits() + { + // GoodClamped (0x00300000): Good severity with informational sub-status bits. + var node = new MonitoredNode + { + NodeId = new NodeId(1000), + DisplayName = "Test", + StatusCode = 0x00300000 + }; + + // Assert + Assert.True(node.IsGood); + Assert.False(node.IsUncertain); + Assert.False(node.IsBad); + } + + [Fact] + public void MonitoredNode_StatusString_ReturnsGoodWithCodeForGoodWithInfoBits() + { + var node = new MonitoredNode + { + NodeId = new NodeId(1000), + DisplayName = "Test", + StatusCode = 0x00300000 // GoodClamped + }; + + // Assert + Assert.StartsWith("Good", node.StatusString); + Assert.Contains("0x00300000", node.StatusString); + } + [Fact] public void MonitoredNode_IsBad_ReturnsTrueWhenBadBitSet() { diff --git a/Tests/Opcilloscope.Tests/OpcUa/SubscriptionManagerTests.cs b/Tests/Opcilloscope.Tests/OpcUa/SubscriptionManagerTests.cs index a6e2e59..6dd27f8 100644 --- a/Tests/Opcilloscope.Tests/OpcUa/SubscriptionManagerTests.cs +++ b/Tests/Opcilloscope.Tests/OpcUa/SubscriptionManagerTests.cs @@ -6,6 +6,23 @@ namespace Opcilloscope.Tests.OpcUa; public class SubscriptionManagerTests { + [Theory] + [InlineData(0, 0)] + [InlineData(250, 250)] + [InlineData(5000, 5000)] + [InlineData(-1, 0)] + [InlineData(100000, 60000)] + public void SamplingInterval_ClampsToValidRange(int requested, int expected) + { + var manager = new SubscriptionManager( + new global::Opcilloscope.OpcUa.OpcUaClientWrapper(), + new global::Opcilloscope.Utilities.Logger()); + + manager.SamplingInterval = requested; + + Assert.Equal(expected, manager.SamplingInterval); + } + [Fact] public void FormatValue_ReturnsNull_WhenValueIsNull() {