fix(conn): one byte no longer hides a connection from the idle sweep (c10k D2) - #434
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
PR Summary by QodoFix idle-park predicate so partial input can’t evade idle sweep (c10k D2)
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Stale park-cap comment
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // read_buf holds at most MAX_PARKED_REMAINDER bytes | ||
| // (predicate) and `read_buf.split()` below carries | ||
| // them into the parked state, so a partial frame |
There was a problem hiding this comment.
1. Stale park-cap comment 🐞 Bug ⚙ Maintainability
The stage-2 sweep-cancel comment claims the park predicate caps read_buf via “MAX_PARKED_REMAINDER”, but the new park_policy::remainder_allows_park() intentionally does not cap read_buf_len at all. This mismatch can mislead future maintenance/security reviews about what bounds exist at park time.
Agent Prompt
### Issue description
`handle_connection_sharded_monoio`’s stage-2 sweep-cancel path contains a new comment stating that `read_buf` is capped by `MAX_PARKED_REMAINDER` via the park predicate. After this PR, the park predicate is `park_policy::remainder_allows_park(read_buf_len, write_buf_len)`, which deliberately **does not** cap `read_buf_len` (it only enforces `write_buf_len == 0`). The comment is now incorrect and should be updated to reflect the actual bound (upstream `client_query_buffer_limit`) and the intentional “no per-park cap” policy.
### Issue Context
This area is security-sensitive (idle sweep / park behavior). Incorrect comments here can cause reviewers/maintainers to reason about the wrong invariant.
### Fix Focus Areas
- src/server/conn/handler_monoio/mod.rs[905-913]
- src/server/conn/park_policy.rs[49-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
5a2b82e to
4269b87
Compare
…(c10k D2)
The stage-2 park predicate required an EMPTY `read_buf`. A single `*` — the
first character of every RESP array — parses to nothing and therefore sits in
`read_buf` forever, which made the connection unparkable, which dropped it into
the handler's UNREGISTERED plain read at mod.rs:921. Nothing on that path
carries a sweep handle, so the connection became invisible to the idle sweep
for as long as the attacker left the socket open, holding its full stage-2
working set. Cost to the attacker: one byte and one socket, no authentication,
no further traffic. At 1M connections, roughly 10-15 GB that no amount of idle
time reclaims.
Unparsed input no longer blocks a park. The remainder is carried in
`MigratedConnectionState::read_buf_remainder` and re-parsed on resume — exactly
what a migrating connection already did, and what `downshift_idle_buffers` was
already documented to preserve.
DELIBERATELY NOT CAPPED. The first version of this fix capped the carried
remainder at 512 bytes; that was wrong and is worth recording. `read_buf` is
already bounded upstream by `client_query_buffer_limit`, enforced after every
read arm ahead of both parse paths. A second, arbitrary cap would only relocate
the attack into the gap between them — send 513 bytes instead of 1 and the
connection is invisible again, 513x more expensive and still free. A partial
fix here would read as a fix while leaving the vector open. `no_remainder_size_
blocks_a_park` fails if anyone reintroduces a threshold.
`write_buf` stays a strict emptiness check. The buffers are not symmetric:
unparsed input is carried and re-parsed, but a pending reply is carried nowhere,
so parking with one would silently drop bytes the client is owed.
Two properties verified rather than assumed:
- Parking cannot strand a COMPLETE command. mod.rs:789's `carried_input`
guard skips the read whenever unparsed input is present, and its own
comment records that a partial-frame carry "parses to nothing ... and the
flag is already cleared". So reaching the read arms with a non-empty
`read_buf` provably means an incomplete frame.
- The resume path is plumbed: spawn_resumed_parked_conn takes
`read_buf_remainder` into `initial_read_buf` into `read_buf`.
The predicate lives in a new runtime-agnostic `park_policy` module rather than
in `handler_monoio`, because `handler_monoio` is `#[cfg(feature =
"runtime-monoio")]` and EVERY CI test job builds `--no-default-features
--features runtime-tokio`. Tests written in the handler tree would never have
run in CI, and a security predicate guarded by invisible tests is guarded by
nothing.
Scope: this closes the D2 instance, not the class. Connections non-parkable for
other reasons (in_multi, saw_replconf, TLS-unsafe) still reach the same
unregistered read. Those need a live session rather than one anonymous byte;
registering the fallback read itself would close all of them and is left as its
own change.
Testing note: the new integration test is a regression guard, not a red test.
The bug was invisible from the client's side — wire behaviour was identical
either way, only the footprint differed — so it pins the half that IS
observable: a command split across a park must reassemble into exactly one
command.
Gates: park_policy 4/4 under the tokio feature set CI runs, parked_idle_parity
6/6, clippy clean on BOTH feature sets, fmt, audit-unsafe 0 missing,
audit-unwrap 0.
Refs #20
author: Tin Dang
ab552ea to
ebe5786
Compare
1 participant
Stacked on #431 (
fix/c10k-resource-limits). Review that one first; this diff is the 5 files below.The bug
The stage-2 park predicate required an empty
read_buf:A single
*— the first character of every RESP array — parses to nothing, so it sits inread_bufforever. That made the connection unparkable, which dropped it into the handler's unregistered plain read atmod.rs:921. Nothing on that path carries a sweep handle, so the connection became invisible to the idle sweep for as long as the attacker left the socket open, holding its full stage-2 working set.Cost to the attacker: one byte and one socket. No authentication, no further traffic, no CPU. At 1M connections that is roughly 10–15 GB which no amount of idle time reclaims.
The fix
Unparsed input no longer blocks a park. The remainder is carried in
MigratedConnectionState::read_buf_remainderand re-parsed on resume — exactly what a migrating connection already did, and whatdownshift_idle_bufferswas already documented to preserve ("a non-emptyread_bufholds a partial frame that must survive the re-park").Deliberately not capped — and my first attempt got this wrong
My first version capped the carried remainder at 512 bytes. That was wrong, and the reasoning is worth stating because the capped version looks more conservative:
read_bufis already bounded byclient_query_buffer_limit, enforced after every read arm ahead of both parse paths (mod.rs:1001, from cluster C). A second arbitrary cap would only relocate the attack into the gap between the two limits — send 513 bytes instead of 1, pay 513× more, still free, still invisible. A partial fix here would read as a fix while leaving the vector open.no_remainder_size_blocks_a_parkfails if anyone reintroduces a threshold.write_bufstays a strict emptiness check. The buffers are not symmetric: unparsed input is carried and re-parsed, but a pending reply is carried nowhere, so parking with one would silently drop bytes the client is owed.Two properties verified, not assumed
mod.rs:789'scarried_inputguard skips the read whenever unparsed input is present, and its own comment records that a partial-frame carry "parses to nothing ... and the flag is already cleared". So reaching the read arms with a non-emptyread_bufprovably means an incomplete frame.spawn_resumed_parked_conn→read_buf_remainder→initial_read_buf→read_buf.Why the predicate moved out of
handler_monoiohandler_monoiois#[cfg(feature = "runtime-monoio")], and every CI test job builds--no-default-features --features runtime-tokio. Tests written in the handler tree would never have run in CI. A security predicate guarded by invisible tests is guarded by nothing, so the policy lives in a new runtime-agnosticpark_policymodule.Scope and honest limits
This closes the D2 instance, not the class. Connections non-parkable for other reasons (
in_multi,saw_replconf, TLS-unsafe) still reach the same unregistered read. Those need a live session rather than one anonymous byte. Registering the fallback read itself closes all of them — that is the agreed next change.The integration test is a regression guard, not a red test. The bug was invisible from the client's side: wire behaviour was identical either way, only the footprint differed. There is no parked-connection gauge to assert against, so the test pins the half that is observable — a command split across a park must reassemble into exactly one command. Adding that gauge would be a feature, not part of this fix.
Gates
park_policy4/4 under the tokio feature set CI runs ·parked_idle_parity6/6 · clippy clean on both feature sets · fmt ·audit-unsafe.sh0 missing ·audit-unwrap.sh0.Refs #20