[HIGH] Ensure command buffers stay NUL-terminated to prevent overflow - #3395
Merged
Merged
Conversation
The serial command buffers must stay NUL-terminated within their bounds: if they ever aren't, strlen() can return >= sizeof(command) and the read loop would index past the buffer. Additionally, a full buffer now becomes a completed line (end-of-line marker placed inside the buffer, NUL terminator kept) instead of overwriting the terminator and silently corrupting the buffer for the next pass. Applies to the serial CLI readers of the repeater, room server, sensor and secure chat examples, and to the CLI rescue reader of the companion example.
|
Can confirm this issue and your bugfix. We worked and debugged on same problem in different worlds at the same time :-) I'll need a lot of Portuguese sun to heal the trauma I suffered from analyzing it... it took many days. This bug crashes my HT-CT62 repeater every 24-36h. RAK3172 is less affected but the implementation shares the same code. My Analysis: #3397 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The CLI loops in the examples size the command buffer with
strlen()and keep appending to it. When the buffer fills, the old code wrote'\r'over the final byte, the one holding the NUL. From then onstrlen()can walk past the buffer and the append loop indexes past the end.Clamps
lenat the top of each pass and, when the buffer is full, places the end-of-line marker atlen-2so the final byte stays NUL. Applied to companion_radio, simple_repeater, simple_room_server, simple_secure_chat and simple_sensor.The corruption usually showed up as random reboots on TX, most visible on 433MHz boards. Serial lines left floating pick up RF noise, and that noise fills the command buffer. Most boards are affected; the Heltec v3 just shows it more often.