From b249c15c1a4dca1ecc5968082e72df88e9ed97f1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 16:54:18 -0400 Subject: [PATCH] fix(ablegacy): refuse status-bit writes, decode status bits as 0/1, reject over-long replies, global TNS --- .../NativeAbLegacyTagRuntime.cs | 67 +++++++++++++------ .../NativeAbLegacyTagRuntimeTests.cs | 56 +++++++++++++++- 2 files changed, 103 insertions(+), 20 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/NativeAbLegacyTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/NativeAbLegacyTagRuntime.cs index 86f37732..a3078527 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/NativeAbLegacyTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/NativeAbLegacyTagRuntime.cs @@ -16,9 +16,9 @@ internal sealed class NativeAbLegacyTagRuntime : IAbLegacyTagRuntime private const int StringDataBytes = 82; private const int StringElementBytes = 84; - // Seeds each runtime's TNS counter apart from the others so a stale reply belonging to another tag on - // the same connection fails the TNS echo check instead of matching by accident. - private static int s_tnsSeed; + // One process-wide TNS counter: no two in-flight requests (on any connection) share a TNS, so a stale + // reply belonging to another tag fails the TNS echo check instead of matching by accident. + private static int s_tns; private readonly string _plc; private readonly RequestScheduler _scheduler; @@ -31,7 +31,6 @@ internal sealed class NativeAbLegacyTagRuntime : IAbLegacyTagRuntime private readonly int _elementSize; private readonly int _count; private readonly byte[] _buffer; - private ushort _tns; private int _status; /// Creates a runtime that leases the shared connection for its device from . @@ -62,7 +61,6 @@ internal sealed class NativeAbLegacyTagRuntime : IAbLegacyTagRuntime _requestorId[0] = 7; BinaryPrimitives.WriteUInt16LittleEndian(_requestorId.AsSpan(1), vendor); BinaryPrimitives.WriteUInt32LittleEndian(_requestorId.AsSpan(3), serial); - _tns = (ushort)Interlocked.Add(ref s_tnsSeed, 1000); _count = Math.Max(1, p.ElementCount); try @@ -85,6 +83,11 @@ internal sealed class NativeAbLegacyTagRuntime : IAbLegacyTagRuntime /// Unconnected Send (0x52) with route 01 00 (README line ~129, scratch capture: ab_server /// rejected it, so the shape is read from the client side), so it is . /// + /// + /// logixpccc framing is UNVERIFIED and differs from libplctag: libplctag uses PLC-5 addressing with + /// 0x68/0x67 for LGX_PCCC, while this runtime sends SLC typed 0xA2/0xAA (no capture exists either way). + /// plc5 is likewise unverified beyond N-file reads. Neither family should default to Native until proven. + /// internal static CipRouting RoutingFor(string plcAttribute) => plcAttribute switch { "logixpccc" => CipRouting.UnconnectedSend, @@ -172,7 +175,7 @@ internal sealed class NativeAbLegacyTagRuntime : IAbLegacyTagRuntime private async Task> ExchangeAsync(Func pccc, CancellationToken ct) { - var tns = ++_tns; + var tns = unchecked((ushort)Interlocked.Increment(ref s_tns)); var request = PcccServices.ExecutePccc(_vendor, _serial, pccc(tns)); var reply = await _scheduler.SendAsync(request, ct).ConfigureAwait(false); return PcccServices.ParseReply(reply, tns, _requestorId); @@ -198,6 +201,8 @@ internal sealed class NativeAbLegacyTagRuntime : IAbLegacyTagRuntime ct).ConfigureAwait(false); if (data.Length < bytes) throw new AbTransportException(AbStatus.BadReply, $"PCCC read returned {data.Length} of {bytes} bytes."); + if (data.Length > bytes) + throw new AbTransportException(AbStatus.TooLarge, $"PCCC read returned {data.Length} bytes, more than the {bytes} requested."); data.Span[..bytes].CopyTo(staged.AsSpan(done * _elementSize)); } @@ -209,6 +214,12 @@ internal sealed class NativeAbLegacyTagRuntime : IAbLegacyTagRuntime { if (_plc == "plc5") throw new AbTransportException(AbStatus.Unsupported, "PLC-5 writes are not capture-verified."); + // libplctag does a masked bit write (0xAB) here; there is no capture of it, and a read-modify-write + // would race the PLC updating TT/DN. Writing the word instead would clobber EN/TT/DN, so refuse + // before anything reaches the wire. + if (StatusBit is not null) + throw new AbTransportException(AbStatus.Unsupported, + "Writes to a status-bit sub-element (e.g. T4:0.DN) are not supported by the native transport."); var (chunk, address) = Plan(); for (var done = 0; done < _count; done += chunk) { @@ -239,25 +250,43 @@ internal sealed class NativeAbLegacyTagRuntime : IAbLegacyTagRuntime private int GetInt32(int offset) => BinaryPrimitives.ReadInt32LittleEndian(Slice(offset, 4)); private float GetFloat32(int offset) => BinaryPrimitives.ReadSingleLittleEndian(Slice(offset, 4)); + /// The bit of a status-bit sub-element (T4:0.DN, C5:0.CU, R6:0.EN...), else null. + private int? StatusBit => _address is { SubElement: not null, Bit: int b } ? b : null; + private bool GetBit(int bit) => bit < 16 ? (((ushort)GetInt16(0) >> bit) & 1) != 0 : (((uint)GetInt32(0) >> bit) & 1) != 0; /// - public object? DecodeValue(AbLegacyDataType type, int? bitIndex) => type switch + public object? DecodeValue(AbLegacyDataType type, int? bitIndex) { - // A status-bit sub-element (T4:0.DN) carries its bit on the transport address; an explicit /N wins. - AbLegacyDataType.Bit => bitIndex is int bit - ? GetBit(bit) - : _address is { SubElement: not null, Bit: int subBit } ? GetBit(subBit) : (GetInt16(0) & 1) != 0, - AbLegacyDataType.Int or AbLegacyDataType.AnalogInt => (int)GetInt16(0), - AbLegacyDataType.Long => GetInt32(0), - AbLegacyDataType.Float => GetFloat32(0), - AbLegacyDataType.String => DecodeString(), - AbLegacyDataType.TimerElement or AbLegacyDataType.CounterElement - or AbLegacyDataType.ControlElement => GetInt32(0), - _ => null, - }; + // A status-bit sub-element is a BIT tag in libplctag: every scalar getter returns that bit (0/1). + if (StatusBit is int sb && type != AbLegacyDataType.String) + { + var v = GetBit(sb); + return type switch + { + AbLegacyDataType.Bit => v, + AbLegacyDataType.Int or AbLegacyDataType.AnalogInt or AbLegacyDataType.Long + or AbLegacyDataType.TimerElement or AbLegacyDataType.CounterElement + or AbLegacyDataType.ControlElement => v ? 1 : 0, + AbLegacyDataType.Float => v ? 1f : 0f, + _ => null, + }; + } + + return type switch + { + AbLegacyDataType.Bit => bitIndex is int bit ? GetBit(bit) : (GetInt16(0) & 1) != 0, + AbLegacyDataType.Int or AbLegacyDataType.AnalogInt => (int)GetInt16(0), + AbLegacyDataType.Long => GetInt32(0), + AbLegacyDataType.Float => GetFloat32(0), + AbLegacyDataType.String => DecodeString(), + AbLegacyDataType.TimerElement or AbLegacyDataType.CounterElement + or AbLegacyDataType.ControlElement => GetInt32(0), + _ => null, + }; + } // SLC ST element: u16 LEN, then 82 data bytes with each byte pair swapped. private string DecodeString() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs index 761a0a0d..266fb44e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/NativeAbLegacyTagRuntimeTests.cs @@ -255,7 +255,7 @@ public sealed class NativeAbLegacyTagRuntimeTests await using var h = new Harness("slc500", "N7:0").Replies(Words(1)); await h.Runtime.ReadAsync(CancellationToken.None); await h.Runtime.ReadAsync(CancellationToken.None); - ((ushort)(TnsOf(h.Channel.Sent[1]) - TnsOf(h.Channel.Sent[0]))).ShouldBe((ushort)1); + TnsOf(h.Channel.Sent[1]).ShouldNotBe(TnsOf(h.Channel.Sent[0])); } [Fact] @@ -359,6 +359,60 @@ public sealed class NativeAbLegacyTagRuntimeTests ex.Status.ShouldBe(AbStatus.BadReply); } + [Fact] + public async Task An_over_long_data_reply_is_TooLarge() + { + await using var h = new Harness("slc500", "N7:0").Replies(Words(1, 2)); + var ex = await Should.ThrowAsync(() => h.Runtime.ReadAsync(CancellationToken.None)); + ex.Status.ShouldBe(AbStatus.TooLarge); + } + + [Theory] + [InlineData("T4:0.DN")] + [InlineData("C5:0.CU")] + [InlineData("R6:0.EN")] + public async Task A_write_to_a_status_bit_sub_element_is_Unsupported_and_sends_nothing(string tag) + { + await using var h = new Harness("slc500", tag).Replies([]); + h.Runtime.EncodeValue(AbLegacyDataType.Bit, null, true); + var ex = await Should.ThrowAsync(() => h.Runtime.WriteAsync(CancellationToken.None)); + ex.Status.ShouldBe(AbStatus.Unsupported); + h.Channel.SentCount.ShouldBe(0); + } + + [Theory] + [InlineData("T4:0.PRE")] + [InlineData("T4:0.ACC")] + [InlineData("R6:0.LEN")] + [InlineData("R6:0.POS")] + public async Task A_write_to_a_word_sub_element_still_works(string tag) + { + await using var h = new Harness("slc500", tag).Replies([]); + h.Runtime.EncodeValue(AbLegacyDataType.Int, null, 9); + await h.Runtime.WriteAsync(CancellationToken.None); + h.Channel.SentCount.ShouldBe(1); + } + + [Theory] + [InlineData(AbLegacyDataType.Bit)] + [InlineData(AbLegacyDataType.Int)] + [InlineData(AbLegacyDataType.AnalogInt)] + [InlineData(AbLegacyDataType.Long)] + [InlineData(AbLegacyDataType.TimerElement)] + [InlineData(AbLegacyDataType.CounterElement)] + [InlineData(AbLegacyDataType.ControlElement)] + public async Task Every_scalar_type_decodes_a_status_bit_sub_element_as_that_bit(AbLegacyDataType type) + { + foreach (var (word, expectSet) in new[] { ((ushort)0xE000, true), ((ushort)0x0000, false) }) + { + await using var h = new Harness("slc500", "T4:0.DN").Replies(Words(word)); + await h.Runtime.ReadAsync(CancellationToken.None); + var v = h.Runtime.DecodeValue(type, null); + if (type == AbLegacyDataType.Bit) v.ShouldBe(expectSet); + else v.ShouldBe(expectSet ? 1 : 0); + } + } + [Fact] public async Task A_short_data_reply_is_BadReply() {