debevv / nanoMODBUS

A compact MODBUS RTU/TCP C library for embedded/microcontrollers
MIT License
234 stars 47 forks source link

Missing validation of unit ID #12

Closed jonathangjertsen closed 2 years ago

jonathangjertsen commented 2 years ago

It seems that if a response is received with a unit ID that doesn't match the request that was sent, it will be accepted as a valid response. Perhaps there should be an NMBS_ERROR_INCORRECT_UNIT_ID?

debevv commented 2 years ago

Yes, I think this is was I was trying to do here at line 352: https://github.com/debevv/nanoMODBUS/blob/30c7ce3854837cda574841a5f3b259eecf8f6f00/nanomodbus.c#L339-L354 but the ignored field is not supposed to be set by recv_msg_header(). I will fix it by checking the unit ID directly here.

BTW this put me in a bit of a rabbit hole, because it would be nicer the client just ignored the message with no error and waited for the one directed to its unit ID. Turns out that the Modbus RTU spec expects exactly this behavior from a master, at page 9 of https://www.modbus.org/docs/Modbus_over_serial_line_V1_02.pdf. So this library is technically out of spec.

The problem with this approach is that, since RTU messages do not contain a length field, the only way I could think to reliably "skip" a message is to parse it. This means that the library should be able to parse any response FC, which is a no no for me. I also checked the implementation of libmodbus, and look like they are just returning an error in this case.

So, in conclusion, when receiving a RTU response not for us, the library will return the new NMBS_ERROR_INVALID_UNIT_ID

debevv commented 2 years ago

037de71d9b7627491f55c7dbd8013f0444f5069c

jonathangjertsen commented 2 years ago

It's strange that the spec requires the message to be silently ignored if it's the wrong unit ID - that should never happen if all devices on the network are conforming. Implementing it shouldn't require parsing the frames though - in RTU you could wait until the t3.5 interval expires, in ASCII mode I think you can scan for CRLF