From 33dbd342e5afa770479a8e5054f59d4a675bff3a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 16:49:19 -0400 Subject: [PATCH] fix(ab-transport): trim-only CIP path canonicalisation, observe abandoned shared opens, no Forward Open after dispose during connect --- .../Cip/CipConnection.cs | 15 ++++++++++++++- .../Cip/CipPath.cs | 4 ++++ .../Cip/DeviceKey.cs | 10 ++++++---- .../Cip/ForwardOpenThrottle.cs | 9 ++++++--- .../DeviceSessionPoolTests.cs | 11 +++++++++++ 5 files changed, 41 insertions(+), 8 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipConnection.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipConnection.cs index a9754706..d9508a81 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipConnection.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipConnection.cs @@ -123,7 +123,14 @@ public sealed class CipConnection : ICipChannel, IAsyncDisposable } pending = _pendingOpen; } - if (started is not null) _ = RunOpenAsync(started); + if (started is not null) + { + // Every waiter may have abandoned the open (its own token); observe a failure so it never + // surfaces as UnobservedTaskException. + _ = started.Task.ContinueWith(static t => _ = t.Exception, CancellationToken.None, + TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); + _ = RunOpenAsync(started); + } return pending.WaitAsync(ct); } @@ -170,6 +177,12 @@ public sealed class CipConnection : ICipChannel, IAsyncDisposable { ThrowIfDisposed(); // disposal may have begun while this attempt waited out the throttle var client = await EncapsulationClient.ConnectAsync(_key.Host, _key.Port, _o.Timeout, ct).ConfigureAwait(false); + if (Volatile.Read(ref _disposed) != 0) + { + // Disposal began during the TCP connect / RegisterSession: never put a Forward Open on the wire. + await client.DisposeAsync().ConfigureAwait(false); + throw new AbTransportException(AbStatus.Abort, "Connection disposed."); + } if (_o.Routing != CipRouting.Connected) return new Open(client, null, 0, UnconnectedMessageLimit); try { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipPath.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipPath.cs index e653a932..058176aa 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipPath.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipPath.cs @@ -17,6 +17,10 @@ public static class CipPath { var port = int.Parse(parts[i], CultureInfo.InvariantCulture); var link = parts[i + 1]; + // A link is a slot byte or an IP address/hostname; neither contains whitespace. Without this a + // typo like "1,2 3" would be sent as an ASCII link "2 3" instead of failing here. + if (link.Length == 0 || link.Any(char.IsWhiteSpace)) + throw new ArgumentException($"CIP path '{cipPath}' has an empty or malformed link '{link}'.", nameof(cipPath)); var isByte = byte.TryParse(link, NumberStyles.None, CultureInfo.InvariantCulture, out var linkByte); if (port < 15) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/DeviceKey.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/DeviceKey.cs index 9756d613..c8331d21 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/DeviceKey.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/DeviceKey.cs @@ -3,14 +3,16 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Cip; /// /// Identity of one physical CIP connection: exactly one (and one Forward Open /// throttle) exists per key (press safety). and compare -/// case-insensitively; compares exactly; is canonicalised (all -/// whitespace removed, so "1,0" and " 1, 0 " are the same device) and then compares exactly. +/// case-insensitively; compares exactly; is canonicalised (each +/// comma-separated part trimmed, so "1,0" and " 1, 0 " are the same device) and then +/// compares exactly. Whitespace INSIDE a part is kept, so a typo such as "1,2 3" still fails loudly in +/// instead of silently becoming slot 23. /// public sealed record DeviceKey(string Host, int Port, string CipPath, string PlcAttribute) { private readonly string _cipPath = Canonicalise(CipPath); - /// The route, whitespace-free. Canonicalised on construction and on with. + /// The route with each part trimmed. Canonicalised on construction and on with. public string CipPath { get => _cipPath; @@ -18,7 +20,7 @@ public sealed record DeviceKey(string Host, int Port, string CipPath, string Plc } private static string Canonicalise(string? path) => - path is null ? "" : string.Concat(path.Where(c => !char.IsWhiteSpace(c))); + string.IsNullOrWhiteSpace(path) ? "" : string.Join(',', path.Split(',').Select(p => p.Trim())); public bool Equals(DeviceKey? other) => other is not null diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ForwardOpenThrottle.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ForwardOpenThrottle.cs index 778f1995..b315aa06 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ForwardOpenThrottle.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ForwardOpenThrottle.cs @@ -24,9 +24,12 @@ public sealed class ForwardOpenThrottle _time = time ?? throw new ArgumentNullException(nameof(time)); } - /// Run one open attempt once the throttle allows it. Throws - /// () when - /// fires before the attempt could start; the attempt itself sees too. + /// Run one open attempt once the throttle allows it. bounds only the wait + /// BEFORE the attempt (queued behind an in-flight attempt, or the interval): when it fires there, this + /// throws (, "Forward Open + /// throttled"). is also passed to , which may ignore it: + /// does, running the attempt under its own open deadline so that an accepted + /// Forward Open is never abandoned without a Forward Close. public async Task RunAsync(Func> open, CancellationToken ct) { ArgumentNullException.ThrowIfNull(open); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/DeviceSessionPoolTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/DeviceSessionPoolTests.cs index d9cdb478..bf14ebf9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/DeviceSessionPoolTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/DeviceSessionPoolTests.cs @@ -162,6 +162,7 @@ public sealed class DeviceSessionPoolTests b.ShouldBe(a); b.GetHashCode().ShouldBe(a.GetHashCode()); (a with { CipPath = "1, 2" }).CipPath.ShouldBe("1,2"); + new DeviceKey("plc1", 44818, " ", "ControlLogix").CipPath.ShouldBe(""); var pool = new DeviceSessionPool(new ManualTimeProvider()); await using var la = pool.Acquire(a, Options()); @@ -170,6 +171,16 @@ public sealed class DeviceSessionPoolTests pool.EntryCount.ShouldBe(1); } + [Fact] + public void Whitespace_inside_a_cip_path_part_is_kept_so_a_typo_fails_loudly() + { + var typo = new DeviceKey("plc1", 44818, "1,2 3", "ControlLogix"); + typo.CipPath.ShouldNotBe("1,23"); // never silently slot 23 on another controller + typo.CipPath.ShouldBe("1,2 3"); + Should.Throw(() => CipPath.EncodeRoute(typo.CipPath)); + CipPath.EncodeRoute(new DeviceKey("plc1", 44818, " 1, 0 ", "ControlLogix").CipPath).ShouldBe(new byte[] { 0x01, 0x00 }); + } + [Fact] public async Task A_new_connection_waits_for_every_earlier_close_not_just_the_latest() {