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
This commit is contained in:
@@ -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`).
|
||||
|
||||
|
||||
@@ -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<f>:<e>.CON/<bit>`
|
||||
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/<bit>` (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:
|
||||
|
||||
@@ -43,14 +43,27 @@ internal static class LibplctagLegacyName
|
||||
/// <summary>Returns the libplctag tag name for a canonical AbLegacy address.</summary>
|
||||
/// <param name="canonical">The canonical (decimal-normalised) address, e.g. <c>S:0</c> or <c>R6:0.DN</c>.</param>
|
||||
/// <returns>The libplctag <c>name=</c> value, e.g. <c>S2:0</c> or <c>R6:0.CON/13</c>.</returns>
|
||||
public static string Translate(string canonical)
|
||||
public static string Translate(string canonical) => TranslateCore(canonical).Name;
|
||||
|
||||
/// <summary>
|
||||
/// True when <paramref name="canonical"/> is a status bit this translator rewrites to control word + bit
|
||||
/// (<c>R6:0.DN</c> → <c>R6:0.CON/13</c>; R <c>EN/EU/DN/EM/ER/UL/IN/FD</c>, counter <c>UA</c>). A WRITE to one is
|
||||
/// refused (<see cref="LibplctagLegacyTagRuntime.WriteRefusal"/>): 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).
|
||||
/// </summary>
|
||||
/// <param name="canonical">The canonical address.</param>
|
||||
/// <returns><see langword="true"/> for a rewritten status bit.</returns>
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -15,6 +15,9 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime
|
||||
{
|
||||
private readonly Tag _tag;
|
||||
|
||||
/// <summary>Why a write to this tag is refused before the wire (<see cref="WriteRefusal"/>), else null.</summary>
|
||||
private readonly string? _writeRefusal;
|
||||
|
||||
/// <summary>
|
||||
/// The status the native tag held when a failed initialisation destroyed it (see
|
||||
/// <see cref="LibplctagOrphanRelease"/>); 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);
|
||||
}
|
||||
|
||||
/// <summary>Builds the (uninitialized) libplctag tag for <paramref name="p"/>.</summary>
|
||||
@@ -59,8 +63,27 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime
|
||||
RunReleasingOrphanAsync(() => _tag.ReadAsync(cancellationToken));
|
||||
|
||||
/// <inheritdoc />
|
||||
/// <remarks>Throws the <see cref="WriteRefusal"/> refusal (Unsupported) before anything is sent — defence in depth;
|
||||
/// the driver asks the factory first and never creates a runtime for a refused write.</remarks>
|
||||
public Task WriteAsync(CancellationToken cancellationToken) =>
|
||||
RunReleasingOrphanAsync(() => _tag.WriteAsync(cancellationToken));
|
||||
_writeRefusal is { } refusal
|
||||
? Task.FromException(new AbTransportException(AbStatus.Unsupported, refusal))
|
||||
: RunReleasingOrphanAsync(() => _tag.WriteAsync(cancellationToken));
|
||||
|
||||
/// <summary>
|
||||
/// The writes the libplctag runtime refuses before the wire, or <see langword="null"/>: a status bit that
|
||||
/// <see cref="LibplctagLegacyName"/> rewrites to control word + bit (<c>R6:0.DN</c> → <c>R6:0.CON/13</c>, R
|
||||
/// <c>EN/EU/DN/EM/ER/UL/IN/FD</c>, counter <c>UA</c>). 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 (<c>T4:0.DN</c>, <c>C5:0.CU</c>…) keep their
|
||||
/// pre-existing behaviour — a masked bit write (documented hazard in <c>docs/drivers/AbLegacy.md</c>).
|
||||
/// </summary>
|
||||
/// <param name="p">The tag's create parameters.</param>
|
||||
/// <returns>The refusal reason, or <see langword="null"/>.</returns>
|
||||
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;
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <inheritdoc />
|
||||
public IAbLegacyTagRuntime Create(AbLegacyTagCreateParams createParams) =>
|
||||
new LibplctagLegacyTagRuntime(createParams);
|
||||
|
||||
/// <inheritdoc />
|
||||
public string? WriteRefusal(AbLegacyTagCreateParams createParams) => LibplctagLegacyTagRuntime.WriteRefusal(createParams);
|
||||
}
|
||||
|
||||
+124
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// #659 review ruling: the libplctag read translation of a status bit libplctag has no mnemonic for
|
||||
/// (<c>R6:0.DN</c> → <c>R6:0.CON/13</c>) 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 <c>BadNotSupported</c> before any runtime is
|
||||
/// created, health unchanged. T/C status bits libplctag itself parses keep their pre-existing (unrefused) behaviour.
|
||||
/// </summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>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).</summary>
|
||||
[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<AbTransportException>(() => rt.WriteAsync(TestContext.Current.CancellationToken));
|
||||
ex.Status.ShouldBe(AbStatus.Unsupported);
|
||||
}
|
||||
|
||||
/// <summary>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.</summary>
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>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).</summary>
|
||||
[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
|
||||
}
|
||||
Reference in New Issue
Block a user