From 7dbb060f9254dc8fc21459090184b8b56a75e7e8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 9 Oct 2026 05:38:24 -0400 Subject: [PATCH] fix(ablegacy): refuse libplctag writes to translated status bits (#659 fix round 1) The read translation R6:0.DN -> R6:0.CON/13 made those names parse in libplctag, which would have turned a write - previously impossible, tag creation failed - into a masked write to the control word. LibplctagLegacyTagRuntime.WriteRefusal (via IAbLegacyTagFactory.WriteRefusal, same plumbing as native) now refuses writes to every rewritten status bit (R EN/EU/DN/EM/ER/UL/IN/FD, C UA) with BadNotSupported before a runtime is created; the runtime also refuses as defence in depth. Reads stay translated. T/C status bits libplctag parses itself keep their pre-existing behaviour. 13 new tests fail with the predicate disabled. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- CLAUDE.md | 4 +- docs/drivers/AbLegacy.md | 15 ++- .../LibplctagLegacyName.cs | 27 +++- .../LibplctagLegacyTagRuntime.cs | 28 +++- .../LibplctagStatusBitWriteRefusalTests.cs | 124 ++++++++++++++++++ 5 files changed, 184 insertions(+), 14 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagStatusBitWriteRefusalTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index fc2c1c63..7a2ac620 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1093,7 +1093,9 @@ re-emit a concrete default. The defaults differ per driver (final review, 2026-1 sub-elements and status bits, and ST byte order, all correct, and wrote N/F8/B-bit (restored). libplctag (the default) used to fail `S:`/`O:`/`I:` and `R` status bits there (incl. the default probe `S:0`) — its `pccc.c` needs explicit file numbers and has no R status-bit mnemonics; since #659 `LibplctagLegacyName` translates (`S2:`/`O0:`/`I1:`, - `R6:0.CON/13`) and both transports read identically on that press. The authored grammar is unchanged. + `R6:0.CON/13`) and both transports read identically on that press. The authored grammar is unchanged. Read-side + only: a libplctag WRITE to a rewritten status bit (R `EN…FD`, `C…UA`) is refused `BadNotSupported` before any runtime + is created (it could never reach the PLC before); libplctag T/C status-bit writes it parses itself are unchanged. L/A files, function files, SLC, PLC-5 and `logixpccc` remain unverified natively. Explicit Native on PLC-5/`LogixPccc` logs a once-per-instance Warning. AbLegacy native refuses status-bit sub-element writes (`T4:0.DN`…, `BadNotSupported`). diff --git a/docs/drivers/AbLegacy.md b/docs/drivers/AbLegacy.md index b2970dfc..91eca534 100644 --- a/docs/drivers/AbLegacy.md +++ b/docs/drivers/AbLegacy.md @@ -194,8 +194,9 @@ Native routing: SLC500/MicroLogix/PLC-5 are direct; `logixpccc` uses Unconnected follow-up). PLC-5 writes are refused the same way. libplctag performs a masked bit write here; a native read-modify-write of the control word would race the PLC updating TT/DN. Reads of status bits are unaffected. - ⚠️ **On libplctag (the default) a status-bit write is NOT refused** — it goes to the controller as a write to the - timer/counter/control element (libplctag's masked bit-tag write; an `R` status bit is sent as `R:.CON/` - since #659; not exercised on a wire — `ab_server` has no T/C/R files). Nothing in authoring prevents it: the address is free text, + timer/counter element (libplctag's masked bit-tag write; not exercised on a wire — `ab_server` has no T/C/R + files). This covers the status bits libplctag parses itself (`T` `EN/TT/DN`, `C` `CU/CD/DN/OV/UN`); the ones it + has no mnemonic for — every `R` status bit and `C…UA` — **are** refused on libplctag too (`BadNotSupported`, #659). Nothing in authoring prevents it: the address is free text, any `dataType` may be chosen (a status bit is naturally authored `Bit`), and `writable` defaults to `true`. Author status-bit tags with `"writable": false` unless writing them is genuinely intended. - **PLC-5 native is N-file reads only**; every other PLC-5 operation is refused with `BadNotSupported`. @@ -242,9 +243,13 @@ status file when a digit follows it), and its sub-element table has only `con`/` bit libplctag lacks (`R` `EN/EU/DN/EM/ER/UL/IN/FD`, `C` `UA`) → `R6:0.CON/` (control word + bit, bit numbers from the native table). The authored grammar is unchanged; `S2:0`/`O0:0`/`I1:0` stay rejected by the parser (one spelling per address). Re-run on the same idle press 2026-10-09: every address above reads **identical** on both -transports (`S:4` is a free-running clock and differs read to read on each). ⚠️ The translation also means a -libplctag **write** to an `R` status bit now reaches the controller as libplctag's masked bit write (it used to fail -at tag creation) — the same behaviour T/C status bits always had on libplctag; see the status-bit write note above. +transports (`S:4` is a free-running clock and differs read to read on each). **The translation is read-side only — +it does not widen the write surface:** before it, a libplctag write to an `R` status bit (or `C…UA`) could never reach +the PLC (tag creation failed), and `R6:0.CON/13` would otherwise turn it into a masked write to the control word. So +on libplctag a write to any status bit the translation rewrites (`R` `EN/EU/DN/EM/ER/UL/IN/FD`, `C` `UA`) is refused +**`BadNotSupported` before a runtime is created** — nothing sent, health unchanged +(`LibplctagLegacyTagRuntime.WriteRefusal`, asked through `IAbLegacyTagFactory.WriteRefusal`, the same plumbing as the +native refusal). T/C status bits libplctag parses itself keep their pre-existing write behaviour (note above). Not tested on hardware: L and A files (no press has them), function files, PLC-5, SLC, `logixpccc`. Simulator evidence: diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyName.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyName.cs index b4bac25d..c68cae96 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyName.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyName.cs @@ -43,14 +43,27 @@ internal static class LibplctagLegacyName /// Returns the libplctag tag name for a canonical AbLegacy address. /// The canonical (decimal-normalised) address, e.g. S:0 or R6:0.DN. /// The libplctag name= value, e.g. S2:0 or R6:0.CON/13. - public static string Translate(string canonical) + public static string Translate(string canonical) => TranslateCore(canonical).Name; + + /// + /// True when is a status bit this translator rewrites to control word + bit + /// (R6:0.DN → R6:0.CON/13; R EN/EU/DN/EM/ER/UL/IN/FD, counter UA). A WRITE to one is + /// refused (): before the translation libplctag could not + /// even create such a tag, so a write never reached the PLC, and the rewrite must not turn it into a new masked + /// write to a production controller's control word (the native transport refuses status-bit writes too). + /// + /// The canonical address. + /// for a rewritten status bit. + public static bool IsRewrittenStatusBit(string canonical) => TranslateCore(canonical).RewrittenStatusBit; + + private static (string Name, bool RewrittenStatusBit) TranslateCore(string canonical) { // Family-neutral parse is correct: the driver hands the runtime AbLegacyAddress.ToLibplctagName(), already // decimal-normalised (PLC-5 octal I/O included) — the same reasoning as NativeAbLegacyTagRuntime.ToPcccAddress. - if (AbLegacyAddress.TryParse(canonical) is not { IsIndirect: false } a) return canonical; + if (AbLegacyAddress.TryParse(canonical) is not { IsIndirect: false } a) return (canonical, false); var fileNumber = a.FileNumber ?? PcccFileType.ImplicitFileNumber(a.FileLetter); - if (fileNumber is null) return canonical; + if (fileNumber is null) return (canonical, false); var name = $"{a.FileLetter}{fileNumber}:{a.WordNumber}"; if (a.SubElement is { } sub) @@ -68,16 +81,16 @@ internal static class LibplctagLegacyName } catch (AbTransportException) { - return canonical; // a mnemonic neither transport knows; libplctag rejects it as before + return (canonical, false); // a mnemonic neither transport knows; libplctag rejects it as before } // Only status bits live in the control word; a word sub-element libplctag lacks has no bit form. - if (native is not { SubElement: 0, Bit: int bit }) return canonical; - name += $".CON/{bit}"; + if (native is not { SubElement: 0, Bit: int bit }) return (canonical, false); + return ($"{name}.CON/{bit}", true); // a sub-element never carries a /bit as well } } if (a.BitIndex is int b) name += $"/{b}"; - return name; + return (name, false); } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs index 19cf406f..bdcef7fc 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs @@ -15,6 +15,9 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime { private readonly Tag _tag; + /// Why a write to this tag is refused before the wire (), else null. + private readonly string? _writeRefusal; + /// /// The status the native tag held when a failed initialisation destroyed it (see /// ); null while the wrapper's own handle is authoritative. @@ -29,6 +32,7 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime public LibplctagLegacyTagRuntime(AbLegacyTagCreateParams p) { _tag = BuildTag(p); + _writeRefusal = WriteRefusal(p); } /// Builds the (uninitialized) libplctag tag for . @@ -59,8 +63,27 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime RunReleasingOrphanAsync(() => _tag.ReadAsync(cancellationToken)); /// + /// Throws the refusal (Unsupported) before anything is sent — defence in depth; + /// the driver asks the factory first and never creates a runtime for a refused write. public Task WriteAsync(CancellationToken cancellationToken) => - RunReleasingOrphanAsync(() => _tag.WriteAsync(cancellationToken)); + _writeRefusal is { } refusal + ? Task.FromException(new AbTransportException(AbStatus.Unsupported, refusal)) + : RunReleasingOrphanAsync(() => _tag.WriteAsync(cancellationToken)); + + /// + /// The writes the libplctag runtime refuses before the wire, or : a status bit that + /// rewrites to control word + bit (R6:0.DN → R6:0.CON/13, R + /// EN/EU/DN/EM/ER/UL/IN/FD, counter UA). Before #659 libplctag could not create those tags at all, + /// so a write to one never reached the controller; the read-side translation must not open a new masked write + /// to a control word. T/C status bits libplctag itself parses (T4:0.DN, C5:0.CU…) keep their + /// pre-existing behaviour — a masked bit write (documented hazard in docs/drivers/AbLegacy.md). + /// + /// The tag's create parameters. + /// The refusal reason, or . + internal static string? WriteRefusal(AbLegacyTagCreateParams p) => + LibplctagLegacyName.IsRewrittenStatusBit(p.TagName) + ? $"Writes to the status bit '{p.TagName}' are not supported on the libplctag transport (libplctag has no mnemonic for it; it is read as control word + bit)." + : null; /// /// Runs a libplctag operation; when it fails and leaves the tag uninitialised, destroys the native tag it @@ -236,4 +259,7 @@ internal sealed class LibplctagLegacyTagFactory : IAbLegacyTagFactory /// public IAbLegacyTagRuntime Create(AbLegacyTagCreateParams createParams) => new LibplctagLegacyTagRuntime(createParams); + + /// + public string? WriteRefusal(AbLegacyTagCreateParams createParams) => LibplctagLegacyTagRuntime.WriteRefusal(createParams); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagStatusBitWriteRefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagStatusBitWriteRefusalTests.cs new file mode 100644 index 00000000..6166b72a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/LibplctagStatusBitWriteRefusalTests.cs @@ -0,0 +1,124 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// #659 review ruling: the libplctag read translation of a status bit libplctag has no mnemonic for +/// (R6:0.DN → R6:0.CON/13) must not widen the WRITE surface. Before the translation libplctag could +/// not create such a tag, so a write never reached the controller; now that the name parses, a write would go out +/// as a masked write to the control word. Such writes are refused BadNotSupported before any runtime is +/// created, health unchanged. T/C status bits libplctag itself parses keep their pre-existing (unrefused) behaviour. +/// +[Trait("Category", "Unit")] +public sealed class LibplctagStatusBitWriteRefusalTests +{ + [Theory] + [InlineData("R6:0.EN", true)] + [InlineData("R6:0.EU", true)] + [InlineData("R6:0.DN", true)] + [InlineData("R6:0.EM", true)] + [InlineData("R6:0.ER", true)] + [InlineData("R6:0.UL", true)] + [InlineData("R6:0.IN", true)] + [InlineData("R6:0.FD", true)] + [InlineData("C5:0.UA", true)] + [InlineData("T4:0.DN", false)] // libplctag parses it natively: pre-existing behaviour, out of scope + [InlineData("C5:0.CU", false)] + [InlineData("C5:0.DN", false)] + [InlineData("R6:0.LEN", false)] + [InlineData("R6:0.POS", false)] + [InlineData("S:0", false)] // translated, but not a status bit + [InlineData("O:0/3", false)] + [InlineData("I:0/0", false)] + [InlineData("N7:0", false)] + [InlineData("B3:0/4", false)] + [InlineData("R6:0.FOO", false)] + [InlineData("garbage", false)] + public void The_libplctag_refusal_covers_exactly_the_rewritten_status_bits(string tag, bool refused) + { + var p = new AbLegacyTagCreateParams("h", 44818, "", "micrologix", tag, TimeSpan.FromSeconds(1)); + (LibplctagLegacyTagRuntime.WriteRefusal(p) is not null).ShouldBe(refused); + (new LibplctagLegacyTagFactory().WriteRefusal(p) is not null).ShouldBe(refused); + LibplctagLegacyName.IsRewrittenStatusBit(tag).ShouldBe(refused); + } + + /// Defence in depth: a runtime for a rewritten status bit refuses the write itself, without touching the + /// network (the tag was never initialised, and the gateway is unroutable). + [Fact] + public async Task The_runtime_refuses_a_rewritten_status_bit_write_before_the_wire() + { + using var rt = new LibplctagLegacyTagRuntime(new AbLegacyTagCreateParams( + "192.0.2.1", 44818, "", "micrologix", "R6:0.DN", TimeSpan.FromSeconds(1))); + var ex = await Should.ThrowAsync(() => rt.WriteAsync(TestContext.Current.CancellationToken)); + ex.Status.ShouldBe(AbStatus.Unsupported); + } + + /// The production libplctag refusal predicate behind a counting factory, so the test sees whether the + /// driver created (and would therefore have initialised and written through) a runtime. + private sealed class CountingLibplctagRefusalFactory : IAbLegacyTagFactory + { + public FakeAbLegacyTagFactory Inner { get; } = new(); + + public IAbLegacyTagRuntime Create(AbLegacyTagCreateParams p) => Inner.Create(p); + + public string? WriteRefusal(AbLegacyTagCreateParams p) => new LibplctagLegacyTagFactory().WriteRefusal(p); + } + +#pragma warning disable OTOPCUA0001 // direct driver calls pin the driver's own status mapping (no resilience wrapper) + private static async Task<(AbLegacyDriver Driver, CountingLibplctagRefusalFactory Factory)> DriverAsync(string address) + { + const string host = "ab://10.0.0.5/"; + var factory = new CountingLibplctagRefusalFactory(); + var driver = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions(host, AbLegacyPlcFamily.MicroLogix)], + RawTags = AbLegacyRawTags.Entries([new AbLegacyTagDefinition("Bit", host, address, AbLegacyDataType.Bit, Writable: true)]), + Timeout = TimeSpan.FromSeconds(1), + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-libplctag-statusbit", factory); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + return (driver, factory); + } + + [Theory] + [InlineData("R6:0.DN")] + [InlineData("R6:0.EN")] + [InlineData("C5:0.UA")] + public async Task A_rewritten_status_bit_write_is_BadNotSupported_with_no_runtime_and_health_unchanged(string address) + { + var ct = TestContext.Current.CancellationToken; + var (driver, factory) = await DriverAsync(address); + var health = driver.GetHealth(); + + (await driver.WriteAsync([new WriteRequest("Bit", true)], ct)).Single().StatusCode + .ShouldBe(AbLegacyStatusMapper.BadNotSupported); + + factory.Inner.Tags.ShouldBeEmpty(); + driver.GetHealth().ShouldBe(health); + + // Reads stay translated and allowed: the same tag reads through a runtime. + (await driver.ReadAsync(["Bit"], ct)).Single().StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + factory.Inner.Tags.Keys.ShouldContain(address); + await driver.ShutdownAsync(ct); + } + + /// Positive control for the refusal's scope: a T/C status bit libplctag parses itself keeps its + /// pre-existing behaviour — the write reaches a runtime (documented hazard, out of scope for #659). + [Fact] + public async Task A_libplctag_native_timer_status_bit_write_still_reaches_a_runtime() + { + var ct = TestContext.Current.CancellationToken; + var (driver, factory) = await DriverAsync("T4:0.DN"); + + (await driver.WriteAsync([new WriteRequest("Bit", true)], ct)).Single().StatusCode + .ShouldBe(AbLegacyStatusMapper.Good); + + factory.Inner.Tags["T4:0.DN"].WriteCount.ShouldBe(1); + await driver.ShutdownAsync(ct); + } +#pragma warning restore OTOPCUA0001 +}