From 7892eb13a8eb182e5b3b1ea797d0a03e277a0743 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 22:54:32 -0400 Subject: [PATCH] fix(abcip): harden Ab.Transport CIP codec and EncapsulationClient (#564, #565) CIP codec (#564): PatchU16 bounds guard; CipReply content equality; CipPath port/length/ASCII/odd/oversize validation with ArgumentException for malformed tag names and routes; CipPath.MessageRouter/ConnectionManager are now ReadOnlySpan; refused Forward Open surfaces ForwardOpenRefusedException with structured general/extended status; test CaptureFile uses BinaryPrimitives. EncapsulationClient (#565): frame write bounded by the exchange timeout, which faults (never cancels) the connection; finite positive timeouts enforced at every entry point; gate wait bounded by the timeout; deterministic reorder of the 300 ms test; new dispose-during-drain, gate-wait and RegisterSession zero-context tests. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- .../AbTransportException.cs | 2 +- .../Cip/CipPath.cs | 64 +++++++++-- .../Cip/CipReply.cs | 19 +++ .../Cip/ConnectionManagerServices.cs | 22 +++- .../Eip/EncapsulationClient.cs | 36 +++++- .../Wire/CipWriter.cs | 9 +- .../AbStatusCodesTests.cs | 4 + .../Captures/CaptureFile.cs | 9 +- .../Captures/CaptureFileTests.cs | 9 +- .../Cip/CipPathTests.cs | 59 ++++++++++ .../Cip/ConnectionManagerServicesTests.cs | 19 +++ .../Eip/EncapsulationClientTests.cs | 108 +++++++++++++++++- .../Eip/EncapsulationCodecTests.cs | 3 +- .../Wire/CipReplyTests.cs | 19 +++ .../Wire/CipWriterTests.cs | 27 +++++ 15 files changed, 379 insertions(+), 30 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbTransportException.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbTransportException.cs index baa6694e..2affce8c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbTransportException.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/AbTransportException.cs @@ -2,7 +2,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; /// Thrown by both AB transports. drives OPC UA status mapping and the /// driver's "controller answered, don't throttle the device" classification (05/STAB-8). -public sealed class AbTransportException(AbStatus status, string message, Exception? inner = null) +public class AbTransportException(AbStatus status, string message, Exception? inner = null) : Exception(message, inner) { public AbStatus Status { get; } = status; 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 058176aa..5276c9d4 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 @@ -15,24 +15,34 @@ public static class CipPath if (parts.Length % 2 != 0) throw new ArgumentException($"CIP path '{cipPath}' must be port,link pairs.", nameof(cipPath)); for (var i = 0; i < parts.Length; i += 2) { - var port = int.Parse(parts[i], CultureInfo.InvariantCulture); + if (!ushort.TryParse(parts[i], NumberStyles.None, CultureInfo.InvariantCulture, out var port)) + throw new ArgumentException($"CIP path '{cipPath}' has a port '{parts[i]}' that is not 0..65535.", nameof(cipPath)); 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); + byte[] a = []; + if (!isByte) + { + // The link-address length rides in one byte, and a non-ASCII char would be sent as '?'. + a = EncodeAscii(link, cipPath, nameof(cipPath), "link"); + } if (port < 15) { if (isByte) w.U8((byte)port).U8(linkByte); - else { var a = Encoding.ASCII.GetBytes(link); w.U8((byte)(0x10 | port)).U8((byte)a.Length).Bytes(a).PadToEven(); } + else w.U8((byte)(0x10 | port)).U8((byte)a.Length).Bytes(a).PadToEven(); } else { - if (isByte) w.U8(0x0F).U16((ushort)port).U8(linkByte); - else { var a = Encoding.ASCII.GetBytes(link); w.U8(0x1F).U8((byte)a.Length).U16((ushort)port).Bytes(a).PadToEven(); } + if (isByte) w.U8(0x0F).U16(port).U8(linkByte); + else w.U8(0x1F).U8((byte)a.Length).U16(port).Bytes(a).PadToEven(); } } + // The route size is carried in words in one byte (Unconnected Send / Forward Open paths). + if (w.Length > MaxPathBytes) + throw new ArgumentException($"CIP path '{cipPath}' encodes to {w.Length} bytes; the maximum is {MaxPathBytes}.", nameof(cipPath)); return w.ToArray(); } @@ -41,12 +51,20 @@ public static class CipPath { var dot = tagName.LastIndexOf('.'); if (dot > 0 && dot < tagName.Length - 1 && tagName[(dot + 1)..].All(char.IsAsciiDigit)) - return (tagName[..dot], int.Parse(tagName[(dot + 1)..], CultureInfo.InvariantCulture)); + { + if (!int.TryParse(tagName[(dot + 1)..], NumberStyles.None, CultureInfo.InvariantCulture, out var bit)) + throw new ArgumentException($"Bit selector in '{tagName}' is out of range.", nameof(tagName)); + return (tagName[..dot], bit); + } return (tagName, null); } + /// The name is empty, has an empty member, an over-long (> 255) or + /// non-ASCII member, an unterminated / non-numeric subscript, or ends in a bit selector. public static byte[] EncodeSymbolic(string tagName) { + if (string.IsNullOrEmpty(tagName)) + throw new ArgumentException("Tag name is empty.", nameof(tagName)); if (SplitBitSelector(tagName).Bit is not null) throw new ArgumentException($"'{tagName}' ends in a bit selector; strip it with SplitBitSelector first.", nameof(tagName)); var w = new CipWriter(); @@ -54,13 +72,19 @@ public static class CipPath { var bracket = part.IndexOf('['); var name = bracket < 0 ? part : part[..bracket]; - var a = Encoding.ASCII.GetBytes(name); + if (name.Length == 0) + throw new ArgumentException($"Tag name '{tagName}' has an empty member.", nameof(tagName)); + var a = EncodeAscii(name, tagName, nameof(tagName), "member"); w.U8(0x91).U8((byte)a.Length).Bytes(a).PadToEven(); if (bracket >= 0) { - foreach (var idxText in part[(bracket + 1)..part.IndexOf(']')].Split(',', StringSplitOptions.TrimEntries)) + var close = part.IndexOf(']', bracket); + if (close != part.Length - 1) + throw new ArgumentException($"Tag name '{tagName}' has an unterminated or misplaced subscript in '{part}'.", nameof(tagName)); + foreach (var idxText in part[(bracket + 1)..close].Split(',', StringSplitOptions.TrimEntries)) { - var idx = uint.Parse(idxText, CultureInfo.InvariantCulture); + if (!uint.TryParse(idxText, NumberStyles.None, CultureInfo.InvariantCulture, out var idx)) + throw new ArgumentException($"Tag name '{tagName}' has a subscript '{idxText}' that is not an unsigned integer.", nameof(tagName)); if (idx < 0x100) w.U8(0x28).U8((byte)idx); else if (idx < 0x10000) w.U8(0x29).U8(0).U16((ushort)idx); else w.U8(0x2A).U8(0).U32(idx); @@ -70,6 +94,17 @@ public static class CipPath return w.ToArray(); } + /// ASCII bytes of a name / link whose length rides in a single byte. + private static byte[] EncodeAscii(string text, string whole, string paramName, string what) + { + if (text.Length > byte.MaxValue) + throw new ArgumentException($"'{whole}' has a {what} longer than 255 characters.", paramName); + foreach (var c in text) + if (!char.IsAscii(c)) + throw new ArgumentException($"'{whole}' has a {what} with a non-ASCII character.", paramName); + return Encoding.ASCII.GetBytes(text); + } + /// Split on '.' outside brackets, keeping "Program:X" whole. private static IEnumerable SplitMembers(string name) { @@ -94,16 +129,25 @@ public static class CipPath } /// Prefixes the path with its size in 16-bit words. + /// The segments are not a whole number of 16-bit words, or exceed + /// the 255-word maximum the one-byte size field can carry. public static byte[] WithPathSize(byte[] segments) { + if ((segments.Length & 1) != 0) + throw new ArgumentException($"A CIP path is whole 16-bit words; got {segments.Length} bytes.", nameof(segments)); + if (segments.Length > MaxPathBytes) + throw new ArgumentException($"A CIP path is at most {MaxPathBytes} bytes (255 words); got {segments.Length}.", nameof(segments)); var w = new CipWriter(); w.U8((byte)(segments.Length / 2)).Bytes(segments); return w.ToArray(); } + /// The path-size field is one byte of 16-bit words. + private const int MaxPathBytes = 255 * 2; + /// Message Router (class 2, instance 1). - public static readonly byte[] MessageRouter = [0x20, 0x02, 0x24, 0x01]; + public static ReadOnlySpan MessageRouter => [0x20, 0x02, 0x24, 0x01]; /// Connection Manager (class 6, instance 1). - public static readonly byte[] ConnectionManager = [0x20, 0x06, 0x24, 0x01]; + public static ReadOnlySpan ConnectionManager => [0x20, 0x06, 0x24, 0x01]; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipReply.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipReply.cs index b43f9059..ec018f3c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipReply.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipReply.cs @@ -5,6 +5,25 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Cip; /// status, extended status words, and the reply data. public sealed record CipReply(byte Service, byte GeneralStatus, ushort[] ExtendedStatus, ReadOnlyMemory Data) { + // Record equality over an array / ReadOnlyMemory is reference equality; two parses of the same wire + // bytes must compare equal, so compare the extended-status words and the data by content. + public bool Equals(CipReply? other) => + other is not null + && Service == other.Service + && GeneralStatus == other.GeneralStatus + && ExtendedStatus.AsSpan().SequenceEqual(other.ExtendedStatus) + && Data.Span.SequenceEqual(other.Data.Span); + + public override int GetHashCode() + { + var h = new HashCode(); + h.Add(Service); + h.Add(GeneralStatus); + foreach (var e in ExtendedStatus) h.Add(e); + h.AddBytes(Data.Span); + return h.ToHashCode(); + } + public AbStatus Status => AbStatusCodes.FromCip(GeneralStatus, ExtendedStatus); public static CipReply Parse(ReadOnlyMemory raw) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs index 50dd65b0..58235c4c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/ConnectionManagerServices.cs @@ -9,6 +9,24 @@ public sealed record ForwardOpenParams( public sealed record ForwardOpenReply(uint OTConnectionId, uint TOConnectionId, uint OTApiMicros, uint TOApiMicros); +/// A target refused a Forward Open. The refusal is data, not just message text: callers (and +/// diagnostics) read / directly. +public sealed class ForwardOpenRefusedException : AbTransportException +{ + public ForwardOpenRefusedException(AbStatus status, byte generalStatus, IReadOnlyList extendedStatus) + : base(status, $"Forward Open refused: general 0x{generalStatus:X2}, extended {string.Join(",", extendedStatus.Select(e => $"0x{e:X4}"))}.") + { + GeneralStatus = generalStatus; + ExtendedStatus = [.. extendedStatus]; // defensive copy: the exception is immutable + } + + /// The CIP general status of the refusal. + public byte GeneralStatus { get; } + + /// The CIP extended-status words of the refusal (may be empty). + public IReadOnlyList ExtendedStatus { get; } +} + /// Connection Manager (class 6) request builders and reply parsers. public static class ConnectionManagerServices { @@ -52,8 +70,8 @@ public static class ConnectionManagerServices public static ForwardOpenReply ParseForwardOpenReply(CipReply reply) { if (reply.GeneralStatus != 0) - throw new AbTransportException(reply.Status == AbStatus.BadReply ? AbStatus.BadConnection : reply.Status, - $"Forward Open refused: general 0x{reply.GeneralStatus:X2}, extended {string.Join(",", reply.ExtendedStatus.Select(e => $"0x{e:X4}"))}."); + throw new ForwardOpenRefusedException(reply.Status == AbStatus.BadReply ? AbStatus.BadConnection : reply.Status, + reply.GeneralStatus, reply.ExtendedStatus); var r = new CipReader(reply.Data.Span); var ot = r.U32(); var to = r.U32(); r.U16(); r.U16(); r.U32(); return new ForwardOpenReply(ot, to, r.U32(), r.U32()); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs index 2b1be8b1..571b6ce2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Eip/EncapsulationClient.cs @@ -21,7 +21,18 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Eip; /// () closes the socket; turns false and /// every later call throws (). /// The owner reconnects with a fresh client. -/// No half-written frames. A frame write never observes a cancellation token. +/// No half-written frames. A frame write never observes a cancellation token. It is +/// nevertheless bounded: the exchange timeout also covers the write, and if it elapses first the +/// connection is faulted (socket closed, which fails the pending write) rather than the write +/// being cancelled — so a peer that stops reading cannot hold the single-flight gate forever, and a +/// torn frame can only ever sit on a socket that is already dead. +/// Timeouts are finite. Every entry point rejects a non-positive or infinite timeout +/// (): the abandoned-exchange drain is bounded by it, so an +/// infinite one would let a cancelled caller's drain hold the gate forever. The gate wait itself is +/// bounded by the same timeout (, nothing written, connection intact). +/// RegisterSession. It goes out with a zero sender context (ab_server refuses a non-zero +/// one) and therefore relies on the unconnected "reply context == 0 is tolerated" rule in +/// TryMatch to match its reply; both ends of that dependency are pinned by tests. /// public sealed class EncapsulationClient : IAsyncDisposable { @@ -55,6 +66,7 @@ public sealed class EncapsulationClient : IAsyncDisposable /// bounds the TCP connect and the RegisterSession exchange separately. public static async Task ConnectAsync(string host, int port, TimeSpan timeout, CancellationToken ct) { + ValidateTimeout(timeout); var tcp = new TcpClient { NoDelay = true }; try { @@ -96,6 +108,7 @@ public sealed class EncapsulationClient : IAsyncDisposable /// Send one unconnected (UCMM) CIP request and return the CIP reply bytes. public async Task> SendRRDataAsync(ReadOnlyMemory cip, TimeSpan timeout, CancellationToken ct) { + ValidateTimeout(timeout); var secs = (ushort)Math.Clamp((int)Math.Ceiling(timeout.TotalSeconds), 1, ushort.MaxValue); var (_, reply) = await ExchangeAsync(connected: false, (ctx, _) => EncapsulationCodec.SendRRData(SessionHandle, ctx, cip.Span, secs), timeout, ct).ConfigureAwait(false); @@ -107,12 +120,19 @@ public sealed class EncapsulationClient : IAsyncDisposable public async Task> SendUnitDataAsync(uint otConnectionId, ReadOnlyMemory cip, TimeSpan timeout, CancellationToken ct) { + ValidateTimeout(timeout); var (_, reply) = await ExchangeAsync(connected: true, (ctx, seq) => EncapsulationCodec.SendUnitData(SessionHandle, ctx, otConnectionId, seq, cip.Span), timeout, ct) .ConfigureAwait(false); return reply; } + private static void ValidateTimeout(TimeSpan timeout) + { + if (timeout <= TimeSpan.Zero || timeout == Timeout.InfiniteTimeSpan) + throw new ArgumentOutOfRangeException(nameof(timeout), timeout, "Timeout must be finite and positive."); + } + /// The connected sequence count after : increments, wrapping /// 65535 → 1 (0 is never put on the wire). internal static ushort NextSequence(ushort current) => current == ushort.MaxValue ? (ushort)1 : (ushort)(current + 1); @@ -123,7 +143,9 @@ public sealed class EncapsulationClient : IAsyncDisposable bool connected, Func build, TimeSpan timeout, CancellationToken ct) { ThrowIfUnusable(); - await _gate.WaitAsync(ct).ConfigureAwait(false); + if (!await _gate.WaitAsync(timeout, ct).ConfigureAwait(false)) + throw new AbTransportException(AbStatus.Timeout, + $"Timed out after {timeout.TotalMilliseconds:F0} ms waiting for the session's single-flight gate."); Task<(EncapsulationHeader, ReadOnlyMemory)> exchange; try @@ -168,7 +190,13 @@ public sealed class EncapsulationClient : IAsyncDisposable using var timeoutCts = new CancellationTokenSource(timeout); try { - await _stream.WriteAsync(request, CancellationToken.None).ConfigureAwait(false); + // The write ignores cancellation (a half-written frame would desync the stream) but is + // bounded: on expiry the catch below faults the connection, closing the socket, which + // is what fails the still-pending write. + var write = _stream.WriteAsync(request, CancellationToken.None).AsTask(); + _ = write.ContinueWith(static t => _ = t.Exception, CancellationToken.None, + TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); + await write.WaitAsync(timeoutCts.Token).ConfigureAwait(false); while (true) { var frame = await ReadFrameAsync(timeoutCts.Token).ConfigureAwait(false); @@ -212,6 +240,8 @@ public sealed class EncapsulationClient : IAsyncDisposable if (!connected) { + // "== 0" is load-bearing, not just ab_server tolerance: RegisterSession is sent with a ZERO + // sender context (ab_server refuses non-zero), so its reply echoes 0 and matches ONLY here. if (h.SenderContext != ctx && h.SenderContext != 0) return false; var p = EncapsulationCodec.ParseReply(frame, expectConnected: false); reply = (p.Header, p.Cip); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs index 3489b4bb..7aaf315b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Wire/CipWriter.cs @@ -23,7 +23,14 @@ public sealed class CipWriter public CipWriter PadToEven() { if ((Length & 1) == 1) U8(0); return this; } /// Overwrite a u16 already written at (length back-patching). - public void PatchU16(int offset, ushort v) => BinaryPrimitives.WriteUInt16LittleEndian(_buf.AsSpan(offset, 2), v); + /// The two bytes at are not already + /// written (an offset in the spare buffer capacity would otherwise be patched silently and lost). + public void PatchU16(int offset, ushort v) + { + if (offset < 0 || offset > Length - 2) + throw new ArgumentOutOfRangeException(nameof(offset), offset, $"Cannot patch 2 bytes at {offset}: only {Length} bytes written."); + BinaryPrimitives.WriteUInt16LittleEndian(_buf.AsSpan(offset, 2), v); + } public byte[] ToArray() => _buf.AsSpan(0, Length).ToArray(); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs index 8cadaa43..905585bc 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/AbStatusCodesTests.cs @@ -31,6 +31,10 @@ public sealed class AbStatusCodesTests public void General_status_FF_maps_by_extended_status(ushort ext, AbStatus expected) => AbStatusCodes.FromCip(0xFF, [ext]).ShouldBe(expected); + [Fact] + public void General_status_FF_with_no_extended_status_is_BadReply() => + AbStatusCodes.FromCip(0xFF, ReadOnlySpan.Empty).ShouldBe(AbStatus.BadReply); + [Theory] [InlineData((byte)0x00, null, AbStatus.Ok)] [InlineData((byte)0x10, null, AbStatus.Unsupported)] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/CaptureFile.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/CaptureFile.cs index 962db6d4..3753ae7d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/CaptureFile.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/CaptureFile.cs @@ -1,3 +1,4 @@ +using System.Buffers.Binary; namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Captures; /// One EtherNet/IP encapsulation frame from a golden capture. @@ -30,15 +31,15 @@ internal static class CaptureFile public static byte[] CipPayload(CapturedFrame f) { var b = f.Bytes; - var cmd = BitConverter.ToUInt16(b, 0); // test-only; fixtures are LE on a LE host + var cmd = BinaryPrimitives.ReadUInt16LittleEndian(b.AsSpan(0)); if (cmd is not (0x6F or 0x70)) return []; // only SendRRData / SendUnitData carry CPF items var pos = 24 + 4 + 2; // header + interface handle + timeout - var itemCount = BitConverter.ToUInt16(b, pos); pos += 2; + var itemCount = BinaryPrimitives.ReadUInt16LittleEndian(b.AsSpan(pos)); pos += 2; byte[] last = []; for (var i = 0; i < itemCount; i++) { - var type = BitConverter.ToUInt16(b, pos); - var len = BitConverter.ToUInt16(b, pos + 2); + var type = BinaryPrimitives.ReadUInt16LittleEndian(b.AsSpan(pos)); + var len = BinaryPrimitives.ReadUInt16LittleEndian(b.AsSpan(pos + 2)); last = b.AsSpan(pos + 4, len).ToArray(); if (type == 0x00B1) last = last[2..]; // strip connected sequence count pos += 4 + len; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/CaptureFileTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/CaptureFileTests.cs index eab2d89e..2cdef3ce 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/CaptureFileTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/CaptureFileTests.cs @@ -1,3 +1,4 @@ +using System.Buffers.Binary; using Shouldly; using Xunit; @@ -36,7 +37,7 @@ public sealed class CaptureFileTests foreach (var f in frames) { f.Bytes.Length.ShouldBeGreaterThanOrEqualTo(24, name); - BitConverter.ToUInt16(f.Bytes, 2).ShouldBe((ushort)(f.Bytes.Length - 24), name); + BinaryPrimitives.ReadUInt16LittleEndian(f.Bytes.AsSpan(2)).ShouldBe((ushort)(f.Bytes.Length - 24), name); } } @@ -54,12 +55,12 @@ public sealed class CaptureFileTests [MemberData(nameof(AllCaptures))] public void Every_CPF_item_walk_ends_exactly_at_the_frame_end(string name) { - foreach (var f in CaptureFile.Load(name).Where(f => BitConverter.ToUInt16(f.Bytes, 0) is 0x6F or 0x70)) + foreach (var f in CaptureFile.Load(name).Where(f => BinaryPrimitives.ReadUInt16LittleEndian(f.Bytes.AsSpan(0)) is 0x6F or 0x70)) { var pos = 24 + 4 + 2; - var itemCount = BitConverter.ToUInt16(f.Bytes, pos); + var itemCount = BinaryPrimitives.ReadUInt16LittleEndian(f.Bytes.AsSpan(pos)); pos += 2; - for (var i = 0; i < itemCount; i++) pos += 4 + BitConverter.ToUInt16(f.Bytes, pos + 2); + for (var i = 0; i < itemCount; i++) pos += 4 + BinaryPrimitives.ReadUInt16LittleEndian(f.Bytes.AsSpan(pos + 2)); pos.ShouldBe(f.Bytes.Length, name); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/CipPathTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/CipPathTests.cs index 8b3b5415..c1fefd03 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/CipPathTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/CipPathTests.cs @@ -49,6 +49,65 @@ public sealed class CipPathTests public void Encodes_class_instance(ushort cls, uint inst, string hex) => Convert.ToHexString(CipPath.EncodeClassInstance(cls, inst)).ShouldBe(hex); + [Theory] + [InlineData("-1,0")] // negative port + [InlineData("65536,0")] // port does not fit a u16 + [InlineData("x,0")] // non-numeric port + [InlineData("1,é.example")] // non-ASCII link + public void Malformed_route_throws_ArgumentException(string path) => + Should.Throw(() => CipPath.EncodeRoute(path)); + + [Fact] + public void Route_link_over_255_characters_throws() + { + Should.Throw(() => CipPath.EncodeRoute("1," + new string('a', 256))); + CipPath.EncodeRoute("1," + new string('a', 255)).Length.ShouldBe(2 + 255 + 1); + } + + [Fact] + public void Route_over_255_words_throws() => + Should.Throw(() => CipPath.EncodeRoute(string.Join(",", Enumerable.Repeat("1,0", 256)))); + + [Theory] + [InlineData("")] + [InlineData("A..B")] // empty member + [InlineData(".A")] + [InlineData("A[1")] // unterminated subscript + [InlineData("A[x]")] // non-numeric subscript + [InlineData("A[-1]")] + [InlineData("A[]")] + [InlineData("A[1]B")] // junk after the subscript + [InlineData("Tä")] // non-ASCII + [InlineData("A[99999999999]")] // subscript overflows u32 + [InlineData("Tag.99999999999")] // bit selector overflows int + public void Malformed_tag_names_throw_ArgumentException(string name) => + Should.Throw(() => CipPath.EncodeSymbolic(name)); + + [Fact] + public void Tag_member_over_255_characters_throws() + { + Should.Throw(() => CipPath.EncodeSymbolic(new string('a', 256))); + CipPath.EncodeSymbolic(new string('a', 255)).Length.ShouldBe(2 + 255 + 1); + } + + [Theory] + [InlineData(1)] // odd + [InlineData(3)] + [InlineData(512)] // 256 words: size byte would wrap + public void WithPathSize_rejects_odd_and_oversize_paths(int length) => + Should.Throw(() => CipPath.WithPathSize(new byte[length])); + + [Fact] + public void WithPathSize_accepts_the_255_word_maximum() => + CipPath.WithPathSize(new byte[510])[0].ShouldBe((byte)255); + + [Fact] + public void Well_known_paths_are_immutable_spans() + { + CipPath.MessageRouter.ToArray().ShouldBe(new byte[] { 0x20, 0x02, 0x24, 0x01 }); + CipPath.ConnectionManager.ToArray().ShouldBe(new byte[] { 0x20, 0x06, 0x24, 0x01 }); + } + [Fact] public void Prefixes_path_size_in_words() => Convert.ToHexString(CipPath.WithPathSize([0x20, 0x02, 0x24, 0x01])).ShouldBe("0220022401"); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs index b8dcd1bf..0d4c2003 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Cip/ConnectionManagerServicesTests.cs @@ -105,6 +105,25 @@ public sealed class ConnectionManagerServicesTests ex.Message.ShouldContain("0x0109"); } + [Fact] + public void Refused_forward_open_carries_the_statuses_as_structured_data() + { + var reply = CipReply.Parse(Convert.FromHexString("DB000102" + "09010203")); + var ex = Should.Throw(() => ConnectionManagerServices.ParseForwardOpenReply(reply)); + ex.GeneralStatus.ShouldBe((byte)0x01); + ex.ExtendedStatus.ShouldBe(new ushort[] { 0x0109, 0x0302 }); + ex.Status.ShouldBe(AbStatus.BadConnection); // general 0x01 has no specific mapping + } + + [Fact] + public void Refused_forward_open_exception_is_not_aliased_to_the_reply_array() + { + var reply = CipReply.Parse(Convert.FromHexString("DB000101" + "0901")); + var ex = Should.Throw(() => ConnectionManagerServices.ParseForwardOpenReply(reply)); + reply.ExtendedStatus[0] = 0xFFFF; + ex.ExtendedStatus[0].ShouldBe((ushort)0x0109); + } + [Theory] [InlineData("DB000800", true)] // Service Not Supported [InlineData("DB0001010901", true)] // general 1 / extended 0x0109 diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Eip/EncapsulationClientTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Eip/EncapsulationClientTests.cs index 3392ff68..56bcda11 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Eip/EncapsulationClientTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Eip/EncapsulationClientTests.cs @@ -151,13 +151,15 @@ public sealed class EncapsulationClientTests using var callerCts = new CancellationTokenSource(); var first = client.SendRRDataAsync(new byte[] { 0x01 }, TimeSpan.FromMilliseconds(300), callerCts.Token); - await server.WaitForFrameAsync(IsRR, Guard); + // Both calls are issued back-to-back, before any await: `first` acquired the gate synchronously + // and `next` is therefore already queued behind it before the cancel — the ordering does not + // depend on how long the test thread takes to get around to it. + var next = client.SendRRDataAsync(new byte[] { 0x02 }, Generous, TestCt); await callerCts.CancelAsync(); await Should.ThrowAsync(first.WaitAsync(Guard, TestCt)); - client.IsConnected.ShouldBeTrue(); // cancel alone never faults + await server.WaitForFrameAsync(IsRR, Guard); // the abandoned request was on the wire // Queued behind the draining exchange: must fail fast once that exchange times out, not hang. - var next = client.SendRRDataAsync(new byte[] { 0x02 }, Generous, TestCt); var ex = await Should.ThrowAsync(next.WaitAsync(Guard, TestCt)); ex.Status.ShouldBe(AbStatus.BadConnection); @@ -168,6 +170,104 @@ public sealed class EncapsulationClientTests .Status.ShouldBe(AbStatus.BadConnection); } + [Fact] + public async Task RegisterSession_reply_matches_only_through_the_zero_context_tolerance() + { + // Documents a dependency: RegisterSession goes out with sender context 0 (ab_server refuses + // non-zero) so its reply echoes 0, while the exchange's own context is a non-zero counter value. + // The only thing that lets the reply match is TryMatch's "reply context == 0" rule; tighten + // that rule and every Connect breaks, not just the zero-context-server case. + await using var server = new FakeEipServer(); + await using var client = await Connect(server); + + var request = EncapsulationCodec.ReadHeader(server.Received[0]); + request.Command.ShouldBe(EncapCommand.RegisterSession); + request.SenderContext.ShouldBe(0UL); + client.SessionHandle.ShouldBe(FakeEipServer.Session); + client.IsConnected.ShouldBeTrue(); + } + + [Theory] + [InlineData(0)] + [InlineData(-5)] + public async Task Non_positive_timeouts_are_rejected_everywhere(int milliseconds) + { + await using var server = new FakeEipServer(); + var bad = TimeSpan.FromMilliseconds(milliseconds); + await Should.ThrowAsync( + EncapsulationClient.ConnectAsync("127.0.0.1", server.Port, bad, TestCt)); + await using var client = await Connect(server); + await Should.ThrowAsync(client.SendRRDataAsync(new byte[] { 0x4C }, bad, TestCt)); + await Should.ThrowAsync(client.SendUnitDataAsync(1, new byte[] { 0x4C }, bad, TestCt)); + client.IsConnected.ShouldBeTrue(); // a bad argument is not a comms fault + server.Received.Count(IsRR).ShouldBe(0); + } + + [Fact] + public async Task Infinite_timeouts_are_rejected_everywhere() + { + await using var server = new FakeEipServer(); + var inf = Timeout.InfiniteTimeSpan; + await Should.ThrowAsync( + EncapsulationClient.ConnectAsync("127.0.0.1", server.Port, inf, TestCt)); + await using var client = await Connect(server); + await Should.ThrowAsync(client.SendRRDataAsync(new byte[] { 0x4C }, inf, TestCt)); + await Should.ThrowAsync(client.SendUnitDataAsync(1, new byte[] { 0x4C }, inf, TestCt)); + client.IsConnected.ShouldBeTrue(); + } + + [Fact] + public async Task Waiting_for_the_gate_is_bounded_by_the_timeout_and_leaves_the_connection_intact() + { + await using var server = new FakeEipServer { Stall = true }; + var client = await Connect(server); + + var holder = client.SendRRDataAsync(new byte[] { 0x01 }, Generous, TestCt); // holds the gate: no reply comes + await server.WaitForFrameAsync(IsRR, Guard); + + var ex = await Should.ThrowAsync( + client.SendRRDataAsync(new byte[] { 0x02 }, TimeSpan.FromMilliseconds(150), TestCt).WaitAsync(Guard, TestCt)); + + ex.Status.ShouldBe(AbStatus.Timeout); + client.IsConnected.ShouldBeTrue(); // the waiter wrote nothing, so nothing desynced + holder.IsCompleted.ShouldBeFalse(); // positive control: the holder is still waiting on its reply + server.Received.Count(IsRR).ShouldBe(1); + + await client.DisposeAsync(); // closes the socket under the holder + (await Should.ThrowAsync(holder.WaitAsync(Guard, TestCt))) + .Status.ShouldBe(AbStatus.BadConnection); + } + + [Theory] + [InlineData(false)] // drain outlives the unregister budget: the socket is closed under it, nothing is written + [InlineData(true)] // drain times out while Dispose waits for the gate: the session is faulted, nothing is written + public async Task Dispose_during_a_cancelled_reply_drain_closes_without_interleaving_an_UnRegisterSession(bool drainTimesOutFirst) + { + await using var server = new FakeEipServer { Stall = true }; + var client = await Connect(server); + + using var callerCts = new CancellationTokenSource(); + var first = client.SendRRDataAsync(new byte[] { 0x01 }, + drainTimesOutFirst ? TimeSpan.FromMilliseconds(100) : Generous, callerCts.Token); + await server.WaitForFrameAsync(IsRR, Guard); + await callerCts.CancelAsync(); + await Should.ThrowAsync(first.WaitAsync(Guard, TestCt)); + + await client.DisposeAsync().AsTask().WaitAsync(Guard, TestCt); // must not hang on the drain + + client.IsConnected.ShouldBeFalse(); + (await Should.ThrowAsync(client.SendRRDataAsync(new byte[] { 0x03 }, Generous, TestCt))) + .Status.ShouldBe(AbStatus.BadConnection); + + // Absence needs a positive control: a second client's clean dispose MUST put an UnRegisterSession + // on the wire, so by the time it arrives the first client's (had there been one) would have too. + var second = await Connect(server); + await second.DisposeAsync(); + await server.WaitForFrameAsync(f => FakeEipServer.Command(f) == EncapCommand.UnRegisterSession, Guard); + server.Received.Count(f => FakeEipServer.Command(f) == EncapCommand.UnRegisterSession).ShouldBe(1); + server.Received.Count(IsRR).ShouldBe(1); // and the queued/abandoned work never doubled + } + [Fact] public async Task An_abandoned_exchange_that_times_out_leaves_no_unobserved_task_exception() { @@ -228,7 +328,7 @@ public sealed class EncapsulationClientTests var units = server.Received.Where(f => FakeEipServer.Command(f) == EncapCommand.SendUnitData).ToList(); var sequences = units.Select(FakeEipServer.RequestSequence).ToList(); sequences.ShouldBe(Enumerable.Range(1, 20).Select(i => (ushort)i).ToList()); // strictly increasing, no dupes - units.Select(f => BitConverter.ToUInt32(f, 36)).ShouldAllBe(id => id == 0xAABBCCDD); // A1 item connection id + units.Select(f => BinaryPrimitives.ReadUInt32LittleEndian(f.AsSpan(36))).ShouldAllBe(id => id == 0xAABBCCDD); // A1 item connection id for (var i = 0; i < 20; i++) results[i].ToArray().ShouldBe(new[] { (byte)(i + 1) }); // each got its own reply server.MaxOutstanding.ShouldBe(1); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Eip/EncapsulationCodecTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Eip/EncapsulationCodecTests.cs index 174b99e7..7175d852 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Eip/EncapsulationCodecTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Eip/EncapsulationCodecTests.cs @@ -1,3 +1,4 @@ +using System.Buffers.Binary; using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Eip; @@ -72,7 +73,7 @@ public sealed class EncapsulationCodecTests var captured = CaptureFile.Load("logix-read-dint.hex").First(f => f.FromClient && f.Bytes[0] == 0x70).Bytes; var h = EncapsulationCodec.ReadHeader(captured); var parsed = EncapsulationCodec.ParseReply(captured, expectConnected: true); - var connId = BitConverter.ToUInt32(captured, 24 + 8 + 4); // header, iface+timeout+count, A1 type+len + var connId = BinaryPrimitives.ReadUInt32LittleEndian(captured.AsSpan(24 + 8 + 4)); // header, iface+timeout+count, A1 type+len var ours = EncapsulationCodec.SendUnitData(h.SessionHandle, h.SenderContext, connId, parsed.Sequence, parsed.Cip.Span); ours.Length.ShouldBe(captured.Length); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipReplyTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipReplyTests.cs index 41026c2b..f5fa7da0 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipReplyTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipReplyTests.cs @@ -34,6 +34,25 @@ public sealed class CipReplyTests r.Status.ShouldBe(AbStatus.OutOfBounds); } + [Fact] + public void Replies_parsed_from_the_same_bytes_are_equal() + { + byte[] raw = [0xCC, 0x00, 0xFF, 0x02, 0x05, 0x21, 0x00, 0x00, 0xAA]; + // Distinct arrays on both sides: reference equality (the record default) would say unequal. + var a = CipReply.Parse(raw.ToArray()); + var b = CipReply.Parse(raw.ToArray()); + a.ShouldBe(b); + a.GetHashCode().ShouldBe(b.GetHashCode()); + } + + [Fact] + public void Replies_differing_in_extended_status_or_data_are_not_equal() + { + var a = CipReply.Parse(new byte[] { 0xCC, 0x00, 0xFF, 0x01, 0x07, 0x21, 0x01 }); + a.ShouldNotBe(CipReply.Parse(new byte[] { 0xCC, 0x00, 0xFF, 0x01, 0x05, 0x21, 0x01 })); + a.ShouldNotBe(CipReply.Parse(new byte[] { 0xCC, 0x00, 0xFF, 0x01, 0x07, 0x21, 0x02 })); + } + [Fact] public void Truncated_reply_throws_BadReply() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs new file mode 100644 index 00000000..d196d294 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Wire/CipWriterTests.cs @@ -0,0 +1,27 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Wire; +namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Wire; + +public sealed class CipWriterTests +{ + [Fact] + public void PatchU16_overwrites_an_already_written_word() + { + var w = new CipWriter().U16(0).U8(7); + w.PatchU16(0, 0xABCD); + w.ToArray().ShouldBe(new byte[] { 0xCD, 0xAB, 0x07 }); + } + + [Theory] + [InlineData(-1)] + [InlineData(2)] // would straddle the end: only bytes 0..2 are written + [InlineData(3)] + [InlineData(100)] // inside spare buffer capacity (256), but never written + public void PatchU16_outside_the_written_range_throws(int offset) + { + var w = new CipWriter().U16(0).U8(7); + Should.Throw(() => w.PatchU16(offset, 1)); + w.ToArray().ShouldBe(new byte[] { 0, 0, 7 }); // nothing was patched + } +}