stack overflow in TIdModBusClient.SendCommand

[petergmorgan]

Hi,
We ran into a issue (using Indy10, Delphi 2007) where our application was crashing to desktop and discovered there can be an overflow in
IdModbusClient
Move(RecBuffer[0], ReceiveBuffer, iSize)
if the server responds with too much data on the input buffer since MBPData is a fixed size but the client doesn't check if the amount of data returned will fit into the recievebuffer.

For my purposes I've added a simple size check and thrown away the rest of the buffer in that cycle and that's stopped the overflow (and random CTD) here.

[JensMertelmeyer]

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.

[petergmorgan]

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.

[JensMertelmeyer]

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.)

[petergmorgan]

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)

[JensMertelmeyer]

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.