While developing a Teltonika-based modbus solution, we noted your modbus TCP to RTU daemon has a bug: If more than one Modbus TCP message arrives in a way where your recv() Calls has access to at least one additional byte beyond the length of the first message, the off-by-one prevents ANY Modbus TCP messages of this connection to be interpreted. Instead, the daemon will read everything from that TCP connection and then close it immediately without handling any message.
This is a very annoying problem if one has to send multiple messages at the same time, as it basically forces us to implement unnecessary pauses between messages.
Unfortunately, the daemon is closed source (why? It’s not exactly rocket science, sorry guys), so I’ll show the bug in a Ghidra screenshot:
Please disregard the function names. These are of course fictitious and where set by myself while I reconstructed the nature of the bug. You should be able to find the correct function in your code by tracking the error messages which I left as comment in the screenshot. The off-by-one bug is in line 17 of my screenshot in combination with the (correct) check in line 30.
You’re reading one byte too many. The +7 in line 17 should be a +6. Therefore the check in line 30 which checks if the message arrived completely never triggers. Subsequent callbacks from uloop will simply lead to more and more data being read. And then, when no more data is available, the if in line 25 will trigger, ending the connection without any message ever handled.
The code also possibly has security issues and allows overwriting the heap as there’s no maximum length checks, and also, the off-by-one error allows endlessly filling the memory anyway as long as TCP data stays available.
While further working with modbusgwd, we noticed a second though less annoying bug: If one sends more than one message, as soon as one of the messages times out (due to a device not answering on the RTU bus), all further messages sent at similar times will time out:
The reason for that: The RTU bus is basically treated as a blocking resource - while an answer is awaited, no further message will be put on the serial. That’s ok.
But at the same time, uloop_timeout_set is already called when the TCP messages were incoming - in the log at the time of the “Adding transaction timeout 5000ms”.
So the first message timing out will cause any nearby other message also to time out, as their timeouts will already have started.
The fix here would be to call uloop_timeout_set later, when the message is actually put on the RTU bus.
