Skip to content

Robustness Improvements#91

Open
MarcAntoineCRUE wants to merge 6 commits intohmueller01:masterfrom
MarcAntoineCRUE:dev/performances
Open

Robustness Improvements#91
MarcAntoineCRUE wants to merge 6 commits intohmueller01:masterfrom
MarcAntoineCRUE:dev/performances

Conversation

@MarcAntoineCRUE
Copy link

Summary

This merge request significantly improves the robustness and safety of the PubSubClient library, without changing its public API or breaking existing usage. All changes are fully covered by the test suite (57/57 tests passing).

Key Fixes

  • Payload Length Underflow Protection: In handlePacket(), the calculation of payloadLen is now guarded to prevent unsigned underflow if a malformed packet claims a topic length larger than the actual buffer. This prevents potential buffer overreads and undefined behavior.
  • Payload Offset Overflow Protection: The calculation of payloadOffset now uses size_t instead of uint16_t to avoid silent overflow when handling large topics.
  • Consistent State in setBufferSize():
    The buffer size and pointer are only updated after a successful allocation. If allocation fails, the previous buffer and size remain unchanged, preventing inconsistent internal state.
  • Null Buffer Guards in connect() and disconnect(): Both methods now check for a valid buffer pointer before use, preventing possible null pointer dereference if buffer allocation failed at construction.
  • Type Safety in subscribeImpl() and unsubscribeImpl(): Internal length variables now use size_t instead of uint16_t to avoid narrowing/truncation when handling large topics.

Rationale

These changes address potential edge cases that could lead to:

  • Buffer overflows or underflows
  • Crashes due to null pointer dereference
  • Silent data corruption with large topics or payloads
  • Inconsistent internal state after memory allocation failures

All fixes are implemented with minimal code changes and maintain full backward compatibility.

return false;
}
size_t payloadLen = length - payloadOffset; // safe: payloadOffset <= length guaranteed above
uint8_t* payload = _buffer + payloadOffset;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm a bit hard to see by just browsing the code in the browser on GH, but shouldn't here also be some check against _bufferSize

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I forgot some checks

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not at that location handlePacket() can't be called without readPacket(). But I would suggest checking in loop(), with that nothing will ever be read.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I proposed a solution, could you check if it is ok for you?

Copy link
Owner

@hmueller01 hmueller01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A first glimps over the changes ... lots of good stuff, but still a few changes.

uint16_t payloadOffset = hdrLen + 3 + topicLen; // payload starts after header and topic (if there is no packet identifier)
size_t payloadLen = length - payloadOffset; // this might change by 2 if we have a QoS 1 or 2 message
uint8_t* payload = _buffer + payloadOffset;
size_t payloadOffset = (size_t)hdrLen + 3u + topicLen; // use size_t to avoid uint16_t overflow on large topics
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. But please leave the comment.

Suggested change
size_t payloadOffset = (size_t)hdrLen + 3u + topicLen; // use size_t to avoid uint16_t overflow on large topics
size_t payloadOffset = (size_t)hdrLen + 3u + topicLen; // payload starts after header and topic (if there is no packet identifier)

size_t payloadOffset = (size_t)hdrLen + 3u + topicLen; // use size_t to avoid uint16_t overflow on large topics

if (length < payloadOffset) { // do not move outside the max bufferSize
// Guard BEFORE computing payloadLen to prevent size_t underflow (payloadOffset > length)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can do it so. But there will be no exception here. payloadLen might be calculated wrong, but never used.

…ing buffer safety checks and memory management improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants