fix(ethexe/malachite): saturate Fin sequence to avoid overflow in StreamState#5500
fix(ethexe/malachite): saturate Fin sequence to avoid overflow in StreamState#5500grishasobol wants to merge 2 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical vulnerability where a maliciously crafted or extreme sequence value in a Fin message could cause the engine to panic in debug mode or enter an inconsistent state in release mode. By switching to saturating arithmetic, the system now gracefully handles these edge cases, ensuring stability in the stream processing logic. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses a potential panic and overflow vulnerability in the streaming module by replacing unchecked addition with saturating addition when calculating total messages from a peer-controlled sequence number. A regression test has been added to ensure that a sequence number of u64::MAX does not cause a crash. The review feedback correctly identifies that the 'as usize' cast remains a risk on 32-bit platforms where truncation could occur, and suggests using a safer try_into conversion with context as per repository standards.
d656f44 to
7ced688
Compare
There was a problem hiding this comment.
Code Review
This pull request prevents panics or silent wrapping of total_messages in StreamState when receiving a peer-controlled Fin message with a sequence number of u64::MAX by replacing the unchecked addition with saturating_add(1). It also adds a regression test for this scenario. The reviewer suggested using try_into() instead of a direct as usize cast to prevent silent truncation on 32-bit targets.
…eamState A peer-controlled `Fin` with `sequence == u64::MAX` triggered an overflow panic at `streaming.rs:101` under the workspace dev profile (overflow-checks=on) — crashing the engine's app task on a single wire-legal stream message. In release the same input silently wrapped `total_messages` to 0, leaving the slot stuck forever. Switch to `saturating_add` so neither failure mode is possible. Leftover stuck-slot behaviour (counters complete but no Init) is covered by the broader PartStreamsMap caps + GC tracked as #5473. Regression test added. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removed comments and a test related to handling peer-controlled Fin messages with sequence number at u64::MAX.
0ffc97b to
1d7b29a
Compare
No description provided.