From b5f6cdfdb98cb4406eae7f0c2cba29af26767796 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 11:34:35 -0400 Subject: [PATCH] review(Driver.Modbus.Addressing): fix misleading byte-order hint + drop dead overflow guard 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. --- .../Driver.Modbus.Addressing/findings.md | 84 ++++++++++++++++++- .../DirectLogicAddress.cs | 11 +-- .../ModbusAddressParser.cs | 19 +++-- .../ModbusAddressEdgeCaseTests.cs | 61 ++++++++++++++ 4 files changed, 161 insertions(+), 14 deletions(-) diff --git a/code-reviews/Driver.Modbus.Addressing/findings.md b/code-reviews/Driver.Modbus.Addressing/findings.md index 2bb822d5..deb555d3 100644 --- a/code-reviews/Driver.Modbus.Addressing/findings.md +++ b/code-reviews/Driver.Modbus.Addressing/findings.md @@ -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`. 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` 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` 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. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/DirectLogicAddress.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/DirectLogicAddress.cs index 88c73903..7bdc8e25 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/DirectLogicAddress.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/DirectLogicAddress.cs @@ -117,11 +117,12 @@ public static class DirectLogicAddress // System bank: the registers are contiguous from V40400, so the offset within the bank // is the plain decimal distance from the octal base, not another octal decode. - var offsetWithinBank = octalValue - SystemVMemoryOctalBase; - if (offsetWithinBank > ushort.MaxValue) - throw new OverflowException( - $"V-memory address '{vAddress}' is outside the addressable system bank"); - return SystemVMemoryToPdu((ushort)offsetWithinBank); + // Driver.Modbus.Addressing-011: the subtraction result is provably <= 0xBEFF because + // DecodeOctalVAddress already caps octalValue at 0xFFFF, so no overflow guard is needed + // here. The real overflow guard (pdu > ushort.MaxValue) lives in SystemVMemoryToPdu and + // is reachable only when that helper is called directly with a large explicit offset. + var offsetWithinBank = (ushort)(octalValue - SystemVMemoryOctalBase); + return SystemVMemoryToPdu(offsetWithinBank); } // Bit-memory bases per DL260 user manual §I/O-configuration. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusAddressParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusAddressParser.cs index b1f513b8..7c3aaf2e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusAddressParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing/ModbusAddressParser.cs @@ -503,13 +503,18 @@ public static class ModbusAddressParser if ((int)order == -1) { - // Driver.Modbus.Addressing-003: if the unknown token looks like a known type code - // (a 4-letter alphanumeric token that matches one of the recognised type strings), - // produce a diagnostic that directs the user to put the type in field 2, not field 3. - var isKnownTypeCode = text.ToUpperInvariant() is "BOOL" or "REAL" or "DINT" or "UINT" - || (text.Length <= 6 && text.StartsWith("STR", StringComparison.OrdinalIgnoreCase)); - error = isKnownTypeCode - ? $"'{text}' looks like a type code; type belongs in field 2 (e.g. '40001:{text.ToUpperInvariant()}'), not field 3. Field 3 must be a 4-letter byte order (ABCD/CDAB/BADC/DCBA)" + // Driver.Modbus.Addressing-003: if the unknown token is a VALID type code in this + // parser, direct the user to put it in field 2 instead. Only BOOL (4-letter) and + // STR are valid type codes that could appear here. "REAL", "DINT", "UINT" look + // like type names from other tools (STEP 7 / RSLogix) but are NOT valid in this + // parser's type table (Driver.Modbus.Addressing-010) — listing them here would give + // advice that leads to a second error ("Unknown type code 'REAL'"). + var upper = text.ToUpperInvariant(); + var isActualTypeCode = upper is "BOOL" + || (text.Length > 3 && text.StartsWith("STR", StringComparison.OrdinalIgnoreCase) + && text[3..].All(char.IsDigit)); + error = isActualTypeCode + ? $"'{text}' looks like a type code; type belongs in field 2 (e.g. '40001:{upper}'), not field 3. Field 3 must be a 4-letter byte order (ABCD/CDAB/BADC/DCBA)" : $"Unknown byte order '{text}'. Valid: ABCD, CDAB, BADC, DCBA"; return false; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/ModbusAddressEdgeCaseTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/ModbusAddressEdgeCaseTests.cs index fc8d9a1e..cc30c773 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/ModbusAddressEdgeCaseTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/ModbusAddressEdgeCaseTests.cs @@ -256,4 +256,65 @@ public sealed class ModbusAddressEdgeCaseTests error.ShouldNotBeNullOrEmpty(); error!.ShouldContain("byte order", Case.Insensitive); } + + // ── Driver.Modbus.Addressing-010: isKnownTypeCode lists non-type-codes (REAL/DINT/UINT) ─ + // + // TryParseByteOrder's isKnownTypeCode heuristic listed "REAL", "DINT", and "UINT" as + // "known type codes" but those strings are NOT valid type codes in TryParseType. Following + // the advice "type belongs in field 2 (e.g. '40001:REAL')" would lead to a second error + // "Unknown type code 'REAL'", misdirecting the user. The list must be restricted to the + // actual 4-letter valid type code: BOOL. + + /// Verifies that REAL in the byte-order slot gives a generic byte-order error, not false type-code advice. + [Theory] + [InlineData("40001:F:REAL")] + [InlineData("40001:F:DINT")] + [InlineData("40001:F:UINT")] + public void ByteOrderSlot_NonTypeCode_Strings_Give_Generic_ByteOrder_Error(string addr) + { + // "REAL", "DINT", "UINT" look like PLC type names but are NOT valid type codes in this + // parser. They should produce the generic "Unknown byte order" message, not a misleading + // "type belongs in field 2" hint that would lead the user to another failure. + var ok = ModbusAddressParser.TryParse(addr, out _, out var error); + ok.ShouldBeFalse(); + error.ShouldNotBeNullOrEmpty(); + // Must NOT claim these are type codes — following that advice produces another error. + error!.ShouldNotContain("type code", Case.Insensitive); + // Must mention the valid byte orders so the user knows what field 3 accepts. + error.ShouldContain("ABCD", Case.Insensitive); + } + + /// Verifies that BOOL in the byte-order slot still gives the helpful type-code hint. + [Fact] + public void ByteOrderSlot_BOOL_Gives_TypeCode_Hint() + { + // BOOL IS a valid type code — the hint "type belongs in field 2 (e.g. '40001:BOOL')" + // is correct advice since '40001:BOOL' does parse successfully. + var ok = ModbusAddressParser.TryParse("40001:F:BOOL", out _, out var error); + ok.ShouldBeFalse(); + error.ShouldNotBeNullOrEmpty(); + error!.ShouldContain("field 2", Case.Insensitive); + } + + // ── Driver.Modbus.Addressing-011: dead overflow check in VMemoryToPdu ────────────────── + // + // VMemoryToPdu's "offsetWithinBank > ushort.MaxValue" guard is unreachable: DecodeOctalVAddress + // already caps octalValue at ushort.MaxValue (0xFFFF), so offsetWithinBank can never exceed + // 0xFFFF - SystemVMemoryOctalBase (0x4100) = 0xBEFF = 48895, which is always < ushort.MaxValue. + // The real overflow guard lives in SystemVMemoryToPdu (pdu > ushort.MaxValue) and is reachable + // when SystemVMemoryToPdu is called directly with a large offset. These tests pin the boundary + // and confirm the overflow is caught by SystemVMemoryToPdu, not the outer check. + + /// Verifies that VMemoryToPdu correctly maps the last valid system-bank V-address. + [Fact] + public void VMemoryToPdu_max_system_bank_address_maps_correctly() + { + // The largest V-address whose octal-decoded value fits in ushort (0xFFFF = 65535 octal) is + // V177777 (octal). octalValue = 0xFFFF. offsetWithinBank = 0xFFFF - 0x4100 = 0xBEFF. + // pdu = 0x2100 + 0xBEFF = 0xDFFF = 57343 which is < 0xFFFF — does not overflow. + // The "offsetWithinBank > ushort.MaxValue" check in VMemoryToPdu never fires for V-addresses + // reachable through the parser (DecodeOctalVAddress caps at 0xFFFF). + var result = DirectLogicAddress.VMemoryToPdu("V177777"); + result.ShouldBe((ushort)(DirectLogicAddress.SystemVMemoryBasePdu + (0xFFFF - DirectLogicAddress.SystemVMemoryOctalBase))); + } }