fix(async): handle WebSocket binary frames and attachment collection race condition#506
Open
bneigher wants to merge 2 commits into1c3t3a:mainfrom
Open
fix(async): handle WebSocket binary frames and attachment collection race condition#506bneigher wants to merge 2 commits into1c3t3a:mainfrom
bneigher wants to merge 2 commits into1c3t3a:mainfrom
Conversation
…race condition Two bugs were causing the async Socket.IO client to crash when receiving binary events (e.g. Socket.IO v4 binary attachments over WebSocket): 1. **Binary frame misidentification**: WebSocket binary frames were prefixed with `PacketId::Message` (0x34 = '4'), causing the Engine.IO packet parser to treat raw binary data as a text message. This led to `InvalidUtf8` and `InvalidPacketId` errors. Fix: binary frames are now prefixed with a `'B'` (0x42) marker that is recognized by the packet parser and mapped to `PacketId::MessageBinary`, bypassing text decoding. 2. **Attachment collection race condition**: `handle_engineio_packet` collected binary attachments using `client.clone()`, which created a second consumer on the same underlying stream. This caused the outer iteration loop and the attachment loop to compete for packets, resulting in binary frames being consumed by the wrong loop and misinterpreted as Socket.IO text packets. Fix: attachment collection is now inlined into `stream()` using the same `&mut client`, so packets are consumed sequentially without contention. Additional fixes: - `parse_payload` in async_socket.rs short-circuits for raw binary packets and single packets (no record separator), avoiding unnecessary splitting. - `handle_binary_event` properly extracts the event name from quoted strings in binary event packets instead of stripping all quotes from the data. - `IncompleteResponseFromEngineIo` error now includes the inner error message. Co-authored-by: Cursor <cursoragent@cursor.com>
f736325 to
22d7c2a
Compare
- Export RAW_BINARY_MARKER (pub) so async_socket can import it - Use raw binary marker in packet parsing and websocket_general with comments - Add [lints.rust] unexpected_cfgs allow for tarpaulin in engineio + socketio Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two bugs that cause the async Socket.IO client to crash when receiving binary events (e.g. Socket.IO v4 binary attachments sent as WebSocket binary frames):
1. Binary frame misidentification (
InvalidUtf8/InvalidPacketId)WebSocket binary frames were prefixed with
PacketId::Message(0x34='4'), causing the Engine.IO packet parser to treat raw binary attachment data as a UTF-8 text message. This producedInvalidUtf8orInvalidPacketIderrors on any binary event.Fix: Binary frames are now prefixed with a
'B'(0x42) marker byte that the packet parser recognizes and maps toPacketId::MessageBinary, bypassing text/base64 decoding entirely.Files:
engineio/src/asynchronous/async_transports/websocket_general.rs— use'B'marker instead ofPacketId::Messagefor binary WebSocket framesengineio/src/packet.rs— recognizeb'B'asPacketId::MessageBinary; handle raw binary path inTryFrom<Bytes>engineio/src/asynchronous/async_socket.rs— short-circuitparse_payloadfor raw binary and single packets2. Attachment collection race condition (
EngineIO Error/ lost packets)handle_engineio_packetcollected binary attachments viaclient.clone(), creating a second consumer on the same underlying stream. The outer iteration loop and the attachment loop then raced for incoming packets, causing binary frames to be consumed by the wrong loop and misinterpreted as Socket.IO text packets.Fix:
handle_engineio_packetis removed. Its logic is inlined directly intostream()using&mut client, so attachment packets are consumed sequentially from the same stream without contention.Files:
socketio/src/asynchronous/socket.rs— inline attachment collection intostream(), removehandle_engineio_packetAdditional improvements
socketio/src/asynchronous/client/client.rs—handle_binary_eventnow properly extracts the event name from quoted strings (e.g."tts:audio") instead of stripping all quotes from the entire data field, which broke event routing for binary events with metadata.socketio/src/error.rs—IncompleteResponseFromEngineIonow includes the inner Engine.IO error in its display message ("EngineIO Error: {0}"instead of just"EngineIO Error").Reproduction
Connect a
rust_socketioasync client (WebSocket transport) to a Socket.IO v4 server that emits binary events (e.g.socket.emit("audio", Buffer.from(...))). Without this fix, the client panics or errors with:EngineIO Error(no details)InvalidPacketId(66)(byte0x42from binary data interpreted as packet type)InvalidUtf8(binary data parsed as text)Test plan
zishang520/socket.io) emitting binary events with audio datasocketio/src/asynchronous/client/client.rs)Made with Cursor