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

fix(driver-abcip): resolve Medium code-review finding (Driver.AbCip-005)

Browse Source 
Structure tags with declared Members no longer register the bare parent
name in `_tagsByName` — reading it would return Good/null, which is
misleading. Clients read individual member paths. Both the member
fan-out and the scalar-tag paths now perform a duplicate-key check that
throws `InvalidOperationException` naming both colliding entries (fail-
fast, consistent with the AbCipHostAddress validation pattern).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 09:20:22 -04:00
parent 1722c0328b
commit d5b8c802ce
2 changed files with 26 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
code-reviews/Driver.AbCip/findings.md
View File

@@ -93,13 +93,13 @@
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `AbCipDriver.cs:124-141` |
| Status | Open |
| Status | Resolved |
**Description:** In `InitializeAsync`, when a `Structure` tag declares `Members`, the loop registers each fanned-out member into `_tagsByName` but the parent Structure tag itself is also left in `_tagsByName` (added at line 125 before the member check). A subsequent `ReadAsync` of the parent name routes through `ReadSingleAsync` then `DecodeValue(AbCipDataType.Structure, ...)` which returns `null` with `Good` status. A client reading the parent UDT node thus gets a Good/null snapshot rather than a fault or a structured value. Also, member registration does not check for name collisions: if two configured tags produce the same parent-dot-member key (or a member name collides with an independently-declared tag), the later silently overwrites the earlier with no diagnostic.
**Recommendation:** Decide the parent-Structure read contract explicitly: either do not register the bare parent name as a readable tag, or have the Structure read return a proper status. Add a duplicate-key check during `_tagsByName` population that throws an `InvalidOperationException` naming both colliding tags, consistent with the fail-fast validation `AbCipHostAddress` parsing already does.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — Structure tags with declared members are no longer added to `_tagsByName` under the bare parent name; clients must address individual member paths. Both member fan-out and scalar tag registration now perform a duplicate-key check that throws `InvalidOperationException` naming both colliding tags (fail-fast, consistent with `AbCipHostAddress` parsing).
### Driver.AbCip-006

25
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs
View File

@@ -144,7 +144,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
}
foreach (var tag in _options.Tags)
{
_tagsByName[tag.Name] = tag;
// Driver.AbCip-005: Structure tags whose Members are declared fan out to per-member
// entries only. The bare parent name is deliberately NOT registered as a readable
// tag — reading it would return Good/null (DecodeValue for Structure returns null),
// which is misleading. Clients read the individual member paths instead.
if (tag.DataType == AbCipDataType.Structure && tag.Members is { Count: > 0 })
{
foreach (var member in tag.Members)
@@ -156,9 +159,29 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
DataType: member.DataType,
Writable: member.Writable,
WriteIdempotent: member.WriteIdempotent);
// Duplicate-key check: a collision means two configured tags produce the
// same readable path (e.g. two structure parents with the same member name,
// or a member name that matches an independently-declared tag). Fail fast
// at init time with a diagnostic rather than silently clobbering.
if (_tagsByName.TryGetValue(memberTag.Name, out var existing))
throw new InvalidOperationException(
$"AbCip tag name collision: '{memberTag.Name}' is produced by both " +
$"'{tag.Name}.{member.Name}' (member fan-out) and an existing tag " +
$"'{existing.Name}'. Rename one of the configured tags to resolve.");
_tagsByName[memberTag.Name] = memberTag;
}
}
else
{
// Non-structure tag (or Structure with no declared members — kept readable
// so the whole-UDT path or declaration-only grouping can serve it).
if (_tagsByName.TryGetValue(tag.Name, out var existing))
throw new InvalidOperationException(
$"AbCip tag name collision: '{tag.Name}' is declared more than once. " +
$"Existing entry DeviceHostAddress='{existing.DeviceHostAddress}', " +
$"TagPath='{existing.TagPath}'. Rename or remove the duplicate.");
_tagsByName[tag.Name] = tag;
}
}
// Probe loops — one per device when enabled + a ProbeTagPath is configured.