Add buffer safety checks to prevent out-of-bounds access#96
Open
hmueller01 wants to merge 12 commits into
Open
Add buffer safety checks to prevent out-of-bounds access#96hmueller01 wants to merge 12 commits into
hmueller01 wants to merge 12 commits into
Conversation
- connect(): null-buffer guard to prevent crash if buffer allocation failed - disconnect(): null-buffer guard - loop(): buffer existence and size validation before packet processing - handlePacket(): three ordered boundary checks for topic length, payload offset, and msgId fields - Detailed error messages with context for debugging
Owner
Author
|
@TD-er @MarcAntoineCRUE This PR is just about the safety checks (and some needed mods to satisfy new linter). |
Collaborator
|
At first glance I don't see any obvious issues, but since I was just about to go to bed, I will have to look at it tomorrow with some fresh eyes :) |
|
Memory usage change @ d3973b9
Click for full report table
Click for full report CSV |
TD-er
reviewed
Jun 8, 2026
Collaborator
|
Only found one thing to think about. |
|
Memory usage change @ 32b657c
Click for full report table
Click for full report CSV |
TD-er
reviewed
Jun 8, 2026
TD-er
reviewed
Jun 8, 2026
|
Memory usage change @ b67cac8
Click for full report table
Click for full report CSV |
…etBufferSize(), no need to check every time in loop()
Owner
Author
|
These branches that the AI (Claude) created are really strange. Now branch
|
Owner
Author
|
@TD-er If you find time again, can you please take a last look on it. Thanks. |
TD-er
reviewed
Jun 14, 2026
TD-er
reviewed
Jun 14, 2026
TD-er
reviewed
Jun 14, 2026
TD-er
reviewed
Jun 14, 2026
| // Cannot set it back to 0 | ||
| // Buffer must be large enough to hold at least a minimal MQTT packet. | ||
| // Note: MQTT_MAX_HEADER_SIZE (5) + protocol (9) + flags (1) + keepalive (2) covers a minmal CONNECT message. | ||
| if (size < MQTT_MAX_HEADER_SIZE + 9 + 1 + 2) { |
Collaborator
There was a problem hiding this comment.
Hmm I guess there might be good reasons to completely disable the buffer to save up all available resources, only to later set the buffer again.
So maybe it is good to accept sizes which are too small and then simply delete the buffer and set its pointer to null.
This value you're now checking against may be a good bottom value to just clear the buffer.
Not sure though what the return value should be for this function.
Owner
Author
There was a problem hiding this comment.
Ok, I now allow setting the buffer to 0, but not to lower values than the min buffer size. It this fine for you?
Owner
Author
There was a problem hiding this comment.
@TD-er Are you ok with the new implementation?
Collaborator
|
There are quite a few functions in the code that try to access For example:
|
Owner
Author
Hmm, this is the question. Do we make every function failsafe by it's own, or do we trust that it was checked before. I see only one case that might fail here: pubsubclient3/src/PubSubClient.cpp Lines 737 to 741 in c856d48 |
|
Memory usage change @ c856d48
Click for full report table
Click for full report CSV |
Owner
Author
|
@MarcAntoineCRUE Heard nothing from you. Hope you are doing well. If no further comments I'll merge this soon. |
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
2 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.
Applied safety checks introduced by @MarcAntoineCRUE in #91