Check level byte length to avoid panic #8889
Conversation
etseidl
left a comment
etseidl
left a comment
There was a problem hiding this comment.
Thanks @rambleraptor, this looks good. Just a few micro-optimizations to consider 😅
If this is erroring for you, it makes me want to check farther upstream and see if we can detect this when we decode the page header.
| @@ -437,6 +437,9 @@ where | |||
| let mut offset = 0; | |||
|
|
|||
| if max_rep_level > 0 { | |||
| if offset > buf.len() { | |||
There was a problem hiding this comment.
This one should never evaluate true?
| } | ||
|
|
||
| if offset > buf.len() { |
There was a problem hiding this comment.
If the checks above are moved to after offset is incremented, then I think this check can be eliminated as it will either always be true (offset is still 0), or the error condition was already caught.
| } | ||
|
|
||
| let def_levels_end = rep_levels_byte_len + def_levels_byte_len; |
There was a problem hiding this comment.
Why not move this check higher up and check all levels in one shot?
|
Hi @rambleraptor , will you have time to address @etseidl 's comments for this PR or shall we plan to do it as a follow on PR? |
3 participants
fix: Check level byte length to avoid panic
We've found a panic in our testing in these certain areas. I've added tests to help show when the errors would be thrown.