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

review(Driver.Modbus.Addressing): fix misleading byte-order hint + drop dead overflow guard

Browse Source 
Re-review at 7286d320. -010 (Low): TryParseByteOrder no longer lists REAL/DINT/UINT as type
codes (gave wrong 'field 2' advice -> second parse error); generic byte-order error instead.
-011 (Low): remove unreachable offsetWithinBank>ushort.MaxValue guard (DecodeOctalVAddress
caps at 0xFFFF). + TDD.
This commit is contained in:
Joseph Doherty
2026-06-19 11:34:35 -04:00
parent 6853a0430f
commit b5f6cdfdb9
4 changed files with 161 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Driver.Modbus.Addressing/findings.md
+82 -2
View File
@@ -4,8 +4,8 @@
|---|---|
| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing` |
| Reviewer | Claude Code |
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Review date | 2026-06-19 |
| Commit reviewed | `7286d320` |
| Status | Reviewed |
| Open findings | 0 |
@@ -263,3 +263,83 @@ state precisely that the check is reached only by the 6-digit form in practice,
both for safety rather than relying on the digit-count invariant. Pure documentation change —
no behavioural change; the existing `ModbusModiconAddressTests` already pin the cross-region
5-digit ranges (00001..09999 / 10001..19999 / 30001..39999 / 40001..49999).
## Re-review 2026-06-19 (commit 7286d320)
All nine prior findings (-001 through -009) are Resolved. The diff since `76d35d1` consists
entirely of the fixes for those findings plus XML doc additions. This re-review covers all
10 checklist categories at HEAD for the 7 src files (1227 LOC) and the 4 test files.
#### Re-review checklist
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | Driver.Modbus.Addressing-010 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | No issues found |
| 4 | Error handling & resilience | No issues found |
| 5 | Security | No issues found |
| 6 | Performance & resource management | Driver.Modbus.Addressing-011 |
| 7 | Design-document adherence | No issues found |
| 8 | Code organization & conventions | No issues found |
| 9 | Testing coverage | No issues found |
| 10 | Documentation & comments | No issues found |
### Driver.Modbus.Addressing-010
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `ModbusAddressParser.cs:509` |
| Status | Resolved |
**Description:** `TryParseByteOrder`'s `isKnownTypeCode` heuristic listed `"REAL"`, `"DINT"`,
and `"UINT"` as "known type codes" but those strings are NOT valid type codes in `TryParseType`.
The valid type codes are `BOOL`, `S`, `US`, `I`, `UI`, `I_64`, `UI_64`, `F`, `D`, `BCD`,
`BCD_32`, and `STR<n>`. Producing the hint `"type belongs in field 2 (e.g. '40001:REAL')"` for
`"40001:F:REAL"` directed the user to try `"40001:REAL"`, which would immediately fail with
`"Unknown type code 'REAL'"` — double-misleading for common PLC tool exports (STEP 7 / RSLogix)
that use `REAL`/`DINT`/`UINT` as type names.
**Recommendation:** Restrict `isKnownTypeCode` to only the actual valid 4-letter type codes from
`TryParseType` — `BOOL` and `STR<n>` forms — and emit the generic `"Unknown byte order"` message
for any other token (`REAL`, `DINT`, `UINT`, etc.).
**Resolution:** Resolved 2026-06-19 (SHA blank) — replaced the `isKnownTypeCode` list with a
check against only the actual valid parser type codes (`BOOL` and `STR<n>` forms). `REAL`, `DINT`,
`UINT` now produce the generic `"Unknown byte order ... Valid: ABCD, CDAB, BADC, DCBA"` message.
Added three regression tests (`ByteOrderSlot_NonTypeCode_Strings_Give_Generic_ByteOrder_Error`
for `REAL`, `DINT`, `UINT`) and one confirmation test (`ByteOrderSlot_BOOL_Gives_TypeCode_Hint`)
pinning that `BOOL` still produces the correct helpful hint.
### Driver.Modbus.Addressing-011
| Field | Value |
|---|---|
| Severity | Low |
| Category | Performance & resource management |
| Location | `DirectLogicAddress.cs:121-123` |
| Status | Resolved |
**Description:** `VMemoryToPdu` contained a dead overflow guard:
`if (offsetWithinBank > ushort.MaxValue) throw new OverflowException(...)`. The guard is
provably unreachable: `DecodeOctalVAddress` already caps `octalValue` at `ushort.MaxValue`
(0xFFFF) via its per-digit loop check, so `offsetWithinBank = octalValue - SystemVMemoryOctalBase`
is at most `0xFFFF - 0x4100 = 0xBEFF = 48895`, which is always ≤ `ushort.MaxValue` (65535).
The real overflow for system-bank V-addresses is caught inside `SystemVMemoryToPdu` when
`pdu = 0x2100 + offsetWithinBank > 0xFFFF`, which is reachable when `SystemVMemoryToPdu` is
called directly with a large offset (≥ 0xDF00) — but not from `VMemoryToPdu` via normal
V-address parsing. The dead branch left a misleading `throw` in place that could confuse a
future reader into thinking the outer check was load-bearing.
**Recommendation:** Remove the dead guard and replace with a comment explaining why the overflow
is provably not reachable from this path; or cast `offsetWithinBank` directly to `ushort` to
make the unreachability obvious.
**Resolution:** Resolved 2026-06-19 (SHA blank) — removed the dead `if (offsetWithinBank > ushort.MaxValue)`
guard, cast `offsetWithinBank` directly to `ushort` (which is always safe given the `DecodeOctalVAddress`
ceiling proof), and added a comment at the call site explaining why the check is unnecessary.
Added regression test `VMemoryToPdu_max_system_bank_address_maps_correctly` confirming that the
maximum valid system-bank V-address (`V177777`, octal 0xFFFF) maps to the correct PDU offset
without error or overflow.