search

ConsenSysMesh / cava

ConsenSys core libraries for Java & Kotlin
Apache License 2.0
84 stars 34 forks source link

scuttlebutt-handshake implementation assumes 1 full message is read at once #193

Closed Happy0 closed 5 years ago

Happy0 commented 5 years ago

I don't have steps to reproduce this, but I think it might be an issue (from reading the code.)

A scuttlebutt RPC message has a 34 bit header, followed by a variable length body. When bytes are received over the socket, the current implementation of scuttlebutt-handshake assumes that it is a complete message in the byte buffer handed over by Vertx.

https://github.com/ConsenSys/cava/blob/e8ea2b897a2ff0a0ed4ae9c47300583b0f0874dc/scuttlebutt-handshake/src/main/java/net/consensys/cava/scuttlebutt/handshake/vertx/SecureScuttlebuttVertxClient.java#L62

Depending on network conditions, application load, etc, this could theoretically be only part of a message in this buffer (or perhaps even 1 RPC response and a little bit of another.) Perhaps this could be re-implemented to make sure we buffer up full messages first before handing over the bytes to the receivedMessage method.

atoulme commented 5 years ago

Yes, it's possible. In practice I have seen vert.x buffer multiple messages at once.

atoulme commented 5 years ago

This PR #199 fixes this issue. That indeed happened with patchbay where 512 bytes were sent at a time.