DecodeVarint: Fix panic on truncated input; Add tests #12
Conversation
Replace references to cb.len with cb.Len() to ensure we are checking the difference from cb.len - cb.index. When some values have already been parsed, DecodeVarint will be called with cb.index > 0, which means the existing bounds checks were wrong Commit 775411c added faster decoding of varints, but omitted some of the bounds checks. This means that using this library to parse truncated inputs now panics, instead of returning an error. Add a fuzz test to ensure this continues to work. Add a deterministic unit test extracted from the fuzz test as an easy to understand example.
| @@ -45,15 +45,15 @@ func init() { | |||
| // | |||
| // This implementation is inlined from https://github.com/dennwc/varint to avoid the call-site overhead | |||
| func (cb *Buffer) DecodeVarint() (uint64, error) { | |||
| if cb.len == 0 { | |||
| if cb.Len() == 0 { | |||
There was a problem hiding this comment.
I'm ... actually not 100% sure these check replacements are correct. They do pass the unit tests, and the fuzz test does fail if you remove any of them, which seems ... good?
|
@evanj I'm approving this so C.I runs, but lets ask @kevinconaway to review it |
|
Will you please help me understand how the length is changing? |
|
The length isn't changing. It is just that the existing code only checks For correctly encoded data, it doesn't matter: the current encoder will never "run off the end". For example, even in the "buffer has > 10 space" branch, if the Varint is terminated, it will return early: https://github.com/richardartoul/molecule/blob/master/src/codec/decode.go#L59-L62 The problem is when the varint is incorrectly encoded, this causes it to run off the end of the buffer. As a simple example, consider the protocol buffer message: This encodes as: If you modify the payload to be This happens because the code assumed the buffer had 2 bytes in it, from the following code, but it actually only had 1: https://github.com/richardartoul/molecule/blob/master/src/codec/decode.go#L162-L164 |
|
I understand, thanks. Would you please post a benchstat diff? |
|
I ran |
Replace references to cb.len with cb.Len() to ensure we are checking
the difference from cb.len - cb.index. When some values have already
been parsed, DecodeVarint will be called with cb.index > 0, which
means the existing bounds checks were wrong
Commit 775411c added faster decoding of varints, but omitted some
of the bounds checks. This means that using this library to parse
truncated inputs now panics, instead of returning an error.
Add a fuzz test to ensure this continues to work. Add a deterministic
unit test extracted from the fuzz test as an easy to understand
example.