Handle very rare stream failure when packets contain magic bytes - #104
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe stream buffer now validates the payload at the declared packet length before rejecting an apparent internal header. A regression test verifies that both the packet and a following recovery packet decode successfully. ChangesStream buffer validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized parser change is intended to prevent valid packets containing protocol magic bytes from stalling the stream; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ProjectMoon
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/connections/stream_buffer.rs`:
- Around line 747-785: Update
resync_after_false_header_match_with_implausible_size to embed [0x94, 0xc3, 254,
6] within the declared payload of a valid length-delimited packet, rather than
appending it between encoded packets. Assert that this affected packet and the
following packet are both delivered and the buffer is empty; if the test fails,
adjust validate_packet_in_buffer framing/resynchronization so the outer valid
frame is preserved instead of drained as MalformedPacket.
- Around line 45-46: Update match patterns for StreamBufferError::DecodeFailure
to use the tuple-variant form DecodeFailure(..), matching its declaration with
the prost::DecodeError payload.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e577820d-07ca-472f-8ba9-29955f7169ba
📒 Files selected for processing (1)
src/connections/stream_buffer.rs
|
OK, new solution, as the previous would drop the packet. Now both packets are decoded, and also there's a test with the actual type of packet that can cause this. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/connections/stream_buffer.rs`:
- Around line 764-770: Update the collision assertion for encoded_outer to
inspect only the payload bytes after PACKET_HEADER_SIZE, then scan those bytes
for the [0x94, 0xc3] window. Preserve the existing assertion message and
collision expectation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4db51d2-92ae-40b9-ada0-6dea9378bf22
📒 Files selected for processing (1)
src/connections/stream_buffer.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| // Only malformed if it doesn't decode at the declared length. | ||
| let declared_packet = | ||
| &self.buffer[packet_data_start_index..packet_data_start_index + packet_data_size]; | ||
| if protobufs::FromRadio::decode(declared_packet).is_ok() { |
There was a problem hiding this comment.
FromRadio::decode is an expensive operation. The chance of calling it by accident is only 1/65536 (1/2^16), though, so this should be OK - assuming the payloads are mostly random.
A malicious actor can send payloads containing [0x94, 0xc3] to trigger this code and burn CPU cycles, but this can only happen on public meshes. Private meshes are protected by passwords.
Let me think about this a bit more, I'd like to know why the code looks for the next packet inside of the current packet. Maybe there's a more elegant way of doing this.
There was a problem hiding this comment.
A better way would be a full state machine, I would think. If we don't see an end marker, but encounter magic bytes start marker, then we have to assume those bytes are part of the current packet?
Co-authored-by: Lukáš Poláček <lukas@polacek.email>
Co-authored-by: Lukáš Poláček <lukas@polacek.email>
Co-authored-by: Lukáš Poláček <lukas@polacek.email>
2 participants
Summary
Fixes a very rare situation where the stream parser can start attempting to parse a legitimate packet's bytes as a new frame/packet when that packet contains the Meshtastic protocol magic bytes, and then the parser just starts eating all incoming bytes as part of that frame, and never completing the packet emission. This completely halts the stream.
Proposed Changes
The approach in this PR recognizes strange packet sizes and is like "no way."Checklist
Summary by CodeRabbit
Summary by CodeRabbit