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

fix(driver-ablegacy): resolve Medium code-review finding (Driver.AbLegacy-008)

Browse Source 
Mark _health volatile. The record-reference assignment is atomic, but
without an acquire/release memory barrier GetHealth() on another thread
can observe a stale snapshot indefinitely. volatile enforces the barrier
at read and write sites without a lock.

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

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

@@ -218,7 +218,7 @@ runtime of any race is disposed.
| Severity | Medium | | Severity | Medium |
| Category | Concurrency & thread safety | | Category | Concurrency & thread safety |
| Location | `AbLegacyDriver.cs:21`, `AbLegacyDriver.cs:138-146`, `AbLegacyDriver.cs:216-229` | | Location | `AbLegacyDriver.cs:21`, `AbLegacyDriver.cs:138-146`, `AbLegacyDriver.cs:216-229` |
| Status | Open | | Status | Resolved |
**Description:** `_health` is a plain non-volatile reference field mutated from **Description:** `_health` is a plain non-volatile reference field mutated from
`ReadAsync`, `WriteAsync` (both can run on multiple threads / poll loops) and `ReadAsync`, `WriteAsync` (both can run on multiple threads / poll loops) and
@@ -233,7 +233,7 @@ successful read can clobber a `Degraded` write from a concurrent failing read.
lock / `Interlocked.Exchange`. Consider only downgrading on failure and upgrading on a lock / `Interlocked.Exchange`. Consider only downgrading on failure and upgrading on a
successful poll so a single failed read does not flap the surface. successful poll so a single failed read does not flap the surface.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-22 — `_health` marked `volatile`; memory barrier comment documents the acquire/release ordering guarantee.
### Driver.AbLegacy-009 ### Driver.AbLegacy-009

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

@@ -17,7 +17,13 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
private readonly PollGroupEngine _poll; private readonly PollGroupEngine _poll;
private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, DeviceState> _devices = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, AbLegacyTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary<string, AbLegacyTagDefinition> _tagsByName = new(StringComparer.OrdinalIgnoreCase);
private DriverHealth _health = new(DriverState.Unknown, null, null);
// volatile: _health is read by GetHealth() on any thread while ReadAsync / WriteAsync /
// InitializeAsync write it from worker / poll threads. The record-reference assignment is
// atomic on all .NET platforms, but without a memory barrier a reader can see a stale
// snapshot indefinitely. volatile enforces acquire/release ordering so GetHealth() always
// observes the most recently written value.
private volatile DriverHealth _health = new(DriverState.Unknown, null, null);
public event EventHandler<DataChangeEventArgs>? OnDataChange; public event EventHandler<DataChangeEventArgs>? OnDataChange;
public event EventHandler<HostStatusChangedEventArgs>? OnHostStatusChanged; public event EventHandler<HostStatusChangedEventArgs>? OnHostStatusChanged;