Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

byteorder crate does not play nicely with our "non-blocking" IO #2820

Closed
antiochp opened this issue May 9, 2019 · 2 comments

Comments

@antiochp
Copy link
Member

commented May 9, 2019

Related #2819.

We jump through a lot of hoops with read_exact in our util crate to make sure we can safely and robustly read multiple bytes via non-blocking tcp streams.

But when we call the various low-level byteorder derived fns we skip this and go straight to the read_exact impl in std::io::Read -

https://docs.rs/byteorder/1.3.1/src/byteorder/io.rs.html#110-113

https://doc.rust-lang.org/std/io/trait.Read.html#method.read_exact

It seems like it is entirely possible for a u64 or other multi-byte value here to cause our non-blocking IO to fail on a partial read.
We can be in a situation where we read say 5 bytes of the 8 bytes of a u64 and return early with effectively corrupted data.

@ignopeverell

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Indeed, very good point.

@antiochp

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

So there's actually a couple of layers of indirection going on here.

I think what is happening is the following -

p2p::msg::read_header and p2p::msg::read_body both use our custom read_exact, reading off our non-blocking stream and writing the bytes (all the bytes) to a local vec of bytes.

grin/p2p/src/msg.rs

Lines 156 to 160 in 998824e

pub fn read_body<T: Readable>(h: &MsgHeader, stream: &mut dyn Read) -> Result<T, Error> {
let mut body = vec![0u8; h.msg_len as usize];
read_exact(stream, &mut body, time::Duration::from_secs(20), true)?;
ser::deserialize(&mut &body[..]).map_err(From::from)
}

We then read from this vec of bytes to actually deserialize the appropriate msg header or msg body.
This is when the BinReader comes into play.

And since we only start deserializing once we have a full vec of bytes we can safely make the assumption that we will never read partial bytes. We are not reading off a stream, we are reading from the intermediate vec of bytes which is filled before we begin.

Rust IO really is kind of janky as we do not get any type safety here in terms of blocking/non-blocking behavior.
We pass a Read in but we don't know if read_exact will behave as expected or not as this is based on whether the Read is actually a TcpStream that has had set_nonblocking() called on it or not.

Q) In an ideal world would BinReader support a Read that is non-blocking?
This may be desirable in terms of "streaming" data, say reading a BlockHeader before deserializing the full Block etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.