BurntSushi / BurntSushi/byteorder
Hazard with `ReadBytesExt` methods and continuing after errors
- Dominant language
- Rust
- Stars
- 1.1k
- Forks
- 161
- PR merge metrics
- No merged PRs in 30d
Description
I don't think this is a bug in `byteorder`. But I thought it demonstrates an interesting hazard that might be worth documenting for others.
Some `byteorder::ReadBytesExt` methods rely on [`std::io::Read::read_exact`](https://doc.rust-lang.org/std/io/trait.Read.html#method.read_exact), which has a warning about the error case:
> If this function encounters an “end of file” before completely filling the buffer, it returns an error of the kind `ErrorKind::UnexpectedEof`. The contents of buf are unspecified in this case.
>
> ... If this function returns an error, it is unspecified how many bytes it has read, but it will never read more than would be necessary to completely fill the buffer.
This means that e.g. `byteorder::read_u16` has the same hazard: if it encounters the end of the reader, the reader is now in an unspecified state.
So code like this is wrong:
```rust
pub fn parse_packet(packet: &[u8]) -> Vec {
let mut result = Vec::new();
let mut reader = Cursor::new(packet);
// Read as many u16s as we can
while let Ok(word) = reader.read_u16::() {
result.push(word);
}
// If there was a byte left over, take that too.
if let Ok(odd_end_val) = reader.read_u8() {
result.push(odd_end_val.into())
}
result
}
```
I recently stumbled over code that did exactly this, because the standard library changed its (unspecified) behavior. In older versions, the function above behaved as expected (remainder bytes stayed in the reader after the error). In rust 1.80.0, the remainder bytes are consumed. The following unit test demonstrates (comment out the assert for your version):
```rust
#[test]
fn test_odd() {
let input = [1, 2, 3];
let output = parse_packet(&input);
// Rust <= 1.79
assert_eq!(output, [0x0201, 0x0003]);
// Rust 1.80
assert_eq!(output, [0x0201]);
}
```
The `byteorder::ReadBytesExt` methods are [documented](https://docs.rs/byteorder/1.5.0/byteorder/trait.ReadBytesExt.html#method.read_u16) to track the `read_exact` error behavior:
> ### Errors
>
> This method returns the same errors as `Read::read_exact`.
But maybe more should be done? I'm not sure how often this pattern might exist in the wild. Maybe the docs should be extended to warn people that the input stream can't be relied on after an error is returned?
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the ReadBytesExt documentation linked in the issue and compare its error wording with std::io::Read::read_exact. Review the test_odd example and determine how the documentation should warn about continuing after an error. Done means the relevant method documentation clearly describes the reader state hazard and the existing behavior is covered or explained.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- documentation
- Issue type
- Documentation
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100