From f85fc8fbeb35c591ec9e63dcc2c1c5ad7beeb856 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 9 Oct 2026 04:31:45 -0400 Subject: [PATCH] =?UTF-8?q?fix(abcip):=20#611=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20native=20keeps=20BOOL=20type=20distinction,=20Write?= =?UTF-8?q?WidthRefusal=20tests,=20doc=20caveats?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - L2: WriteWidthRefusal leaves a BOOL over a wider value to a runtime that sees the CIP type (native): mistyped integer member -> BadTypeMismatch (as its read), 0xD3 BOOL-array DWORD -> BadNotSupported; libplctag keeps BadNotSupported for both (size only). - L4: direct theory over WriteWidthRefusal (1/2/4/8 widths incl LINT/ULINT/LREAL, String/Dt, unknown size) + array/bit-selector exemptions with a non-vacuous control. - L1: CLAUDE.md member/BIT-member writes are ab_server-proven only. - L3: patch 0003 blind spots documented (Docker README, AbServer-Test-Fixture.md); nits. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- CLAUDE.md | 2 +- docs/drivers/AbCip.md | 9 +- docs/drivers/AbServer-Test-Fixture.md | 4 +- .../AbCipDriver.cs | 13 +- .../Docker/README.md | 10 +- .../Docker/docker-compose.yml | 2 +- .../AbCipDiscoveredMemberWriteTests.cs | 115 ++++++++++++++++++ 7 files changed, 146 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9d1513cf..c8f1717e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1102,7 +1102,7 @@ once per tag — only genuine transport failures evict and degrade (a runtime `N it used to latch `Degraded`). AbCip width-checks every scalar **write** against the controller's bytes before sending, on both transports (#611; libplctag used to patch part of its buffer and send it all). Controller-discovered UDT members are writable once browse-committed and authored `ReadWrite` (BIT members included — the controller -applies the bit); whole-UDT writes are refused `BadNotSupported` by decision (`docs/drivers/AbCip.md` §Member writes). `DeviceSessionPool.Shared` logs through the +applies the bit) — ab_server-proven only, member and BIT-member writes are unverified on real hardware; whole-UDT writes are refused `BadNotSupported` by decision (`docs/drivers/AbCip.md` §Member writes). `DeviceSessionPool.Shared` logs through the host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT factories' `Register(registry, ILoggerFactory?)` take one too, #602 CONV-3). The AbCip `browsePageDelayMs` knob has an AdminUI field (`AbCipDriverForm` "Browse page delay (ms)"). libplctag is confined to `Libplctag*.cs` adapters by `LibplctagConfinementGuardTests`; do not reference it elsewhere. See `docs/drivers/AbCip.md` §Transport, `docs/drivers/AbLegacy.md` §Transport + §Wire evidence, `docs/Configuration.md` §`Drivers:AbCip`, and diff --git a/docs/drivers/AbCip.md b/docs/drivers/AbCip.md index 98cb235d..bb81a9f1 100644 --- a/docs/drivers/AbCip.md +++ b/docs/drivers/AbCip.md @@ -405,8 +405,10 @@ when it is browse-committed in `/raw`, and that tag writes like any other atomic with the bytes the controller holds at the path (the runtime's last read) and refuses a mismatch without writing: a narrower/wider atomic type is `BadTypeMismatch` (libplctag would otherwise patch part of its read buffer and send the whole buffer back — e.g. an `Int` over a DINT member wrote four bytes with a stale high - word); a `Bool` over a wider value (a packed BOOL-array DWORD, a mistyped integer) is `BadNotSupported`, - because writing it would rewrite every neighbouring bit. Native additionally refuses a same-width + word); a `Bool` over a wider value is never written, because writing it would rewrite every neighbouring + bit. On **native** the runtime judges it from the CIP type, exactly as its read does: a packed BOOL-array + DWORD (`0xD3`) is `BadNotSupported`, a BOOL authored over a mistyped integer member is `BadTypeMismatch`. + **libplctag** exposes only the size, cannot tell the two apart, and refuses both `BadNotSupported`. Native additionally refuses a same-width mismatch (REAL vs DINT) from the CIP type code — see [Native transport behaviour](#native-transport-behaviour). A refusal keeps the handle, warns once per tag and **does not touch driver health**. - **Not verified on a real controller.** Production presses are read-only, so member writes are proven on @@ -414,7 +416,8 @@ when it is browse-committed in `/raw`, and that tag writes like any other atomic Z-35162 controller tag `_EmptyTemperatureSegment` (`UDT_TemperatureProfile_Segment`), which no logic references — its members (`ID`, `Enable`, `RampTime`, …) are all zero and resolve as described above. -Pre-declared `Members` with `Writable: true` keep working exactly as before (same write path). +Pre-declared `Members` with `Writable: true` keep working as before — same write path, now width-checked +(a mistyped declared member is refused where libplctag used to write it). ### Whole-UDT writes — refused, by decision diff --git a/docs/drivers/AbServer-Test-Fixture.md b/docs/drivers/AbServer-Test-Fixture.md index e198c6c2..a56eba20 100644 --- a/docs/drivers/AbServer-Test-Fixture.md +++ b/docs/drivers/AbServer-Test-Fixture.md @@ -55,7 +55,9 @@ smoke test passes on both transports. Evidence: - ab_server serves no UDTs, so template parity is vacuous there. Docker patch 0003 serves a UDT **member path** from a flat tag named by that dotted path (`UdtSeg.ID`, `UdtSeg.Sub.Code`, `UdtSeg.Points[2]`) — the wire of a member read/write, not a structure — and accepts the pad byte libplctag adds to a 1-byte write, - which stock ab_server refused (`0x03`) for every libplctag BOOL/SINT write. Native writes remain + which stock ab_server refused (`0x03`) for every libplctag BOOL/SINT write. Two blind spots follow (see + the Docker README §0003): a +1-byte over-write of an odd-sized tag is no longer caught, and a malformed + single segment `"UdtSeg.ID"` resolves where a real Logix would refuse it. Native writes remain simulator-only. - **Golden-frame capture:** `scripts/ab-capture/capture.sh` (capture client in `scripts/ab-capture/client`); fixtures live in `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/` with a README. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 64a22ca0..25bf13ba 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1197,7 +1197,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, // BOOL-array DWORD wrote all 32 bits. Discovered UDT members are where a mis-authored type // is most likely (browse-commit fills dataType from the template, an operator may retype). size = runtime.GetSize(); - refusal = WriteWidthRefusal(def, parsedPath?.BitIndex, size); + refusal = WriteWidthRefusal(def, parsedPath?.BitIndex, size, runtimeTypesBool: runtime is NativeAbCipTagRuntime); if (refusal is null) { runtime.EncodeValue(def.DataType, parsedPath?.BitIndex, w.Value); @@ -1710,15 +1710,24 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// would clobber the neighbouring bits, the same refusal the native runtime raises; any other width /// mismatch is BadTypeMismatch. Arrays, bit selectors (Flags.3, served by the /// read-modify-write path) and an unknown size (<= 0) are not judged here. + /// + /// A BOOL over a wider value is left to the runtime when it can see the CIP type + /// ( — the native transport): it tells a packed BOOL-array DWORD + /// (0xD3, BadNotSupported) from a mistyped integer member (BadTypeMismatch, the + /// same verdict as its read), and refuses both before sending. libplctag exposes only the size, so the + /// two are indistinguishable there and both get the safe BadNotSupported. + /// /// /// The tag being written. /// The parsed bit selector, if any. /// The runtime's read-buffer size in bytes. + /// The runtime checks a BOOL write against the controller's CIP type itself. /// The refusal status, or null. - internal static uint? WriteWidthRefusal(AbCipTagDefinition def, int? bitIndex, int size) + internal static uint? WriteWidthRefusal(AbCipTagDefinition def, int? bitIndex, int size, bool runtimeTypesBool = false) { if (IsArrayTag(def) || bitIndex is not null || size <= 0) return null; if (ExpectedScalarWriteWidth(def.DataType) is not { } width || size == width) return null; + if (def.DataType == AbCipDataType.Bool && runtimeTypesBool) return null; return def.DataType == AbCipDataType.Bool ? AbCipStatusMapper.BadNotSupported : AbCipStatusMapper.BadTypeMismatch; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md index c0ebb5f0..189892c5 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/README.md @@ -90,6 +90,14 @@ things for the discovered-UDT-member write suite (`AbCipUdtMemberWriteTests`, Gi `0x03` (libplctag `ErrorBadParam`), so no libplctag BOOL/SINT write could pass here. The patch drops exactly one trailing pad byte when it alone would overrun an odd-sized target. +**Blind spots this patch introduces (accepted for a fixture):** (a) the pad trim also accepts a genuinely +wrong 2-byte payload aimed at a 1-byte tag, so ab_server can no longer catch a client over-writing an +odd-sized target by exactly one byte — the driver's own pre-write width check (`AbCipDriver.WriteWidthRefusal`) +covers our paths; (b) because `.` is legal in a `--tag` name, a malformed *single* symbolic segment +`"UdtSeg.ID"` (which a real Logix rejects) also resolves, via the original exact-match lookup — +`NativeAbCipTagRuntimeTests.A_member_path_write_names_the_member_and_sends_only_its_own_bytes` pins that our +encoder splits members into separate segments. + **Bumping `LIBPLCTAG_TAG` means re-checking every patch against the new tag.** It is a concrete tag rather than the `release` branch precisely so that this stays a deliberate act — the branch moves, and the Dockerfile previously called it @@ -159,7 +167,7 @@ place means updating both. | Family | Seeded tags | Notes | |---|---|---| -| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` `Pump_1_Speed` `Line1_Count` `Motor_Run` `UdtSeg.*` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server; the `UdtSeg.*` member tags (patch 0003) serve member-path writes only. The last three serve the controller-browse gate and are additive — the read/write suites key off the `Test*` tags. `Motor_Run` (9), `Line1_Count` (11) and `TestArray` (9) have **odd-length names on purpose**: entries are unpadded, so an odd name puts every following entry on an odd offset, and a decoder that assumes even alignment loses everything behind it | +| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` `Pump_1_Speed` `Line1_Count` `Motor_Run` `UdtSeg.*` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server; the `UdtSeg.*` member tags (patch 0003) serve member-path writes only. `Pump_1_Speed`, `Line1_Count` and `Motor_Run` serve the controller-browse gate and are additive — the read/write suites key off the `Test*` tags. `Motor_Run` (9), `Line1_Count` (11) and `TestArray` (9) have **odd-length names on purpose**: entries are unpadded, so an odd name puts every following entry on an odd offset, and a decoder that assumes even alignment loses everything behind it | | CompactLogix | `TestDINT` `TestREAL` `TestBOOL` | Narrow ConnectionSize cap enforced driver-side; ab_server accepts any size | | Micro800 | `TestDINT` `TestREAL` | ab_server has no `micro800` mode; falls back to `controllogix` emulation | | GuardLogix | `TestDINT` `SafetyDINT_S` | ab_server has no safety subsystem; `_S` suffix triggers driver-side classification only | diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml index 8e6e45ae..ce9dfaa0 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.IntegrationTests/Docker/docker-compose.yml @@ -20,7 +20,7 @@ services: restart: "no" ports: - "44818:44818" - # The last three are here for the controller-browse gate (Gitea #468) and + # Pump_1_Speed, Line1_Count and Motor_Run are here for the controller-browse gate (Gitea #468) and # are additive — the read/write suites key off the Test* tags above. # The UdtSeg.* tags (Gitea #611) stand in for the members of a UDT tag `UdtSeg` # (modelled on the Z-35162 press's UDT_TemperatureProfile_Segment): ab_server has diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs index 2fd0df3b..74fdaae2 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipDiscoveredMemberWriteTests.cs @@ -2,6 +2,7 @@ using System.Runtime.CompilerServices; using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes; namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; @@ -153,6 +154,120 @@ public sealed class AbCipDiscoveredMemberWriteTests factory.Tags["Seg.ID"].WriteCount.ShouldBe(0); } + /// + /// Review L4 — directly, with a real size so the check runs: + /// every atomic width (1/2/4/8, incl. LINT/ULINT/LREAL) passes at its own size and is refused at another; + /// arrays, Tag.N bit selectors, STRING/DT and an unknown size (<= 0) are never judged. + /// + [Theory] + // matching widths write + [InlineData(AbCipDataType.Bool, 1, null)] + [InlineData(AbCipDataType.SInt, 1, null)] + [InlineData(AbCipDataType.USInt, 1, null)] + [InlineData(AbCipDataType.Int, 2, null)] + [InlineData(AbCipDataType.UInt, 2, null)] + [InlineData(AbCipDataType.DInt, 4, null)] + [InlineData(AbCipDataType.UDInt, 4, null)] + [InlineData(AbCipDataType.Real, 4, null)] + [InlineData(AbCipDataType.LInt, 8, null)] + [InlineData(AbCipDataType.ULInt, 8, null)] + [InlineData(AbCipDataType.LReal, 8, null)] + // mismatches are refused + [InlineData(AbCipDataType.LInt, 4, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.LReal, 4, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.ULInt, 2, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.Real, 8, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.Int, 4, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.SInt, 2, AbCipStatusMapper.BadTypeMismatch)] + [InlineData(AbCipDataType.Bool, 4, AbCipStatusMapper.BadNotSupported)] + // types with no fixed encoded width are never judged + [InlineData(AbCipDataType.String, 88, null)] + [InlineData(AbCipDataType.String, 4, null)] + [InlineData(AbCipDataType.Dt, 8, null)] + // unknown size is never judged + [InlineData(AbCipDataType.DInt, 0, null)] + [InlineData(AbCipDataType.DInt, -1, null)] + public void WriteWidthRefusal_judges_scalar_widths_only(AbCipDataType type, int size, uint? expected) + { + var def = new AbCipTagDefinition("T", Device, "Seg.X", type); + AbCipDriver.WriteWidthRefusal(def, bitIndex: null, size).ShouldBe(expected); + } + + /// Review L4 — array tags and bit selectors are exempt even at a size that would otherwise refuse. + [Fact] + public void WriteWidthRefusal_exempts_arrays_and_bit_selectors() + { + // Control: the same scalar at the same size IS refused, so the exemptions below are not vacuous. + AbCipDriver.WriteWidthRefusal(new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int), null, 4) + .ShouldBe(AbCipStatusMapper.BadTypeMismatch); + + AbCipDriver.WriteWidthRefusal( + new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int, ElementCount: 4, IsArray: true), null, 4) + .ShouldBeNull(); + AbCipDriver.WriteWidthRefusal( + new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int, ElementCount: 2), null, 4) + .ShouldBeNull(); + AbCipDriver.WriteWidthRefusal( + new AbCipTagDefinition("T", Device, "Flags.3", AbCipDataType.Bool), bitIndex: 3, size: 4) + .ShouldBeNull(); + AbCipDriver.WriteWidthRefusal( + new AbCipTagDefinition("T", Device, "Word.3", AbCipDataType.Int), bitIndex: 3, size: 4) + .ShouldBeNull(); + } + + /// + /// Review L2 — a runtime that types BOOL writes itself (native) is left to judge a BOOL over a wider value; + /// only libplctag, which sees a size and nothing else, gets the pre-check's BadNotSupported. + /// + [Fact] + public void WriteWidthRefusal_leaves_a_wide_BOOL_to_a_runtime_that_sees_the_CIP_type() + { + var def = new AbCipTagDefinition("T", Device, "Seg.Flag", AbCipDataType.Bool); + AbCipDriver.WriteWidthRefusal(def, null, 4, runtimeTypesBool: false).ShouldBe(AbCipStatusMapper.BadNotSupported); + AbCipDriver.WriteWidthRefusal(def, null, 4, runtimeTypesBool: true).ShouldBeNull(); + // Non-BOOL mismatches are still refused up front on every runtime. + AbCipDriver.WriteWidthRefusal(new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int), null, 4, + runtimeTypesBool: true).ShouldBe(AbCipStatusMapper.BadTypeMismatch); + } + + /// + /// Review L2, end to end on the native runtime: a BOOL authored over a mistyped DINT member writes + /// BadTypeMismatch (the same verdict as its read); a BOOL over a packed BOOL-array DWORD writes + /// BadNotSupported. Neither sends a write; health is untouched. + /// + [Theory] + [InlineData("C400", AbCipStatusMapper.BadTypeMismatch)] // DINT member mistyped as Bool + [InlineData("D300", AbCipStatusMapper.BadNotSupported)] // packed BOOL-array DWORD + public async Task A_native_BOOL_over_a_wider_value_keeps_its_type_distinction(string typeHex, uint expected) + { + var channels = new List(); + var factory = new NativeFactory(p => + { + var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString("D2000000" + typeHex + "00000000")) }; + lock (channels) channels.Add(ch); + return NativeAbCipTagRuntime.ForTest(ch, p); + }); + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = [Committed("Seg.Flag", "Bool")], + Probe = new AbCipProbeOptions { Enabled = false }, + }, "drv-native", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + var health = drv.GetHealth(); + + (await drv.WriteAsync([new WriteRequest("Seg.Flag", true)], CancellationToken.None)) + .Single().StatusCode.ShouldBe(expected); + + channels.ShouldHaveSingleItem().Sent.ShouldAllBe(req => req[0] != 0x4D); // no Write Tag request + drv.GetHealth().ShouldBe(health); + } + + private sealed class NativeFactory(Func create) : IAbCipTagFactory + { + public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => create(createParams); + } + // ---- helpers --------------------------------------------------------------------------------------- /// Browses a UDT shaped like the Z-35162 press's UDT_TemperatureProfile_Segment (+ a nested struct).