diff --git a/code-reviews/Driver.AbLegacy/findings.md b/code-reviews/Driver.AbLegacy/findings.md index c4ffd56..c827121 100644 --- a/code-reviews/Driver.AbLegacy/findings.md +++ b/code-reviews/Driver.AbLegacy/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 13 | +| Open findings | 11 | ## Checklist coverage @@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank. | Severity | High | | Category | Correctness & logic bugs | | Location | `AbLegacyAddress.cs:54`, `AbLegacyDriver.cs:368-374` | -| Status | Open | +| Status | Resolved | **Description:** `AbLegacyAddress.TryParse` accepts a `BitIndex` of `0..31` for every file type. A PCCC N-file word is a signed 16-bit integer, so valid bit indices are @@ -54,7 +54,10 @@ in `WriteBitInWordAsync` - reject bit > 15 for N/B/I/O/S files and bit > 31 for files. For bit-in-word RMW against L files, read the parent as `Long`. Mask the read-back value to the word width before applying the bit operation. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `AbLegacyAddress.TryParse` now range-checks the +bit index against per-file word width (0..15 for N/B/I/O/S/A, 0..31 for L, no bits on +F); `WriteBitInWordAsync` reads/writes an L-file parent as 32-bit `Long` and masks the +RMW arithmetic to the native width so sign-extension can no longer corrupt high bits. ### Driver.AbLegacy-002 @@ -161,7 +164,7 @@ libplctag status per device. | Severity | High | | Category | Concurrency & thread safety | | Location | `AbLegacyDriver.cs:107-158`, `AbLegacyDriver.cs:162-234`, `LibplctagLegacyTagRuntime.cs` | -| Status | Open | +| Status | Resolved | **Description:** A per-tag `IAbLegacyTagRuntime` (wrapping a single libplctag `Tag`) is cached in `DeviceState.Runtimes` and reused. `ReadAsync` (called directly by the @@ -179,7 +182,10 @@ hazard. Only the bit-in-word RMW path is serialised (per-parent `SemaphoreSlim`) `SemaphoreSlim`, or a per-device read lock - so no two threads touch the same `Tag` handle concurrently. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added a per-runtime `SemaphoreSlim` +(`DeviceState.GetRuntimeLock`, keyed by tag name); `ReadAsync` and `WriteAsync` now +hold it around the whole Read→GetStatus→Decode / Encode→Write→GetStatus sequence so the +shared libplctag `Tag` handle is never touched by two threads at once. ### Driver.AbLegacy-007 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyAddress.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyAddress.cs index 3da4f48..2cce003 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyAddress.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyAddress.cs @@ -46,12 +46,14 @@ public sealed record AbLegacyAddress( if (string.IsNullOrWhiteSpace(value)) return null; var src = value.Trim(); - // BitIndex: trailing /N + // BitIndex: trailing /N. The valid range depends on the parent word width, which is + // determined by the file letter (16-bit N/B/I/O/S/A → 0..15, 32-bit L → 0..31). Capture + // the raw value here and range-check it once the file letter is known (see below). int? bitIndex = null; var slashIdx = src.IndexOf('/'); if (slashIdx >= 0) { - if (!int.TryParse(src[(slashIdx + 1)..], out var bit) || bit < 0 || bit > 31) return null; + if (!int.TryParse(src[(slashIdx + 1)..], out var bit) || bit < 0) return null; bitIndex = bit; src = src[..slashIdx]; } @@ -91,9 +93,30 @@ public sealed record AbLegacyAddress( // Reject unknown file letters — these cover SLC/ML/PLC-5 canonical families. if (!IsKnownFileLetter(letter)) return null; + // Range-check the bit index against the file's word width. A PCCC N/B/I/O/S/A word is a + // 16-bit element, so valid bit indices are 0..15; an L-file element is 32-bit (0..31). + // F-files are 32-bit IEEE-754 floats and are not bit-addressable at all. + if (bitIndex is int b) + { + var maxBit = MaxBitIndexFor(letter); + if (maxBit < 0 || b > maxBit) return null; + } + return new AbLegacyAddress(letter, fileNumber, word, bitIndex, subElement); } + /// + /// Highest valid bit index for a file letter, or -1 if the file type is not + /// bit-addressable. 16-bit element files (N/B/I/O/S/A) permit bits 0..15; the 32-bit + /// L-file permits 0..31. + /// + private static int MaxBitIndexFor(string letter) => letter switch + { + "L" => 31, + "N" or "B" or "I" or "O" or "S" or "A" => 15, + _ => -1, + }; + private static bool IsKnownFileLetter(string letter) => letter switch { "N" or "F" or "B" or "L" or "ST" or "T" or "C" or "R" or "I" or "O" or "S" or "A" => true, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 3145e58..3bc5f74 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -128,9 +128,26 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover try { var runtime = await EnsureTagRuntimeAsync(device, def, cancellationToken).ConfigureAwait(false); - await runtime.ReadAsync(cancellationToken).ConfigureAwait(false); - var status = runtime.GetStatus(); + int status; + object? value; + // Serialise the Read → GetStatus → DecodeValue sequence against the shared + // runtime — the server read path and the poll loop both call ReadAsync against + // the same cached libplctag Tag handle, which is not concurrency-safe. + var opLock = device.GetRuntimeLock(def.Name); + await opLock.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + await runtime.ReadAsync(cancellationToken).ConfigureAwait(false); + status = runtime.GetStatus(); + var parsed = AbLegacyAddress.TryParse(def.Address); + value = status == 0 ? runtime.DecodeValue(def.DataType, parsed?.BitIndex) : null; + } + finally + { + opLock.Release(); + } + if (status != 0) { results[i] = new DataValueSnapshot(null, @@ -140,8 +157,6 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover continue; } - var parsed = AbLegacyAddress.TryParse(def.Address); - var value = runtime.DecodeValue(def.DataType, parsed?.BitIndex); results[i] = new DataValueSnapshot(value, AbLegacyStatusMapper.Good, now, now); _health = new DriverHealth(DriverState.Healthy, now, null); } @@ -201,10 +216,23 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover } var runtime = await EnsureTagRuntimeAsync(device, def, cancellationToken).ConfigureAwait(false); - runtime.EncodeValue(def.DataType, parsed?.BitIndex, w.Value); - await runtime.WriteAsync(cancellationToken).ConfigureAwait(false); - var status = runtime.GetStatus(); + int status; + // Serialise Encode → Write → GetStatus against the shared runtime — the same + // cached Tag handle may be in use by a concurrent ReadAsync or poll loop. + var opLock = device.GetRuntimeLock(def.Name); + await opLock.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + runtime.EncodeValue(def.DataType, parsed?.BitIndex, w.Value); + await runtime.WriteAsync(cancellationToken).ConfigureAwait(false); + status = runtime.GetStatus(); + } + finally + { + opLock.Release(); + } + results[i] = new WriteResult(status == 0 ? AbLegacyStatusMapper.Good : AbLegacyStatusMapper.MapLibplctagStatus(status)); @@ -345,14 +373,24 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover } /// - /// Read-modify-write one bit within a PCCC N-file word. Strips the /N bit suffix to - /// form the parent-word address (N7:0/3 → N7:0), creates / reuses a parent-word runtime - /// typed as Int16, serialises concurrent bit writers against the same parent via a - /// per-parent . + /// Read-modify-write one bit within a PCCC word. Strips the /N bit suffix to form the + /// parent-word address (N7:0/3 → N7:0), creates / reuses a parent-word runtime typed at + /// the parent's native width (Int16 for N/I/O/S/A files, Int32 for the 32-bit L file), + /// and serialises concurrent bit writers against the same parent via a per-parent + /// . The parent word is masked to its native width before the + /// bit operation so a sign-extended decode never corrupts the high bits. /// private async Task WriteBitInWordAsync( AbLegacyDriver.DeviceState device, AbLegacyAddress bitAddress, int bit, object? value, CancellationToken ct) { + // The parent word width follows the file letter: an L-file element is 32-bit, every + // other bit-addressable PCCC file (N/I/O/S/A) is a 16-bit word. bit is already + // range-checked by AbLegacyAddress.TryParse (0..15 for 16-bit files, 0..31 for L), so + // 1 << bit can never overflow the chosen width here. + var isLong = bitAddress.FileLetter == "L"; + var parentType = isLong ? AbLegacyDataType.Long : AbLegacyDataType.Int; + var widthMask = isLong ? unchecked((int)0xFFFFFFFF) : 0xFFFF; + var parentAddress = bitAddress with { BitIndex = null }; var parentName = parentAddress.ToLibplctagName(); @@ -365,12 +403,19 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover var readStatus = parentRuntime.GetStatus(); if (readStatus != 0) return AbLegacyStatusMapper.MapLibplctagStatus(readStatus); - var current = Convert.ToInt32(parentRuntime.DecodeValue(AbLegacyDataType.Int, bitIndex: null) ?? 0); + // Mask the decoded word to its native width: DecodeValue for a 16-bit Int returns a + // sign-extended int, so a word with bit 15 set decodes negative. Masking pins the + // RMW arithmetic to exactly the parent's bits. + var current = Convert.ToInt32(parentRuntime.DecodeValue(parentType, bitIndex: null) ?? 0) & widthMask; var updated = Convert.ToBoolean(value) ? current | (1 << bit) : current & ~(1 << bit); + updated &= widthMask; - parentRuntime.EncodeValue(AbLegacyDataType.Int, bitIndex: null, (short)updated); + if (isLong) + parentRuntime.EncodeValue(AbLegacyDataType.Long, bitIndex: null, updated); + else + parentRuntime.EncodeValue(AbLegacyDataType.Int, bitIndex: null, (short)updated); await parentRuntime.WriteAsync(ct).ConfigureAwait(false); var writeStatus = parentRuntime.GetStatus(); return writeStatus == 0 @@ -464,6 +509,20 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover public SemaphoreSlim GetRmwLock(string parentName) => _rmwLocks.GetOrAdd(parentName, _ => new SemaphoreSlim(1, 1)); + private readonly System.Collections.Concurrent.ConcurrentDictionary _runtimeLocks = new(); + + /// + /// Per-runtime operation lock. A libplctag Tag handle is not safe for + /// concurrent Read/GetStatus/DecodeValue from multiple threads — the server read + /// path and the poll loop both call against + /// the same cached runtime. Callers hold this lock around the whole + /// Read → GetStatus → Decode (or Encode → Write → GetStatus) sequence so a status + /// or value is never observed mid-update by another thread. Keyed by tag name, which + /// is also the dictionary key. + /// + public SemaphoreSlim GetRuntimeLock(string tagName) => + _runtimeLocks.GetOrAdd(tagName, _ => new SemaphoreSlim(1, 1)); + public object ProbeLock { get; } = new(); public HostState HostState { get; set; } = HostState.Unknown; public DateTime HostStateChangedUtc { get; set; } = DateTime.UtcNow; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyBitIndexRangeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyBitIndexRangeTests.cs new file mode 100644 index 0000000..7b3be80 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyBitIndexRangeTests.cs @@ -0,0 +1,106 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Regression coverage for Driver.AbLegacy-001 — a PCCC bit index must be range-checked +/// against the parent word width: 0..15 for 16-bit element files (N/B/I/O/S/A), 0..31 for +/// the 32-bit L file. Float files are not bit-addressable. +/// +[Trait("Category", "Unit")] +public sealed class AbLegacyBitIndexRangeTests +{ + [Theory] + [InlineData("N7:0/15")] + [InlineData("B3:0/15")] + [InlineData("I:0/15")] + [InlineData("O:1/15")] + [InlineData("S:1/15")] + [InlineData("A10:0/15")] + public void Bit_index_0_to_15_accepted_on_16bit_files(string input) => + AbLegacyAddress.TryParse(input).ShouldNotBeNull(); + + [Theory] + [InlineData("N7:0/16")] // first bit past a 16-bit word + [InlineData("N7:0/20")] + [InlineData("N7:0/31")] + [InlineData("B3:0/16")] + [InlineData("I:0/16")] + [InlineData("O:1/16")] + [InlineData("S:1/16")] + [InlineData("A10:0/16")] + public void Bit_index_above_15_rejected_on_16bit_files(string input) => + AbLegacyAddress.TryParse(input).ShouldBeNull(); + + [Theory] + [InlineData("L9:0/0")] + [InlineData("L9:0/15")] + [InlineData("L9:0/16")] // L-file words are 32-bit, so 16..31 are valid + [InlineData("L9:0/31")] + public void Bit_index_0_to_31_accepted_on_L_file(string input) => + AbLegacyAddress.TryParse(input).ShouldNotBeNull(); + + [Fact] + public void Bit_index_above_31_rejected_on_L_file() => + AbLegacyAddress.TryParse("L9:0/32").ShouldBeNull(); + + [Theory] + [InlineData("F8:0/0")] // float files are not bit-addressable at all + [InlineData("F8:0/3")] + public void Bit_index_rejected_on_float_file(string input) => + AbLegacyAddress.TryParse(input).ShouldBeNull(); + + [Fact] + public void Negative_bit_index_still_rejected() => + AbLegacyAddress.TryParse("N7:0/-1").ShouldBeNull(); + + [Fact] + public async Task Bit_in_word_RMW_against_L_file_uses_32bit_parent_and_high_bit() + { + // L9:0/20 — bit 20 of a 32-bit L-file word. The parent must be read/written as a + // 32-bit Long so the high bits are addressable; a 16-bit (short)cast would truncate. + var factory = new FakeAbLegacyTagFactory + { + Customise = p => new FakeAbLegacyTag(p) { Value = 0 }, + }; + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [new AbLegacyTagDefinition("LBit20", "ab://10.0.0.5/1,0", "L9:0/20", AbLegacyDataType.Bit)], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var results = await drv.WriteAsync([new WriteRequest("LBit20", true)], CancellationToken.None); + + results.Single().StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + factory.Tags.ShouldContainKey("L9:0"); + Convert.ToInt32(factory.Tags["L9:0"].Value).ShouldBe(1 << 20); + } + + [Fact] + public async Task Bit_in_word_RMW_high_bit_15_does_not_corrupt_via_sign_extension() + { + // Parent word has bit 15 set (0x8000) — DecodeValue returns a sign-extended negative + // int. Setting bit 0 must yield exactly 0x8001, not a sign-extended value. + var factory = new FakeAbLegacyTagFactory + { + Customise = p => new FakeAbLegacyTag(p) { Value = unchecked((short)0x8000) }, + }; + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [new AbLegacyTagDefinition("Bit0", "ab://10.0.0.5/1,0", "N7:0/0", AbLegacyDataType.Bit)], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + await drv.WriteAsync([new WriteRequest("Bit0", true)], CancellationToken.None); + + // (short)0x8001 round-trips through the fake as -32767. + Convert.ToInt32(factory.Tags["N7:0"].Value).ShouldBe(unchecked((short)0x8001)); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyRuntimeConcurrencyTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyRuntimeConcurrencyTests.cs new file mode 100644 index 0000000..3ca7215 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyRuntimeConcurrencyTests.cs @@ -0,0 +1,120 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Regression coverage for Driver.AbLegacy-006 — a per-tag libplctag runtime (a single +/// Tag handle) is cached and shared between the server read path and the poll loop. +/// A Tag is not safe for concurrent operations, so the driver must serialise the +/// Read → GetStatus → DecodeValue (and Encode → Write → GetStatus) sequence per runtime. +/// +[Trait("Category", "Unit")] +public sealed class AbLegacyRuntimeConcurrencyTests +{ + /// + /// A fake runtime that records the maximum number of operations in flight against the + /// same handle. If the driver fails to serialise, two callers overlap inside the + /// Read → GetStatus → Decode window and exceeds 1. + /// + private sealed class OverlapDetectingFake : FakeAbLegacyTag + { + private int _inFlight; + public int MaxConcurrent { get; private set; } + + public OverlapDetectingFake(AbLegacyTagCreateParams p) : base(p) { } + + public override async Task ReadAsync(CancellationToken ct) + { + EnterOp(); + try + { + // Yield + small delay so an unserialised second caller is guaranteed to overlap. + await Task.Delay(15, ct).ConfigureAwait(false); + await base.ReadAsync(ct).ConfigureAwait(false); + } + finally { LeaveOp(); } + } + + public override async Task WriteAsync(CancellationToken ct) + { + EnterOp(); + try + { + await Task.Delay(15, ct).ConfigureAwait(false); + await base.WriteAsync(ct).ConfigureAwait(false); + } + finally { LeaveOp(); } + } + + private void EnterOp() + { + var n = Interlocked.Increment(ref _inFlight); + lock (this) { if (n > MaxConcurrent) MaxConcurrent = n; } + } + + private void LeaveOp() => Interlocked.Decrement(ref _inFlight); + } + + [Fact] + public async Task Concurrent_reads_of_same_tag_are_serialised_against_the_shared_runtime() + { + OverlapDetectingFake? shared = null; + var factory = new FakeAbLegacyTagFactory + { + Customise = p => + { + shared = new OverlapDetectingFake(p) { Value = 7 }; + return shared; + }, + }; + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + // Eight callers race for the same tag — mimics the server read path + poll loop(s) + // hitting one cached runtime at once. + var reads = Enumerable.Range(0, 8) + .Select(_ => drv.ReadAsync(["X"], CancellationToken.None)) + .ToArray(); + await Task.WhenAll(reads); + + shared.ShouldNotBeNull(); + shared!.MaxConcurrent.ShouldBe(1, "operations on a shared libplctag Tag must not overlap"); + reads.ShouldAllBe(r => r.Result.Single().Value!.Equals(7)); + } + + [Fact] + public async Task Concurrent_read_and_write_of_same_tag_do_not_overlap() + { + OverlapDetectingFake? shared = null; + var factory = new FakeAbLegacyTagFactory + { + Customise = p => + { + shared = new OverlapDetectingFake(p) { Value = 1 }; + return shared; + }, + }; + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [new AbLegacyTagDefinition("X", "ab://10.0.0.5/1,0", "N7:0", AbLegacyDataType.Int)], + Probe = new AbLegacyProbeOptions { Enabled = false }, + }, "drv-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + + var readTask = drv.ReadAsync(["X"], CancellationToken.None); + var writeTask = drv.WriteAsync([new WriteRequest("X", 99)], CancellationToken.None); + await Task.WhenAll(readTask, writeTask); + + shared.ShouldNotBeNull(); + shared!.MaxConcurrent.ShouldBe(1); + } +}