You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I don't currently have the hardware to test this so I'm hesitant to make a PR, but I think there's a bug in the firmware's cobs_encode and cobs_decode implementations. Both issues arise from using the parameter len, which is the length of the input buffer, to make sure that they aren't at the end of the output buffer.
In cobs_encode this is "fixed" by adding an offset to the length (here.) However, COBS encoding does not have a one to one relationship between input buffer size and output buffer size; len + 2 is only the correct size if the input buffer is under 255 bytes (which it always is, to be fair). I think the best course of action is to remove the check entirely and always write the trailing zero, I don't actually see how a situation where we don't want to write it arises -- but I feel like I might be missing something there.
In cobs_decode the intention seems to be to avoid writing the trailing zero to the output buffer, but again the relationship between dst_i and len isn't actually consistent; it varies with the length of the data, so this will sometimes write past the end of the array. This one doesn't even have the + 2 to try and correct it, so I'm pretty sure it's illegally writing zeros pretty consistently. I think the correct check would be i < len instead of dst_i < len, the same as in the for loop above.
These are both probably fine considering the actual data being sent, but could cause some subtle issues (unless I'm totally off base with both which is likely).
The text was updated successfully, but these errors were encountered:
Thanks for this thoughtful issue. I just caught this but may not have time to fix it before passing over maintenance of the project to someone else at Hack Club this Friday. I'll keep an eye out if it causes some issues.
I don't currently have the hardware to test this so I'm hesitant to make a PR, but I think there's a bug in the firmware's
cobs_encode
andcobs_decode
implementations. Both issues arise from using the parameterlen
, which is the length of the input buffer, to make sure that they aren't at the end of the output buffer.In
cobs_encode
this is "fixed" by adding an offset to the length (here.) However, COBS encoding does not have a one to one relationship between input buffer size and output buffer size;len + 2
is only the correct size if the input buffer is under 255 bytes (which it always is, to be fair). I think the best course of action is to remove the check entirely and always write the trailing zero, I don't actually see how a situation where we don't want to write it arises -- but I feel like I might be missing something there.In
cobs_decode
the intention seems to be to avoid writing the trailing zero to the output buffer, but again the relationship betweendst_i
andlen
isn't actually consistent; it varies with the length of the data, so this will sometimes write past the end of the array. This one doesn't even have the+ 2
to try and correct it, so I'm pretty sure it's illegally writing zeros pretty consistently. I think the correct check would bei < len
instead ofdst_i < len
, the same as in thefor
loop above.These are both probably fine considering the actual data being sent, but could cause some subtle issues (unless I'm totally off base with both which is likely).
The text was updated successfully, but these errors were encountered: