Expand
## Problem
`read_varint32()` and `read_varint64()` silently ignore any extra bits, which I felt could lead to errors seemingly out of nowhere.
Brief explanation
> Varints need to encode data within a whole number of bytes, and each byte encodes 7 bits of data (last bit is a flag), so there are often bits left over. To encode a 64-bit number, we need a minimum of 10 bytes, but this gives us 10*7=70 bits of content, hence we have 70-64 = 6 bits extra. If we were to set those extra bits, it might indicate that we wanted a larger number than could be encoded in 64 bits, and we are mistakenly reading it using this function. As of now, the function simply ignores extra bits, constituting a silent truncation.
## Should we add checks against this?
Originally, I thought it would be desirable for the function to throw an error when extra bits were detected, instead of silently truncating to 32/64 bits. However, there are some cons:
- Disallows users from using extra bits for their own purposes (messy, but this crate is intended for speed optimization first and foremost after all)
- Does not follow Google's main implementation of protobuf
Thus, after discussing with some seniors within my org, we decided not to enforce this, though I have left in the implementation [commit](https://github.com/tafia/quick-protobuf/pull/223/commits/e3a40ed9a7ddd4c5d13ca5aec88b2e4080f54cd4) for any future reference.
## Final decision
- Did NOT add the checks in the end!
- Instead, added comments to `read_varint32()` and `read_varint64()` regarding what the user must guarantee about input in order to avoid byte truncation.
Minor adjustments to #209
Expand
## Immediate issue
`./run_tests.sh` does not work:
```
error[E0433]: failed to resolve: use of undeclared crate or module `std`
--> quick-protobuf/src/reader.rs:458:13
|
458 | use std::convert::TryFrom;
| ^^^ use of undeclared crate or module `std`
error[E0599]: no function or associated item named `try_from` found for type `usize` in the current scope
--> quick-protobuf/src/reader.rs:480:29
|
480 | let offset = usize::try_from(offset).map_err(|_| Error::SizeOverflow)?;
| ^^^^^^^^ function or associated item not found in `usize`
|
= help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
|
10 | use core::convert::TryFrom;
|
```
## Other issues
- Only `WIRE_TYPE_LENGTH_DELIMITED` needs to be checked by `usize::try_from()`, since `WIRE_TYPE_FIXED32` and `WIRE_TYPE_FIXED64` both give hardcoded values (4 and 8 respectively) within `usize` range. However, all 3 are being checked by `usize::try_from()`, which is a minor inefficiency.
- An erroneous `offset` addition may still exceed the range of `self.end - self.start` without actually overflowing `usize` itself; this is arguably a more likely error that we want to check for.
- As far as I could tell, `Error::SizeOverflow` is only used with varints, *and* `Error::Varint` is only used (thus far) in overflow situations; so one of them is redundant.
## Adjustments made:
- Used `core` instead of `std` for `TryFrom`
- Used a stricter check in `read_unknown()` and refactored control flow for efficiency
- Removed `Error::SizeOverflow` in favor of `Error::Varint`
Varint truncation comments
Expand
## Problem `read_varint32()` and `read_varint64()` silently ignore any extra bits, which I felt could lead to errors seemingly out of nowhere.Brief explanation
> Varints need to encode data within a whole number of bytes, and each byte encodes 7 bits of data (last bit is a flag), so there are often bits left over. To encode a 64-bit number, we need a minimum of 10 bytes, but this gives us 10*7=70 bits of content, hence we have 70-64 = 6 bits extra. If we were to set those extra bits, it might indicate that we wanted a larger number than could be encoded in 64 bits, and we are mistakenly reading it using this function. As of now, the function simply ignores extra bits, constituting a silent truncation.Minor adjustments to #209
Expand
## Immediate issue `./run_tests.sh` does not work: ``` error[E0433]: failed to resolve: use of undeclared crate or module `std` --> quick-protobuf/src/reader.rs:458:13 | 458 | use std::convert::TryFrom; | ^^^ use of undeclared crate or module `std` error[E0599]: no function or associated item named `try_from` found for type `usize` in the current scope --> quick-protobuf/src/reader.rs:480:29 | 480 | let offset = usize::try_from(offset).map_err(|_| Error::SizeOverflow)?; | ^^^^^^^^ function or associated item not found in `usize` | = help: items from traits can only be used if the trait is in scope help: the following trait is implemented but not in scope; perhaps add a `use` for it: | 10 | use core::convert::TryFrom; | ``` ## Other issues - Only `WIRE_TYPE_LENGTH_DELIMITED` needs to be checked by `usize::try_from()`, since `WIRE_TYPE_FIXED32` and `WIRE_TYPE_FIXED64` both give hardcoded values (4 and 8 respectively) within `usize` range. However, all 3 are being checked by `usize::try_from()`, which is a minor inefficiency. - An erroneous `offset` addition may still exceed the range of `self.end - self.start` without actually overflowing `usize` itself; this is arguably a more likely error that we want to check for. - As far as I could tell, `Error::SizeOverflow` is only used with varints, *and* `Error::Varint` is only used (thus far) in overflow situations; so one of them is redundant. ## Adjustments made: - Used `core` instead of `std` for `TryFrom` - Used a stricter check in `read_unknown()` and refactored control flow for efficiency - Removed `Error::SizeOverflow` in favor of `Error::Varint`