search

cwalter-at / freemodbus

BSD licensed MODBUS RTU/ASCII and TCP slave
720 stars 379 forks source link

In mbfuncholding.c, line 185 is there a bug? #14

Closed rhino949 closed 4 years ago

rhino949 commented 4 years ago

freemodbus/modbus/functions/mbfuncholding.c; line 185, usRegCount = ( USHORT )( pucFrame[MB_PDU_FUNC_READ_REGCNT_OFF + 1] ); like should be: usRegCount |= ( USHORT )( pucFrame[MB_PDU_FUNC_READ_REGCNT_OFF + 1] );

JoelStienlet commented 4 years ago

Indeed, I agree this looks like a bug: the 8 upper bits get trashed. The function will sometimes succeed where it should have returned "eStatus = MB_EX_ILLEGAL_DATA_VALUE;"

JoelStienlet commented 4 years ago

@cwalter-at can you please make this very small change, just add the "|" at L185?

ntfreak commented 4 years ago

see https://github.com/cwalter-at/freemodbus/pull/4/files

JoelStienlet commented 4 years ago

So, given the good explanations of karlp on this link, perhaps that first line (L184) could indeed be dropped? The compiler probably drops it anyway (when optimizations are enabled), but that would make the code easier to understand IMO. Nevertheless, the behaviour of this library would change for buggy implementations of the protocol, those that put some non-zero value in the first byte ( as karlp explains, 0 is the only correct value). So what should the default behaviour of this library be? Is it safer to detect this error and refuse to process the frame, or should the frame be processed anyway because allowing this kind of small bugs increases compatibility?

rhino949 commented 4 years ago

I got it.