From e2e441d45ed992afa523589fa6a32f885eab7a85 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 16:56:30 -0400 Subject: [PATCH] fix(abcip): native runtime write guards, same-width integer exemption, deferred path encoding (Task 12 review r1) --- .../NativeAbCipTagRuntime.cs | 72 ++++++++++++-- .../NativeAbCipTagRuntimeTests.cs | 97 +++++++++++++++++++ 2 files changed, 159 insertions(+), 10 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs index f2d9f8c5..f445bd2d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/NativeAbCipTagRuntime.cs @@ -28,10 +28,13 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime private readonly AbCipTagCreateParams _p; private readonly DeviceSessionPool? _pool; - private readonly int? _implicitBit; - private readonly byte[] _path; private readonly ushort _elements; + // Encoded in InitializeAsync, never in the constructor: a malformed name must surface from + // Initialize (inside the driver's / probe's try), not from IAbCipTagFactory.Create — as libplctag. + private int? _implicitBit; + private byte[]? _path; + private ConnectionLease? _lease; private RequestScheduler? _privateScheduler; // ForTest only — production always uses the lease's private LogixTagClient? _client; @@ -57,9 +60,6 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime private NativeAbCipTagRuntime(AbCipTagCreateParams p) { _p = p ?? throw new ArgumentNullException(nameof(p)); - var (name, bit) = CipPath.SplitBitSelector(p.TagName); - _implicitBit = bit; - _path = CipPath.EncodeSymbolic(name); _elements = (ushort)Math.Clamp(p.ElementCount, 1, ushort.MaxValue); } @@ -97,6 +97,14 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime public async Task InitializeAsync(CancellationToken cancellationToken) { ObjectDisposedException.ThrowIf(_disposed, this); + if (_path is null) + { + // Before any lease: a malformed name costs no connection. + var (name, bit) = CipPath.SplitBitSelector(_p.TagName); + var path = CipPath.EncodeSymbolic(name); + _implicitBit = bit; + _path = path; + } if (_client is null) { var lease = _pool!.Acquire(NativeAbCipDevice.Key(_p), NativeAbCipDevice.Options(_p)); @@ -111,9 +119,10 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime public async Task ReadAsync(CancellationToken cancellationToken) { var client = Client(); + var path = Path(); try { - var (type, handle, data) = await client.ReadAsync(_path, _elements, cancellationToken).ConfigureAwait(false); + var (type, handle, data) = await client.ReadAsync(path,_elements, cancellationToken).ConfigureAwait(false); _data = data; _typeCode = type; _structHandle = handle; @@ -131,6 +140,15 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime public async Task WriteAsync(CancellationToken cancellationToken) { var client = Client(); + var path = Path(); + + // A bit tag ("Flags.3") names one bit of its host word. Writing any encoded value to it would + // write the WHOLE host and wipe its other bits on the controller; BOOL bit writes are an RMW the + // driver performs on a parent-DINT runtime (WriteBitInDIntAsync), so anything reaching here is refused. + if (_implicitBit is int implicitBit) + throw new NotSupportedException( + $"Write to bit tag '{_p.TagName}' (bit {implicitBit}) is not supported here — bit writes go through AbCipDriver.WriteBitInDIntAsync."); + try { if (_pendingString is { } text) @@ -144,7 +162,7 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime throw new AbTransportException(AbStatus.Unsupported, $"STRING write to '{_p.TagName}': the controller returned no structure handle."); var payload = EncodeStringPayload(text); - await client.WriteAsync(_path, _typeCode!.Value, _structHandle, 1, payload, cancellationToken) + await client.WriteAsync(path, _typeCode!.Value, _structHandle, 1, payload, cancellationToken) .ConfigureAwait(false); } else if (_writeBuffer is { } buffer) @@ -152,11 +170,25 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime var type = _typeCode ?? AtomicCode(_p.AuthoredType) ?? throw new AbTransportException(AbStatus.Unsupported, $"Write to '{_p.TagName}': type unknown — no prior read and no authored atomic type."); + // The read rule decides first: a same-width mismatch (REAL under a DINT-authored tag) + // would otherwise send the int bit pattern as a REAL and report Good. + if (_typeCode is { } returned && IsTypeMismatch(returned)) + throw new AbTransportException(AbStatus.TypeMismatch, + $"Write to '{_p.TagName}': the controller's type 0x{returned:X4} does not match the authored {_p.AuthoredType}."); + // Width is checked only for codes with a known width: DATE / DATE_AND_TIME (0xCD/0xCF) and + // Micro800 BYTE/WORD/LWORD (0xD1/0xD2/0xD4) are outside LogixTypeCode's table and write as today. var width = LogixTypeCode.ByteWidth(type); - if (width != buffer.Length) + if (width > 0 && width != buffer.Length) + { + // An authored BOOL over a packed BOOL-array DWORD: writing the DWORD would clobber the + // 31 sibling bools (libplctag does exactly that), so refuse it as a config error. + if (type == LogixTypeCode.BoolArray) + throw new NotSupportedException( + $"Write to '{_p.TagName}': a BOOL-array element (type 0xD3 DWORD) cannot be written as a single BOOL."); throw new AbTransportException(AbStatus.TypeMismatch, $"Write to '{_p.TagName}': encoded {buffer.Length} byte(s) but the controller's type 0x{type:X4} is {width} byte(s) wide."); - await client.WriteAsync(_path, type, null, 1, buffer, cancellationToken).ConfigureAwait(false); + } + await client.WriteAsync(path, type, null, 1, buffer, cancellationToken).ConfigureAwait(false); } else { @@ -324,6 +356,9 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime _privateScheduler = null; } + private byte[] Path() => + _path ?? throw new InvalidOperationException("InitializeAsync has not been called."); + private LogixTagClient Client() { ObjectDisposedException.ThrowIf(_disposed, this); @@ -334,7 +369,10 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime /// True when the controller returned an atomic type other than the authored one. Exempt: /// no authored type, String/Structure, a non-atomic return, a BOOL bit selector against an integer /// host (Flags.3 on a DINT), and an authored BOOL answered as 0xD3 (a packed BOOL-array - /// DWORD, which is how Logix returns a BOOL-array element). + /// DWORD, which is how Logix returns a BOOL-array element), and same-width integers that differ only + /// in signedness or are the Micro800 bit-string type of that width (SINT/USINT/BYTE, INT/UINT/WORD, + /// DINT/UDINT/DWORD, LINT/ULINT/LWORD): libplctag reads those Good today and a transport cutover + /// must not turn them Bad. Different widths and integer↔real still mismatch. /// private bool IsTypeMismatch(ushort returned) { @@ -345,9 +383,23 @@ internal sealed class NativeAbCipTagRuntime : IAbCipTagRuntime if (returned == LogixTypeCode.BoolArray) return false; if (_implicitBit is not null && IsIntegerCode(returned)) return false; } + if (IntegerWidth(expected) is int width && IntegerWidth(returned) == width) return false; return true; } + /// Micro800 bit-string type codes (DWORD is , 0xD3). + private const ushort ByteCode = 0xD1, WordCode = 0xD2, LWordCode = 0xD4; + + /// Width of an integer or bit-string code; null for anything else (BOOL, REAL, ...). + private static int? IntegerWidth(ushort code) => code switch + { + LogixTypeCode.SInt or LogixTypeCode.USInt or ByteCode => 1, + LogixTypeCode.Int or LogixTypeCode.UInt or WordCode => 2, + LogixTypeCode.DInt or LogixTypeCode.UDInt or LogixTypeCode.BoolArray => 4, + LogixTypeCode.LInt or LogixTypeCode.ULInt or LWordCode => 8, + _ => null, + }; + /// The Logix type code an authored type expects, or null when it is exempt from checking. private static ushort? AtomicCode(AbCipDataType? type) => type switch { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs index 4ef31541..2e6e6b5d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/NativeAbCipTagRuntimeTests.cs @@ -248,6 +248,103 @@ public sealed class NativeAbCipTagRuntimeTests ch.SentCount.ShouldBe(sentBefore); } + [Fact] + public async Task A_Dt_write_under_DATE_type_0xCD_sends_four_bytes() + { + // 0xCD is outside LogixTypeCode's width table: no width check, the write goes out as today. + var ch = Replying("CD00", "00000000"); + using var rt = await ReadAsync(ch, Params("D", AbCipDataType.Dt)); + rt.GetStatus().ShouldBe((int)AbStatus.Ok); + ch.Respond = req => Task.FromResult(FakeCipChannel.OkReply(req)); + rt.EncodeValue(AbCipDataType.Dt, null, 123456); + await rt.WriteAsync(CancellationToken.None); + ch.Sent[^1].ShouldBe(Convert.FromHexString("4D02" + "91014400" + "CD00" + "0100" + "40E20100")); + } + + [Fact] + public async Task A_BOOL_write_over_a_packed_BOOL_array_DWORD_is_NotSupported_and_sends_nothing() + { + var ch = Replying("D300", "01000000"); + using var rt = await ReadAsync(ch, Params("Bits[0]", AbCipDataType.Bool)); + var sentBefore = ch.SentCount; + rt.EncodeValue(AbCipDataType.Bool, null, false); + await Should.ThrowAsync(() => rt.WriteAsync(CancellationToken.None)); + ch.SentCount.ShouldBe(sentBefore); + } + + [Theory] + [InlineData(AbCipDataType.DInt)] + [InlineData(AbCipDataType.Int)] + [InlineData(AbCipDataType.Bool)] + public async Task Any_write_to_a_bit_tag_is_NotSupported_and_sends_nothing(AbCipDataType authored) + { + // "Flags.3" authored as DInt/Int would otherwise write the value over the WHOLE host DINT. + var ch = Replying("C400", "08000000"); + using var rt = await ReadAsync(ch, Params("Flags.3", authored)); + var sentBefore = ch.SentCount; + rt.EncodeValue(authored, null, 1); + await Should.ThrowAsync(() => rt.WriteAsync(CancellationToken.None)); + ch.SentCount.ShouldBe(sentBefore); + } + + [Fact] + public async Task A_write_after_a_same_width_type_mismatch_is_TypeMismatch_and_sends_nothing() + { + // REAL returned for a DINT-authored tag: same width, so only the read rule can catch it. + var ch = Replying("CA00", "0000803F"); + using var rt = await ReadAsync(ch, Params("Speed", AbCipDataType.DInt)); + var sentBefore = ch.SentCount; + rt.EncodeValue(AbCipDataType.DInt, null, 42); + (await Should.ThrowAsync(() => rt.WriteAsync(CancellationToken.None))) + .Status.ShouldBe(AbStatus.TypeMismatch); + ch.SentCount.ShouldBe(sentBefore); + rt.GetStatus().ShouldBe((int)AbStatus.TypeMismatch); + } + + [Theory] + [InlineData(AbCipDataType.DInt, "C800", "2A000000", 42)] // DINT ← UDINT + [InlineData(AbCipDataType.UDInt, "C400", "2A000000", 42u)] // UDINT ← DINT + [InlineData(AbCipDataType.Int, "C700", "2A00", 42)] // INT ← UINT + [InlineData(AbCipDataType.UInt, "C300", "2A00", 42)] // UINT ← INT + [InlineData(AbCipDataType.SInt, "C600", "2A", 42)] // SINT ← USINT + [InlineData(AbCipDataType.USInt, "C200", "2A", 42)] // USINT ← SINT + [InlineData(AbCipDataType.LInt, "C900", "2A00000000000000", 42L)] // LINT ← ULINT + [InlineData(AbCipDataType.ULInt, "C500", "2A00000000000000", 42UL)] // ULINT ← LINT + [InlineData(AbCipDataType.DInt, "D300", "2A000000", 42)] // DINT ← Micro800 DWORD + [InlineData(AbCipDataType.UDInt, "D300", "2A000000", 42u)] // UDINT ← Micro800 DWORD + [InlineData(AbCipDataType.Int, "D200", "2A00", 42)] // INT ← Micro800 WORD + [InlineData(AbCipDataType.USInt, "D100", "2A", 42)] // USINT ← Micro800 BYTE + [InlineData(AbCipDataType.LInt, "D400", "2A00000000000000", 42L)] // LINT ← Micro800 LWORD + public async Task Same_width_integer_pairs_are_not_a_type_mismatch( + AbCipDataType authored, string typeHex, string dataHex, object expected) + { + using var rt = await ReadAsync(Replying(typeHex, dataHex), Params("T", authored)); + rt.GetStatus().ShouldBe((int)AbStatus.Ok); + rt.DecodeValue(authored, null).ShouldBe(expected); + } + + [Theory] + [InlineData(AbCipDataType.Int, "C400", "2A000000")] // different width + [InlineData(AbCipDataType.Real, "D300", "2A000000")] // integer ↔ real, same width + [InlineData(AbCipDataType.UDInt, "CA00", "0000803F")] // real ↔ integer, same width + public async Task Different_widths_and_integer_real_pairs_still_mismatch( + AbCipDataType authored, string typeHex, string dataHex) + { + using var rt = await ReadAsync(Replying(typeHex, dataHex), Params("T", authored)); + rt.GetStatus().ShouldBe((int)AbStatus.TypeMismatch); + } + + [Fact] + public async Task A_malformed_tag_name_never_throws_from_Create_and_surfaces_on_Initialize_without_IO() + { + var ch = new FakeCipChannel(); + var p = Params("Arr[abc]", AbCipDataType.DInt); + using var created = new NativeAbCipTagFactory().Create(p); // must not throw + using var rt = NativeAbCipTagRuntime.ForTest(ch, p); + await Should.ThrowAsync(() => rt.InitializeAsync(CancellationToken.None)); + ch.SentCount.ShouldBe(0); + } + [Fact] public async Task A_refused_write_stores_the_status_and_rethrows() {