-
Notifications
You must be signed in to change notification settings - Fork 97
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 decoding of truncated messages containing variable length arrays [AP-948] #1381
Conversation
@@ -218,6 +218,9 @@ bool (((m.internal_decode_fn)))(sbp_decode_ctx_t *ctx, (((m.type_name))) *msg) | |||
if (!(((f.decode_fn)))(ctx, &(((field)))[i])) { return false; } | |||
} | |||
((*- elif f.packing == "variable-array" *)) | |||
if ( ((ctx->buf_len - ctx->offset) % (((f.encoded_len_macro)))) != 0) { |
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.
This is the only change in need of review, everything else is generated
Note code coverage is going to fail for the moment, I'm working on increasing it on separate PRs |
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.
Nice catch
c6dafad
to
28afe31
Compare
[libsbp-c] SonarCloud Quality Gate failed. 0 Bugs 0.0% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
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.
LGTM!
Description
@swift-nav/devinfra
If an SBP message has a variable length array of some type, when decoding a input wire encoded buffer which has been truncated the decoder will not produce an error when it should.
Imagine a message containing some amount of overhead, then a variable length array of some type.
In this message the array is not fixed length, rather the number of elements is variable and is calculated from the size of the input buffer:
Decoding of each element is then performed in a loop:
If the size of the header is 3 bytes and the size of each element is 4 bytes an example representation on the wire could be:
offset
And so long as this is what’s fed in to the decoder then
num_elems
will be correctly calculated as 2 and all input data will be consumed.But if the input buffer is somehow truncated:
then due to integer rounding
num_elems
will be calculated as 1. Only the first element of the array will be decoded (and it will succeed), but the truncated data at the end of the input buffer will not be considered at all.The correct behaviour in this situation should be for the decoder to notice that the input buffer does not contain an integer number of elements in the variable length array and return an error
API compatibility
Does this change introduce a API compatibility risk?
No
API compatibility plan
If the above is "Yes", please detail the compatibility (or migration) plan:
N/A
JIRA Reference
https://swift-nav.atlassian.net/browse/AP-948