modbus: stop variable arrays over-reading into the next ADU

[ArockiaRajamanickam]

Fixes #4096. The diagnosis is Insanitree's, from the issue thread.

The bug

FieldListField.count_from expects an element count, but the Modbus PDUs pass byteCount, which is a byte count. For the arrays whose elements are 16-bit registers that is off by a factor of two, so the field consumes twice as many items as the PDU holds and reads into whatever follows. Modbus/TCP is a stream protocol, so several ADUs routinely arrive in one buffer.

On master (5fb7637) and on released 2.7.0, parsing two concatenated Read Input Registers responses:

registerVal = [0x1111, 0x2222, 0x3333, 0x1, 0x0, 0x9]     # expected 3 values

The trailing three shorts are the next ADU's transId, protoId and len. The remaining buffer then starts mid-header, so the second ADU is unrecoverable. Nothing raises, so this is silent data corruption in a dissector people point at ICS traffic.

The fix

Divide byteCount by the element size for the six register arrays.

The three arrays of byte-sized elements (coilStatus, inputStatus, and outputsValue on Write Multiple Coils) were already correct, since there byteCount and the element count coincide. They are untouched.

Read FIFO Queue is a special case. Its byteCount is built as p * 2 + 2 because the spec has it cover the FIFOCount field as well, so byteCount // 2 would be wrong by one element. It already carries FIFOCount, which is exactly the element count, so that is used instead.

ModbusPDU01ReadCoilsResponse had a second, different problem: it derived from _ModbusPDUNoPayload, whose extract_padding returns b"", so a following ADU was not mangled but discarded entirely. Its structural counterpart ModbusPDU02ReadDiscreteInputsResponse already derives from Packet, so this looks like an oversight rather than a decision. Changed to match.

Verification

I swept all nine byteCount-driven arrays, checking three things each: a lone ADU still parses, two concatenated ADUs give the right element count, and the second ADU survives intact.

before:  FAILURES: 7 / 9
after:   FAILURES: 0 / 9

The seven were the six register arrays plus Read Coils. Note the issue slightly under-counts the blast radius: it focuses on the register over-read, but Read Coils fails a different way, by dropping the trailing ADU rather than corrupting it.

Three regression tests added to test/contrib/modbus.uts. The suite goes 164 → 167 passing, and reverting just the source change while keeping the tests fails exactly those three, so they bind to the defect rather than to the implementation:

###(164)=[failed] Register arrays are not over-read when a second ADU follows
###(165)=[failed] Read Coils response keeps the ADU that follows it
###(166)=[failed] Every byteCount-driven array round-trips across concatenated ADUs

flake8 scapy/contrib/modbus.py is clean. mypy --ignore-missing-imports reports an identical count before and after, so nothing new was introduced.

One thing I did not change

Every class deriving from _ModbusPDUNoPayload drops trailing bytes, so fixed-size request PDUs also lose a following ADU in the same buffer — I confirmed that with ModbusPDU01ReadCoilsRequest. Fixing that would mean changing the base class behaviour for 24 classes, which is a design call rather than a bug fix, so I left it alone and scoped this PR to the arrays plus the one response that was inconsistent with its sibling. Happy to follow up if you want the broader change.

AI-Assisted: yes (Claude Fable 5, via Claude Code) is on the commit, per CONTRIBUTING. The reproduction, the nine-way sweep and the negative control are all runs I made against this branch.