stack overflow in TIdModBusClient.SendCommand about delphi-modbus HOT 5 OPEN

Yes, using Move(..) without any checks is extremely dangerous. A crash to desktop is plausible, since Move(..) does not do any checks at all. I believe replacing this line with
ReceiveBuffer := TBitConverter.InTo<TModBusResponseBuffer>( TBytes(RecBuffer) );
with no other changes at all does solve the memory corruption issue.

The current behaviour is going to be the same when the reply is too long but will now raise a range check error (ERangeError) when the received reply is not sufficient to fill a TModbusResponseBuffer.

Please let me know if I should submit a pull request.

from delphi-modbus.

TBitConverter isn't available in D2007 but I agree that it is better an error is returned.
maybe something like:

  //compare the size of the input buffer against the max packet size
  if iSize<=Sizeof(ReceiveBuffer) then
    Move(RecBuffer[0], ReceiveBuffer, iSize)
  else
      raise Exception.Create(sBufferLengthExceptionMessage);

Then at least an exception is raised if the buffer is an invalid size.

from delphi-modbus.

Yes, but this alone is not going to solve your problem when there is too much data (when iSize > SizeOf(ReceiveBuffer)). Then Move(..) will still corrupt your memory as it will write beyond the end of ReceiveBuffer. Another check is needed as well.

(Having to support environments older than a decade is such a bane for Delphi code.)

from delphi-modbus.

Would the if iSize<=sizeof(ReceiveBuffer) then... check not be enough to prevent the move operation all all instances where the server has returned too much data? (I may have missed further move commands in the module - this was the only one that caused us a problem so far)

from delphi-modbus.

No: iSize is the number of raw bytes you received from the network buffer (RecBuffer). We are trying to copy those bytes into a record of type TModbusResponseBuffer (which has a size of 269 bytes). If we have more than 269 bytes in our RecBuffer (this means iSize > SizeOf(ReceiveBuffer)) then the Move(..) command will first completely fill the TModbusResponseBuffer-record. After that, it will happy write the remaining bytes into the memory after that. This leads to memory corruption.

from delphi-modbus.