Joseph Doherty
b5f6cdfdb9
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.
19 KiB
Code Review — Driver.Modbus.Addressing
| Field | Value |
|---|---|
| Module | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing |
| Reviewer | Claude Code |
| Review date | 2026-06-19 |
| Commit reviewed | 7286d320 |
| Status | Reviewed |
| Open findings | 0 |
Checklist coverage
A comprehensive review completes every category, recording "No issues found" where a category produced nothing rather than leaving it blank.
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | Driver.Modbus.Addressing-001, -002, -003, -004 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | No issues found |
| 4 | Error handling & resilience | Driver.Modbus.Addressing-005, -006 |
| 5 | Security | No issues found |
| 6 | Performance & resource management | No issues found |
| 7 | Design-document adherence | Driver.Modbus.Addressing-001, -007 |
| 8 | Code organization & conventions | No issues found |
| 9 | Testing coverage | Driver.Modbus.Addressing-008 |
| 10 | Documentation & comments | Driver.Modbus.Addressing-009 |
Findings
Driver.Modbus.Addressing-001
| Field | Value |
|---|---|
| Severity | High |
| Category | Correctness & logic bugs |
| Location | ModbusAddressParser.cs:230-235, DirectLogicAddress.cs:66-73 |
| Status | Resolved |
Description: The DL205 family-native branch routes every V-prefixed address through
DirectLogicAddress.UserVMemoryToPdu, which is a plain octal-to-decimal conversion. DL205/DL260
system V-memory (V40400 and up) is NOT a simple octal decode — per docs/v2/dl205.md section
V-Memory, V40400 must map to Modbus PDU 0x2100 (decimal 8448) on a factory-mode ECOM module.
The parser instead octal-decodes V40400 to decimal 16640 (0x4100), the wrong register. The
DirectLogicAddress.SystemVMemoryToPdu / SystemVMemoryBasePdu helper that exists to do this
correctly is never called by the parser — it is dead code from the parser point of view. A tag
spreadsheet that addresses any DL system register through the grammar string silently reads and
writes the wrong PLC memory. The companion test ModbusFamilyParserTests.cs:20 bakes the wrong
value (V40400 to 16640) into a passing assertion, so the regression is locked in.
Recommendation: Make the DL205 V branch detect the system bank (octal address >= 40400) and
route it through SystemVMemoryToPdu, or explicitly reject system V-memory in the grammar string
with a diagnostic pointing at the structured tag form. Either way, fix the V40400 test to assert
the corrected mapping.
Resolution: Resolved 2026-05-22 — added DirectLogicAddress.VMemoryToPdu, which detects the
system bank (octal >= V40400) and relocates it through SystemVMemoryToPdu to PDU 0x2100; the
DL205 V branch in ModbusAddressParser now calls it, and the ModbusFamilyParserTests V40400
assertion was corrected from 16640 to 0x2100 with system-bank regression cases added.
Driver.Modbus.Addressing-002
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | ModbusAddressParser.cs:86-94 |
| Status | Resolved |
Description: In the 3-field disambiguation, an empty 3rd field (40001:F:) reaches
parts[2].All(char.IsDigit). Enumerable.All returns true for an empty sequence, so the empty
string is classified as a valid-shaped array count, assigned to countPart, then silently dropped
by the later string.IsNullOrEmpty(countPart) guard. The result is that 40001:F: parses
successfully as a plain scalar with a dangling empty field rather than being rejected as
malformed. The 4-field form 40001:F:: has the analogous effect. A user who mistypes a trailing
colon gets no diagnostic.
Recommendation: Reject an empty 3rd field explicitly, or guard the All(char.IsDigit) branch
with parts[2].Length > 0.
Resolution: Resolved 2026-05-22 — added an explicit parts[2].Length == 0 check before the All(char.IsDigit) branch that returns a descriptive error, so a trailing colon typo produces a diagnostic instead of silently parsing as a scalar.
Driver.Modbus.Addressing-003
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | ModbusAddressParser.cs:405-406, ModbusAddressParser.cs:128 |
| Status | Resolved |
Description: LooksLikeByteOrderToken classifies any 4-letter token as a byte-order token.
A 3-field address whose 3rd field is a 4-letter type-like token (e.g. 40001:S:BOOL) is routed
into TryParseByteOrder, producing the misleading diagnostic "Unknown byte order BOOL" instead
of telling the user the type belongs in field 2. The type code BOOL is exactly 4 letters and
could only ever be intended as a type — the shape heuristic cannot tell a mistyped type from a
byte order, so the diagnostic actively misdirects.
Recommendation: When TryParseByteOrder fails on a 4-letter token in the 3-field form, widen
the error message to mention that field 3 is a byte order and field 2 is the type, or attempt a
type-parse fallback before emitting the byte-order error.
Resolution: Resolved 2026-05-22 — in the 3-field disambiguation error path, a 4-letter alphanumeric token that looks like a type code now produces a diagnostic explicitly stating that field 3 is the byte-order slot and field 2 is the type slot, directing the user to the correct fix.
Driver.Modbus.Addressing-004
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | ModbusAddressParser.cs:182-194 |
| Status | Resolved |
Description: The bit suffix is stripped using text.IndexOf('.') — the first dot. An input
such as 40001.5.3 produces a bit text of "5.3", rejected by byte.TryParse with the generic
"Bit index must be 0..15" message. A Modicon-style decimal-point typo like 400.01 is silently
treated as region/offset 400 plus bit 01; 400 then fails Modicon length validation, so the
surfaced error is the Modicon length diagnostic rather than a bit-index diagnostic, because the
bit was parsed first and 01 is a valid bit. The dot-handling assumes a single dot without
asserting it, and the diagnostics for these malformed inputs are inconsistent.
Recommendation: Use LastIndexOf('.') or assert exactly one dot, and validate that the
region/offset segment is non-empty and dot-free after the strip so malformed inputs get a precise
diagnostic.
Resolution: Resolved 2026-05-22 — switched to LastIndexOf('.'), added a non-empty guard for the address segment before the dot, and added a check that the address segment itself contains no dot (diagnosing multi-dot inputs with "contains multiple dots" rather than a confusing bit-index error).
Driver.Modbus.Addressing-005
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Error handling & resilience |
| Location | ModbusAddressParser.cs:200-213 |
| Status | Resolved |
Description: TryParseRegionAndOffset tries family-native, then mnemonic, then Modicon. When
all three fail it returns false with whatever error the Modicon parser last wrote (comment: "the
Modicon error is the more specific diagnostic"). For a non-Generic family this is misleading:
TryParseFamilyNative returns false with error left null for any address that does not start with
a recognised family prefix, and even for recognised prefixes it only sets error inside the catch.
The subsequent mnemonic and Modicon attempts overwrite error. Net effect: a clearly
family-native-shaped input that fails deep in the family helper can still surface a generic
Modicon "must be 5 or 6 digits" error, hiding the real cause (e.g. "contains non-octal digit").
Recommendation: When a non-Generic family is configured and the input matches a family prefix, prefer and preserve the family-native error rather than letting the Modicon fallback overwrite it.
Resolution: Resolved 2026-05-22 — the family-native error is now captured in familyNativeError and, after all three branches fail, preferred over the Modicon fallback error when it is non-null (indicating the address matched a family prefix but failed deep inside the helper).
Driver.Modbus.Addressing-006
| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | ModbusAddressParser.cs:297-301 |
| Status | Resolved |
Description: TryParseFamilyNative catches only ArgumentException and OverflowException.
The current helpers throw only those (including ArgumentOutOfRangeException, which derives from
ArgumentException), so today it is correct. But the parser intent is to convert helper
exceptions into structured errors; any future helper change that throws a different exception type
(e.g. a FormatException from a ushort.Parse swap) would escape as an unhandled exception out
of a TryParse method, violating the try-parse contract that config-bind hot-path callers
depend on.
Recommendation: Either document the exact exception contract of the helpers and keep the narrow catch, or broaden to a general catch-all that records the message — a try-parse method should never throw.
Resolution: Resolved 2026-05-23 — broadened the catch filter in
ModbusAddressParser.TryParseFamilyNative from ArgumentException or OverflowException to a
general catch (Exception ex) so any future helper exception type is converted to a structured
(false, error) rather than escaping the TryParse method. Added DL205_TryParse_NeverThrows
and MELSEC_TryParse_NeverThrows parameterised regression tests in
ModbusAddressEdgeCaseTests covering ~20 pathological inputs (empty prefixes, octal/hex digit
violations, overflow inputs, unknown prefixes) to pin the defensive contract.
Driver.Modbus.Addressing-007
| Field | Value |
|---|---|
| Severity | Low |
| Category | Design-document adherence |
| Location | ModbusDataType.cs:91-95, docs/v2/dl205.md section Strings |
| Status | Resolved |
Description: ModbusStringByteOrder (HighByteFirst / LowByteFirst) is defined in this
assembly and documented as the DL205 low-byte-first string-packing knob, but ParsedModbusAddress
has no field for it and ModbusAddressParser never produces or consumes it. The STR<n> grammar
form cannot express the DL205 string byte order described in docs/v2/dl205.md — a DL205 string
tag parsed from the grammar string always carries the default order. The enum is effectively
unreachable from the parser, so the grammar cannot represent a known, documented device quirk.
Recommendation: Either add a StringByteOrder field to ParsedModbusAddress plus a grammar
token for it, or document explicitly that DL205 string byte order is only configurable via the
structured tag form and is intentionally out of grammar scope.
Resolution: Resolved 2026-05-23 — chose the "document the limitation" branch of the
recommendation rather than adding a grammar token: the 3rd field slot is the multi-register
word/byte order and the 4th is the array count, so a 5th :<order> suffix would conflict with
the existing count-shape disambiguation; ModbusStringByteOrder is already plumbed through the
structured tag form (ModbusDriverFactoryExtensions.ModbusTagDto.StringByteOrder →
ModbusTagDefinition.StringByteOrder) which is the canonical config path. Added an explicit
"Grammar scope" remarks block to ModbusStringByteOrder and to the ModbusAddressParser
<remarks> block stating that string byte order is configurable only via the structured tag
form. Added a corresponding bullet to docs/v2/dl205.md §Strings. Added two regression tests
(Parser_STR_grammar_does_not_carry_StringByteOrder reflecting on ParsedModbusAddress, and
Parser_rejects_unknown_string_byte_order_token_in_grammar) pinning the contract so a future
grammar change can't quietly add a conflicting token.
Driver.Modbus.Addressing-008
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Testing coverage |
| Location | tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Addressing.Tests/ |
| Status | Resolved |
Description: Several edge cases of the address arithmetic are untested or asserted wrong:
(a) DL205 system V-memory mapping is tested only with the incorrect expected value
(ModbusFamilyParserTests.cs:20, see finding -001); (b) there is no test for UserVMemoryToPdu
or AddOctalOffset overflow (V200000, C200000) hitting the OverflowException path; (c) no test
for the empty-trailing-field cases of finding -002; (d) MelsecAddress.ParseHex overflow and
DRegisterToHolding / MRelayToCoil bank-base overflow are untested; (e) no test that
SystemVMemoryToPdu is exercised at all. The address-arithmetic overflow and off-by-one paths
are exactly the high-risk surface this module owns, and they are the least covered.
Recommendation: Add overflow/boundary tests for every PDU/coil/discrete translation helper and for the parser count/bit/field edge cases. Correct the V40400 assertion as part of fixing finding -001.
Resolution: Resolved 2026-05-22 — added ModbusAddressEdgeCaseTests.cs covering: empty 3rd-field rejection, multi-dot input rejection, UserVMemoryToPdu overflow, AddOctalOffset overflow via Y and C helpers, SystemVMemoryToPdu base/overflow, MelsecAddress.ParseHex overflow, DRegisterToHolding and MRelayToCoil bank-base overflow.
Driver.Modbus.Addressing-009
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | ModbusModiconAddress.cs:55-64, ModbusModiconAddress.cs:104-110 |
| Status | Resolved |
Description: The comments on ModbusModiconAddress.TryParse are slightly inaccurate. The
remark that 5-digit Modicon is always exactly 5 chars (40001..49999) and 6-digit is exactly 6
(400001..465536-shaped) implies the leading digit is always 4, but the parser accepts leading
0/1/3 too — a 5-digit coil is 00001..09999, not 40001..49999. Separately, the line-106 comment
says the 5-digit form caps at 9999 by construction while the adjacent code path applies the same
> 65536 check to both forms; the comment describes an invariant the code does not rely on.
Recommendation: Reword the range examples to cover all four region digits and drop the caps-at-9999 aside or restate it as a precise statement about trailing-digit count.
Resolution: Resolved 2026-05-23 — reworded the up-front range-check comment to describe all
four region digits (0/1/3/4) and give examples covering each region (coils 00001..09999 /
000001..065536, holding registers 40001..49999 / 400001..465536). Reworded the lower
> 65536 comment to drop the misleading "5-digit form caps at 9999 by construction" framing and
state precisely that the check is reached only by the 6-digit form in practice, but applied to
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.