Dangerous behavior when faced with invalid ADUs

[chintal]

Versions

• Pymodbus: 1.4.0+ atleast, probably historic versions as well

Pymodbus Specific

• Client: observed in sync, though probably applies to async as well

Servers should reject malformed commands

The bug that led to #291 was a pymodbus issue that has since been resolved. However, the way it manifested itself exposed a serious flaw in the way pymodbus handles malformed packets.

_process() of ModbusRtuFramer in transaction.py, as does that of the ModbusSocketFramer, attempts to process a packet irrespective of whether the packet checks out. It simply assumes that the PDU starts at byte 0, without a header. In the case of ModbusRtuFramer, this treats the first byte of the ADU, which is the slave / server address, as the first byte of the PDU, ie, the function code. In this case, it caused all manner of headaches when attempting to diagnose a seeming connection error from a fully compliant Modbus server, which seemed to be sending a Bit Read response to a CommEventLog request. I manually checked the memory on the modbus transmit buffer from within the device using GDB before I decided to see why pymodbus seemed to be crapping out.

More critically, if a message fails CRC or has the wrong length, it is processed nevertheless. In this case, the error was such that the incorrect response handler that the RTU decoder chose to use was unable to process the received response. However, if the message just happens to look about right for the command it seems to be a response for, it would cause unpredictable , uncommanded, and hard to debug behavior. Given the nature of typical Modbus slaves, this might have implications in the physical world. Consider a register read response with bit errors in the response for some reason.

As per the modbus specification, anything that fails CRC (or more generally, has an ADU that does not validate) should be discarded immediately and no response sent.

[dhoomakethu]

@chintal this has been fixed as part of 1.5.0 release, could you please confirm if you still have this issue with 1.5.0 as well ?

[chintal]

@dhoomakethu Indeed it seems to have been. I am unable to actually test this at the moment, but it looks like the code does now discard invalid packets. Closing the issue.