-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix buffer overrun found with address sanitizers #39
Conversation
Here's another (IMO cleaner) way to fix the problem: diff --git a/cobs.c b/cobs.c
index c080867..1e0e324 100644
--- a/cobs.c
+++ b/cobs.c
@@ -36,10 +36,9 @@ cobs_ret_t cobs_decode_inplace(void *buf, unsigned const len) {
cobs_byte_t *const src = (cobs_byte_t *)buf;
unsigned ofs, cur = 0;
- while ((ofs = src[cur]) != COBS_FRAME_DELIMITER) {
+ while (cur < len && ((ofs = src[cur]) != COBS_FRAME_DELIMITER)) {
src[cur] = 0;
cur += ofs;
- if (cur >= len) { return COBS_RET_ERR_BAD_PAYLOAD; }
}
if (cur != len - 1) { return COBS_RET_ERR_BAD_PAYLOAD; } any preference? |
cobs.c
Outdated
@@ -39,7 +39,7 @@ cobs_ret_t cobs_decode_inplace(void *buf, unsigned const len) { | |||
while ((ofs = src[cur]) != COBS_FRAME_DELIMITER) { | |||
src[cur] = 0; | |||
cur += ofs; | |||
if (cur > len) { return COBS_RET_ERR_BAD_PAYLOAD; } | |||
if (cur >= len) { return COBS_RET_ERR_BAD_PAYLOAD; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How embarrassing :-D
Thanks a ton for the PR; this is what happens when I don't UBSAN / ASAN from day 1, sigh :) I agree, I like your "cleaner" version more as well, though I'd happily merge either, lemme know. Do you happen to have a test case? It'd be nice to harness this w/a unit test as well as the sanitizer. Also if you want to add a |
We don't build doctest.h with the sanitizers since doctest itself triggers the address sanitizer.
If the payload passed to cobs_decode_inplace is not 0-terminated then cobs_decode_inplace may access out-of-bounds memory in its main loop due to checking the wrong boundary condition. This commit fixes the problem. Discovered using the recently added address sanitizers.
Sounds good, just amended the commit with the "cleaner" version.
Ah, I forgot to mention that this already has a test case, which failed when I turned sanitizers on (but before I fixed the bug in cobs_decode_inplace): https://github.com/charlesnicholson/nanocobs/blob/main/tests/test_cobs_decode_inplace.cc#L31-L32
Done! |
Thanks for this! I'll fix the issue you filed and then cut a new release, unless turning on the sanitizers reveals any other horrors :) |
See commit messages for more info. And thank you for this project!