review(Driver.Modbus.Contracts): first review; guard equipment-tag bitIndex/stringLength
First review at 7286d320. -001 (Medium): reject bitIndex>15 for BitInRegister equipment tags
(was silent always-false/no-op via the (1<<16) mask). -002 (Medium): reject stringLength<1
for String (was misleading BadCommunicationError). -003/-004 cap-doc + [Range(0,2000)] on
MaxCoilsPerRead. Consuming Driver.Modbus.Tests green.
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,97 @@
|
||||
# Code Review — Driver.Modbus.Contracts
|
||||
|
||||
<!-- Per-module findings file. See ../../REVIEW-PROCESS.md for the full process.
|
||||
The base README.md is generated from these files by regen-readme.py — do not
|
||||
edit README.md by hand. -->
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts` |
|
||||
| Reviewer | Claude Code |
|
||||
| Review date | 2026-06-19 |
|
||||
| Commit reviewed | `a19b0f86` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
| # | Category | Result |
|
||||
|---|---|---|
|
||||
| 1 | Correctness & logic bugs | 2 findings (001, 002) — both fixed in this review |
|
||||
| 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 | No issues found |
|
||||
| 7 | Design-document adherence | No issues found |
|
||||
| 8 | Code organization & conventions | No issues found |
|
||||
| 9 | Testing coverage | No dedicated test project. The pre-declared-tag parse path is covered in Driver.Modbus.Tests; the equipment-tag TryParse path has no unit tests — deferred. |
|
||||
| 10 | Documentation & comments | 2 findings (003, 004) — both fixed in this review |
|
||||
|
||||
## Findings
|
||||
|
||||
### Driver.Modbus.Contracts-001
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Medium |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs:35` |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** TryParse cast the JSON "bitIndex" value to byte with no range check. ModbusTagDefinition documents the valid range as 0-15 (LSB-first within a 16-bit register). When a user typed a BitIndex >= 16 in the AdminUI numeric input, the byte was passed unmodified to the driver. ModbusDriver computes (raw & (1 << tag.BitIndex)) != 0 for reads and (ushort)(current | (1 << bit)) for writes. Because C# int shifts are modulo-32, 1 << 16 = 0x10000; masking against a ushort read value always produces 0 (read always returns false) and the write OR has no effect on the register. Correct data flow is silently lost for any BitInRegister equipment tag whose bitIndex is 16 or greater. ModbusTagConfigModel.Validate() returned null unconditionally so the AdminUI save path did not gate this. (The grammar parser ModbusAddressParser.TryParse correctly rejects bit indices > 15 on line 247, but equipment tags bypass the grammar parser.)
|
||||
|
||||
**Recommendation:** Add a range guard in TryParse immediately after reading bitIndex: `if (dataType == ModbusDataType.BitInRegister && bitIndex > 15) return false;`
|
||||
|
||||
**Resolution:** Fixed in this review (2026-06-19). Added guard in ModbusEquipmentTagParser.TryParse after the bitIndex read. The parser now returns false (BadNodeIdUnknown at the driver) rather than silently producing a broken definition. Build verified (0 errors, 0 warnings).
|
||||
|
||||
---
|
||||
|
||||
### Driver.Modbus.Contracts-002
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Medium |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs:36` |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** TryParse read "stringLength" via ReadInt (returning 0 when absent) and constructed a ModbusTagDefinition with StringLength = 0 even when DataType = String. No equivalent of ModbusDriverFactoryExtensions.ValidateStringLength was applied on the equipment-tag path. At runtime RegisterCount computes (0 + 1) / 2 = 0 (integer division), the driver issues a 0-register FC03/FC04 PDU, the PLC responds with Modbus exception 03 (Illegal Data Value), and the tag surfaces a misleading BadCommunicationError OPC UA status. ModbusTagConfigModel.Validate() returned null unconditionally, so the AdminUI save path also did not guard against this.
|
||||
|
||||
**Recommendation:** Add `if (dataType == ModbusDataType.String && stringLength < 1) return false;` in TryParse.
|
||||
|
||||
**Resolution:** Fixed in this review (2026-06-19). Added guard after the stringLength read, analogous to the existing Driver.Modbus-009 / ValidateStringLength protection in the pre-declared tag path. Build verified (0 errors, 0 warnings).
|
||||
|
||||
---
|
||||
|
||||
### Driver.Modbus.Contracts-003
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusDriverOptions.cs:40` |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** MaxRegistersPerRead carried the comment "Setting to 0 disables the cap (discouraged — the spec upper bound still applies)". This is inaccurate and self-contradictory: setting the value to 0 causes the ReadPlanner to substitute the Modbus-spec maximum of 125, not to remove the cap. An operator reading "disables the cap" could reasonably expect unlimited chunk sizes.
|
||||
|
||||
**Recommendation:** Replace "disables the cap" with precise language stating that 0 resets to spec maximum.
|
||||
|
||||
**Resolution:** Fixed in this review (2026-06-19). Replaced the comment with language explaining that 0 resets to the spec maximum (125) and noting that values above 125 are spec-illegal for conforming servers. Build verified.
|
||||
|
||||
---
|
||||
|
||||
### Driver.Modbus.Contracts-004
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusDriverOptions.cs:53-60` |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** MaxCoilsPerRead is typed ushort (0-65535) but the Modbus spec limits FC01/FC02 to 2000 coils per request. The comment only warned against 0; it did not warn that values above 2000 are spec-illegal. Setting MaxCoilsPerRead = 2001 caused the ReadPlanner to issue a single PDU requesting 2001 coils, which most PLCs reject with exception 03. No [Range] attribute was present to constrain Options binding, unlike ProbeTimeoutSeconds which already carries [Range(1, 60)]. The comment also used the inaccurate "disables the cap" phrasing (same root issue as 003).
|
||||
|
||||
**Recommendation:** Add [Range(0, 2000)] and clarify the 0-means-spec-maximum semantics in the doc.
|
||||
|
||||
**Resolution:** Fixed in this review (2026-06-19). Added [Range(0, 2000)] attribute to MaxCoilsPerRead and updated the comment to explain that 0 resets to spec maximum and that values above 2000 are spec-illegal. Build verified (0 errors, 0 warnings).
|
||||
@@ -37,7 +37,9 @@ public sealed class ModbusDriverOptions
|
||||
/// Omron CJ/CS cap at <c>125</c>. Set to the lowest cap across the devices this driver
|
||||
/// instance talks to; the driver auto-chunks larger reads into consecutive requests.
|
||||
/// Default <c>125</c> — the spec maximum, safe against any conforming server. Setting
|
||||
/// to <c>0</c> disables the cap (discouraged — the spec upper bound still applies).
|
||||
/// to <c>0</c> resets to the spec maximum (<c>125</c>) without requiring the operator
|
||||
/// to hard-code the limit. Values above <c>125</c> are spec-illegal for conforming
|
||||
/// servers; only use them when the target device's documentation explicitly allows it.
|
||||
/// </summary>
|
||||
public ushort MaxRegistersPerRead { get; init; } = 125;
|
||||
|
||||
@@ -55,8 +57,11 @@ public sealed class ModbusDriverOptions
|
||||
/// spec allows up to <c>2000</c> bits per request — separate from
|
||||
/// <see cref="MaxRegistersPerRead"/> because the underlying packing is different
|
||||
/// (1 bit per coil vs 16 bits per register). Default <c>2000</c>; setting to <c>0</c>
|
||||
/// disables the cap. The driver auto-chunks coil-array reads above the cap.
|
||||
/// resets to the spec maximum (<c>2000</c>). Values above <c>2000</c> are spec-illegal
|
||||
/// and will cause most PLCs to reject the request with exception 03. The driver
|
||||
/// auto-chunks coil-array reads above the cap.
|
||||
/// </summary>
|
||||
[Range(0, 2000)]
|
||||
public ushort MaxCoilsPerRead { get; init; } = 2000;
|
||||
|
||||
/// <summary>
|
||||
|
||||
@@ -34,6 +34,16 @@ public static class ModbusEquipmentTagParser
|
||||
var byteOrder = ReadEnum(root, "byteOrder", ModbusByteOrder.BigEndian);
|
||||
var bitIndex = (byte)ReadInt(root, "bitIndex");
|
||||
var stringLength = (ushort)ReadInt(root, "stringLength");
|
||||
// Guard: String tags require StringLength >= 1. RegisterCount = (StringLength+1)/2,
|
||||
// so StringLength=0 → 0 registers → spec-illegal FC03/FC04 with quantity=0 → PLC
|
||||
// returns exception 03. Reject here (analogous to Driver.Modbus-009 / ValidateStringLength
|
||||
// in the pre-declared tag path) so the driver surfaces BadNodeIdUnknown rather than a
|
||||
// misleading BadCommunicationError. (Driver.Modbus.Contracts-002)
|
||||
if (dataType == ModbusDataType.String && stringLength < 1) return false;
|
||||
// Guard: BitInRegister tags require BitIndex 0–15. Shift by ≥ 16 on an int overflows
|
||||
// a ushort mask silently — reads always return false, writes have no effect.
|
||||
// (Driver.Modbus.Contracts-001)
|
||||
if (dataType == ModbusDataType.BitInRegister && bitIndex > 15) return false;
|
||||
// isArray / arrayLength — optional keys authored by the typed Modbus tag editor.
|
||||
// Canonical rule: a tag is an array iff isArray:true AND arrayLength >= 1.
|
||||
// isArray:false (with any arrayLength) is scalar — the foundation materialises a
|
||||
|
||||
Reference in New Issue
Block a user