dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(driver-ablegacy): resolve High code-review findings (Driver.AbLegacy-001, Driver.AbLegacy-006)

Browse Source 
Driver.AbLegacy-001 — PCCC bit-index range. AbLegacyAddress.TryParse
accepted a bit index of 0..31 for every file type, but a 16-bit
N/B/I/O/S/A word only has bits 0..15. TryParse now range-checks the
bit index against the file's word width (0..15 for 16-bit element
files, 0..31 for the 32-bit L file, no bits on float files), so
addresses like N7:0/20 are rejected at parse time instead of silently
truncating in the (short) cast. WriteBitInWordAsync reads and writes
an L-file parent word as 32-bit Long and masks the RMW arithmetic to
the native width, so a sign-extended 16-bit decode can no longer
corrupt the high bits.

Driver.AbLegacy-006 — shared-runtime concurrency. A per-tag libplctag
Tag handle is cached and reused by both the server read path and the
poll loop, with no synchronisation around Read/GetStatus/DecodeValue.
Added a per-runtime SemaphoreSlim (DeviceState.GetRuntimeLock, keyed
by tag name); ReadAsync and WriteAsync now hold it across the whole
Read -> GetStatus -> Decode / Encode -> Write -> GetStatus sequence so
no two threads touch the same Tag handle concurrently.

Added xUnit + Shouldly regression coverage: AbLegacyBitIndexRangeTests
(per-file bit-range validation + L-file 32-bit RMW + sign-extension
safety) and AbLegacyRuntimeConcurrencyTests (overlap-detecting fake
proving concurrent read/read and read/write are serialised).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 06:33:10 -04:00
parent 8a7668c678
commit d89be2a011
5 changed files with 334 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyAddress.cs
View File

@@ -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);
}
/// <summary>
/// Highest valid bit index for a file letter, or <c>-1</c> 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.
/// </summary>
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,

85
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs
View File

@@ -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
}
/// <summary>
/// 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 <see cref="SemaphoreSlim"/>.
/// 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
/// <see cref="SemaphoreSlim"/>. The parent word is masked to its native width before the
/// bit operation so a sign-extended decode never corrupts the high bits.
/// </summary>
private async Task<uint> 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<string, SemaphoreSlim> _runtimeLocks = new();
/// <summary>
/// Per-runtime operation lock. A libplctag <c>Tag</c> handle is not safe for
/// concurrent Read/GetStatus/DecodeValue from multiple threads — the server read
/// path and the poll loop both call <see cref="AbLegacyDriver.ReadAsync"/> 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 <see cref="Runtimes"/> dictionary key.
/// </summary>
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;