"Certain invalid inputs to ReadUvarint or ReadVarint could cause those functions to read an unlimited number of bytes from the ByteReader argument before returning an error. This could lead to processing more input than expected when the caller is reading directly from a network and depends on ReadUvarint and ReadVarint only consuming a small, bounded number of bytes, even from invalid inputs.
"With the update, ReadUvarint and ReadVarint now always return after consuming a bounded number of bytes (specifically, MaxVarintLen64, which is 10). The result being returned has not changed; the functions merely detect and return some errors without reading as much input."
Proto has a different structure with a fully unrolled loop. It could never read more than 10 bytes. encoding/binary just looped as long as the more bit was set, which is a hazard of more-bit encodings (which suck; unary-length-prefixed codes like UTF-8 are strictly better in every way).
FWIW, more bit encodings allow you to quickly "resynchronize" a stream if you are dropped in the middle... UTF-8 is actually a hybrid and uses both strategies.
I think it's unlikely this would have been caught by fuzzing. I think you'd have to write assertions that it only read as many bytes as necessary, which is basically saying just don't write the bug in the first place.
Not sure but couldn't a fuzzer have created an input that would have timed out the fuzzer? I guess it also depends on the timeout you set when running it.
You would also have needed to have a slow unboundedly large data stream on the other end. I fuzzed a bunch of varint-using code last month and never found this, because I was always working with in-memory buffers. The loop is simple enough that anything reasonably sized (e.g. under a GB) would probably not hit any reasonable timeout.
Hypothetically yes, but I'm not sure any common fuzzing strategy would catch that. Pure random mutations would never get this far, and code-coverage based mutation won't either. How to create a strategy that would catch this but not be rife with false positives and wasted fuzzing time is not immediately obvious.
Go doesn't have assertions. Of course it is not hard to mimic them, but it's an intentional choice to increase the friction of doing that and make it feel unidiomatic.
I should not have used the word "assertion". I meant it in the more general way of "code that checks some property", not a literal "assert" feature as built in to the language.
I was just pointing out that this bug basically requires an explicit test checking for exactly the bug. Fuzzing would not have found the check for you.
An assertion, I think, would crash the program. In that sense implementing them is simple - just panic when the condition is violated. Go prefers plain checks that return errors, though. Panics should be used for truly unexpected conditions, such as a nil pointer. Invalid input, as in this case, would be better served as an error.
> Panics should be used for truly unexpected conditions, such as a nil pointer.
Exceptional cases, if you will.
I find it funny when people say Go doesn't have exceptions when Go is the only mainstream language that advocates for and prefers the use of exceptions.
"Certain invalid inputs to ReadUvarint or ReadVarint could cause those functions to read an unlimited number of bytes from the ByteReader argument before returning an error. This could lead to processing more input than expected when the caller is reading directly from a network and depends on ReadUvarint and ReadVarint only consuming a small, bounded number of bytes, even from invalid inputs.
"With the update, ReadUvarint and ReadVarint now always return after consuming a bounded number of bytes (specifically, MaxVarintLen64, which is 10). The result being returned has not changed; the functions merely detect and return some errors without reading as much input."
What's a varint? This: https://developers.google.com/protocol-buffers/docs/encoding
You have to use varints to be affected, but of course you could be using them in a library without knowing it.