emcute: fix payload copy error for emcute_pub#11957
Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom Oct 7, 2019
Merged
Conversation
`len` is used with the `memcpy()` to copy the payload to `tbuf`. With a payload provided that is just long enough to fill `tbuf`, `len += 6` leads to the `memcpy()` overriding data after `tbuf` (e.g. the `mutex` that is unlocked right after) and thus resulting in potential segmentation faults. Additionally `+ 6` can only be applied if the total packet length is below 256 (see spec), so `len + pos` is what needs to be provided to the corresponding send functions instead (`pos` adapts to the header length of the PUBLISH message).
5917475 to
10a3f3e
Compare
Member
Author
|
Fixed some typos in commit message 😅 |
benpicco
reviewed
Oct 1, 2019
Contributor
|
I guess the original code was based on the same false assumption as the one fixed in #11958 |
Member
Author
Yeah.. before I lost interest in this due to missing reviews, I planned to replace those too 😅 |
Member
Author
|
Ping @benpicco? |
benpicco
approved these changes
Oct 7, 2019
Contributor
benpicco
left a comment
There was a problem hiding this comment.
Code makes more sense now and pos ends up being 6 in the end anyway.
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.
Contribution description
lenis used with thememcpy()to copy the payload totbuf. With a payload provided that is just long enough to filltbuf,len += 6leads to thememcpy()overriding data aftertbuf(e.g. themutexthat is unlocked right after) and thus resulting in potential segmentation faults.Additionally
+ 6can only be applied if the total packet length is below 256 (see spec), solen + posis what needs to be provided to the corresponding send functions instead (posadapts to the header length of the PUBLISH message).Testing procedure
Merge #11823 and run its testing procedure.
Issues/PRs references
Split out of #11823 to stream-line the review.