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<byte>; 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
2026-10-07 22:54:32 -04:00
parent 04b6563286
commit 7892eb13a8
15 changed files with 379 additions and 30 deletions
@@ -2,7 +2,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport;

 /// <summary>Thrown by both AB transports. <see cref="Status"/> drives OPC UA status mapping and the
 /// driver's "controller answered, don't throttle the device" classification (05/STAB-8).</summary>
-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;
@@ -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);
    }

+    /// <exception cref="ArgumentException">The name is empty, has an empty member, an over-long (&gt; 255) or
+    /// non-ASCII member, an unterminated / non-numeric subscript, or ends in a bit selector.</exception>
    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();
    }

+    /// <summary>ASCII bytes of a name / link whose length rides in a single byte.</summary>
+    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);
+    }
+
    /// <summary>Split on '.' outside brackets, keeping "Program:X" whole.</summary>
    private static IEnumerable<string> SplitMembers(string name)
    {
@@ -94,16 +129,25 @@ public static class CipPath
    }

    /// <summary>Prefixes the path with its size in 16-bit words.</summary>
+    /// <exception cref="ArgumentException">The segments are not a whole number of 16-bit words, or exceed
+    /// the 255-word maximum the one-byte size field can carry.</exception>
    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();
    }

+    /// <summary>The path-size field is one byte of 16-bit words.</summary>
+    private const int MaxPathBytes = 255 * 2;
+
    /// <summary>Message Router (class 2, instance 1).</summary>
-    public static readonly byte[] MessageRouter = [0x20, 0x02, 0x24, 0x01];
+    public static ReadOnlySpan<byte> MessageRouter => [0x20, 0x02, 0x24, 0x01];

    /// <summary>Connection Manager (class 6, instance 1).</summary>
-    public static readonly byte[] ConnectionManager = [0x20, 0x06, 0x24, 0x01];
+    public static ReadOnlySpan<byte> ConnectionManager => [0x20, 0x06, 0x24, 0x01];
 }
@@ -5,6 +5,25 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Cip;
 /// status, extended status words, and the reply data.</summary>
 public sealed record CipReply(byte Service, byte GeneralStatus, ushort[] ExtendedStatus, ReadOnlyMemory<byte> 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<byte> raw)
@@ -9,6 +9,24 @@ public sealed record ForwardOpenParams(

 public sealed record ForwardOpenReply(uint OTConnectionId, uint TOConnectionId, uint OTApiMicros, uint TOApiMicros);

+/// <summary>A target refused a Forward Open. The refusal is data, not just message text: callers (and
+/// diagnostics) read <see cref="GeneralStatus"/> / <see cref="ExtendedStatus"/> directly.</summary>
+public sealed class ForwardOpenRefusedException : AbTransportException
+{
+    public ForwardOpenRefusedException(AbStatus status, byte generalStatus, IReadOnlyList<ushort> 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
+    }
+
+    /// <summary>The CIP general status of the refusal.</summary>
+    public byte GeneralStatus { get; }
+
+    /// <summary>The CIP extended-status words of the refusal (may be empty).</summary>
+    public IReadOnlyList<ushort> ExtendedStatus { get; }
+}
+
 /// <summary>Connection Manager (class 6) request builders and reply parsers.</summary>
 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());
@@ -21,7 +21,18 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Eip;
 ///     (<see cref="AbStatus.BadReply"/>) closes the socket; <see cref="IsConnected"/> turns false and
 ///     every later call throws <see cref="AbTransportException"/>(<see cref="AbStatus.BadConnection"/>).
 ///     The owner reconnects with a fresh client.</para>
-///     <para><b>No half-written frames.</b> A frame write never observes a cancellation token.</para>
+///     <para><b>No half-written frames.</b> 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 <i>faulted</i> (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.</para>
+///     <para><b>Timeouts are finite.</b> Every entry point rejects a non-positive or infinite timeout
+///     (<see cref="ArgumentOutOfRangeException"/>): 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 (<see cref="AbStatus.Timeout"/>, nothing written, connection intact).</para>
+///     <para><b>RegisterSession.</b> 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
+///     <c>TryMatch</c> to match its reply; both ends of that dependency are pinned by tests.</para>
 /// </remarks>
 public sealed class EncapsulationClient : IAsyncDisposable
 {
@@ -55,6 +66,7 @@ public sealed class EncapsulationClient : IAsyncDisposable
    /// bounds the TCP connect and the RegisterSession exchange separately.</summary>
    public static async Task<EncapsulationClient> 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
    /// <summary>Send one unconnected (UCMM) CIP request and return the CIP reply bytes.</summary>
    public async Task<ReadOnlyMemory<byte>> SendRRDataAsync(ReadOnlyMemory<byte> 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<ReadOnlyMemory<byte>> SendUnitDataAsync(uint otConnectionId, ReadOnlyMemory<byte> 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.");
+    }
+
    /// <summary>The connected sequence count after <paramref name="current"/>: increments, wrapping
    /// 65535 → 1 (0 is never put on the wire).</summary>
    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<ulong, ushort, byte[]> 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<byte>)> 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);
@@ -23,7 +23,14 @@ public sealed class CipWriter
    public CipWriter PadToEven() { if ((Length & 1) == 1) U8(0); return this; }

    /// <summary>Overwrite a u16 already written at <paramref name="offset"/> (length back-patching).</summary>
-    public void PatchU16(int offset, ushort v) => BinaryPrimitives.WriteUInt16LittleEndian(_buf.AsSpan(offset, 2), v);
+    /// <exception cref="ArgumentOutOfRangeException">The two bytes at <paramref name="offset"/> are not already
+    /// written (an offset in the spare buffer capacity would otherwise be patched silently and lost).</exception>
+    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();
 }