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-009)

Browse Source 
The ConcurrentDictionary + TryAdd/dispose-loser pattern for Runtimes
and ParentRuntimes was already applied as part of the Driver.AbCip-008
fix. Recording resolution with evidence rather than applying a
duplicate change.

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

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

@@ -153,13 +153,13 @@
| Severity | Medium | | Severity | Medium |
| Category | Concurrency & thread safety | | Category | Concurrency & thread safety |
| Location | `AbCipDriver.cs:621-648`, `AbCipDriver.cs:591-614` | | Location | `AbCipDriver.cs:621-648`, `AbCipDriver.cs:591-614` |
| Status | Open | | Status | Resolved |
**Description:** `EnsureTagRuntimeAsync` and `EnsureParentRuntimeAsync` are check-then-act on a non-thread-safe `Dictionary` (`device.Runtimes` / `device.ParentRuntimes`). `ReadAsync` is `IReadable` and may be invoked concurrently: the server read path, each polled subscription loop, and the alarm projection poll loop all call `ReadAsync` independently. Two concurrent `ReadAsync` calls that both miss the cache for the same tag both create a `LibplctagTagRuntime`, both initialize it, and both write into the dictionary; the loser leaks an initialized native tag (never disposed, since only the dictionary value is disposed at shutdown), and concurrent `Dictionary` mutation can throw or corrupt the bucket structure. `WriteBitInDIntAsync` serializes the parent via a per-parent `SemaphoreSlim`, but `EnsureParentRuntimeAsync` still runs the same unguarded check-then-act on the shared `ParentRuntimes` dict. **Description:** `EnsureTagRuntimeAsync` and `EnsureParentRuntimeAsync` are check-then-act on a non-thread-safe `Dictionary` (`device.Runtimes` / `device.ParentRuntimes`). `ReadAsync` is `IReadable` and may be invoked concurrently: the server read path, each polled subscription loop, and the alarm projection poll loop all call `ReadAsync` independently. Two concurrent `ReadAsync` calls that both miss the cache for the same tag both create a `LibplctagTagRuntime`, both initialize it, and both write into the dictionary; the loser leaks an initialized native tag (never disposed, since only the dictionary value is disposed at shutdown), and concurrent `Dictionary` mutation can throw or corrupt the bucket structure. `WriteBitInDIntAsync` serializes the parent via a per-parent `SemaphoreSlim`, but `EnsureParentRuntimeAsync` still runs the same unguarded check-then-act on the shared `ParentRuntimes` dict.
**Recommendation:** Use `ConcurrentDictionary` for `Runtimes` and `ParentRuntimes`, creating the runtime via `GetOrAdd` with a lazily-initialized factory, or guard the ensure path with a per-device lock / `SemaphoreSlim`. Ensure the losing creator runtime is disposed rather than leaked. **Recommendation:** Use `ConcurrentDictionary` for `Runtimes` and `ParentRuntimes`, creating the runtime via `GetOrAdd` with a lazily-initialized factory, or guard the ensure path with a per-device lock / `SemaphoreSlim`. Ensure the losing creator runtime is disposed rather than leaked.
**Resolution:** _(open)_ **Resolution:** Resolved 2026-05-22 — already addressed as part of the Driver.AbCip-008 fix: `DeviceState.Runtimes` and `ParentRuntimes` were switched to `ConcurrentDictionary`; both `EnsureTagRuntimeAsync` and `EnsureParentRuntimeAsync` use the `TryGetValue` → create → `TryAdd` → dispose-loser pattern so concurrent callers that both miss the cache produce exactly one live runtime and the losing creator is disposed rather than leaked.
### Driver.AbCip-010 ### Driver.AbCip-010