Skip to content

fix(h2): close pending handshake on peer close_notify to prevent socket leak - #68

Open
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-h2-close-pending-handshake-on-peer-close-notif-b8d9a6
Open

detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-h2-close-pending-handshake-on-peer-close-notif-b8d9a6

Conversation

@detail-app

@detail-app detail-app Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Detail bug report: View on Detail

Change

Bug: Connection::finish_shutdown() in crates/bemo/src/http/h2.rs only handled State::Connected, silently skipping State::Handshake. When a remote client completed the TLS handshake with ALPN h2 but sent fewer than 24 bytes of the HTTP/2 client preface and then sent TLS close_notify, the driver marked peer_closed and repeatedly called finish_shutdown() — a no-op while the h2 handshake was pending. Since Connection::eof() is compiled out in production (#[cfg(any(test, feature = "fuzzing"))]), the handshake future never resolved, is_closed() never became true, and the ABI should_close condition (is_closed() && wire.is_empty() && !http.sending) never fired. The accepted socket, its HttpSocket, TLS lane state, and a permanent retry-queue slot leaked indefinitely — an unauthenticated slow resource-exhaustion DoS with no idle timeout to reclaim it.

Fix: finish_shutdown() now handles all three State variants. State::Handshake transitions directly to State::Closed, dropping the pending handshake future. The server preface already sits in Wire::output and stays flushable via take_output_limit, so the driver's should_close condition fires once the wire drains and the socket is reclaimed with EVENT_CLOSED.

Affected layers: Rust only — crates/bemo/src/http/h2.rs (the Connection state machine) and crates/bemo/tests/http.rs (integration tests). No ABI, JVM, or Native Image changes.

Closes #61

Validation

  • Unit tests (cargo nextest run -p bemo --locked -E "test(/http::h2::tests::/)"): 22/22 passed, including 3 new tests covering partial-preface + finish_shutdown, no-preface + finish_shutdown, and idempotency after close.
  • Integration tests (cargo nextest run --workspace --locked --test http): 70/71 passed. The 1 failure (arm_receive_failure_leaves_socket_dormant) is pre-existing — confirmed by running it on the baseline commit without this change. Two new tests verify a real rustls client sending a partial (9-byte) and single-byte HTTP/2 preface followed by close_notify produces EVENT_CLOSED within a bounded number of driver polls, with no EVENT_REQUEST dispatched.
  • Full workspace tests (cargo nextest run --workspace --locked --lib --tests): 387/389 passed; both failures (arm_receive_failure_leaves_socket_dormant, persistent_keepalive_reuses_receive_registration_and_fixed_send_state) are pre-existing on the baseline.
  • Miri (cargo miri test -p bemo --locked --no-default-features --features rust-allocator --lib --test buffers --test abi --test pool --test workload): 187 passed, 0 failed (48 Miri-ignored socket/AWS-LC tests). All 3 new unit tests pass under Miri.
  • Fuzz (cargo fuzz run h2 -- -runs=1000): 1000 runs, no crashes. Fuzz seed replay (cargo test --manifest-path fuzz/Cargo.toml --lib): 7/7 passed including replay::h2.
  • Build, format, clippy, lockfiles: cargo build --workspace --locked, cargo fmt -- --check, cargo clippy -p bemo --locked -- -D warnings, and git diff --exit-code -- Cargo.lock fuzz/Cargo.lock all clean.
  • Platform: Linux x86_64, epoll backend.

Automatic Fixes PRs can be configured here.

…et leak

Connection::finish_shutdown() only handled State::Connected, silently
skipping State::Handshake. When a peer completed TLS with ALPN h2 but
sent fewer than 24 bytes of the HTTP/2 client preface and then sent
close_notify, the driver marked peer_closed and repeatedly called
finish_shutdown() — a no-op while the handshake was pending. With
Connection::eof() compiled out in production, the handshake future
never resolved, is_closed() never became true, and the ABI should_close
condition never fired. The accepted socket, its HttpSocket, TLS lane
state, and a permanent retry-queue slot leaked indefinitely — an
unauthenticated slow resource-exhaustion DoS.

Handle State::Handshake in finish_shutdown() by transitioning directly
to State::Closed, dropping the pending handshake future. The server
preface already sits in Wire::output and stays flushable via
take_output_limit, so the driver's should_close condition can fire
once the wire drains.
@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.83%. Comparing base (26fb123) to head (04b868c).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff            @@
##               main      #68   +/-   ##
=========================================
  Coverage     86.82%   86.83%           
+ Complexity      616      615    -1     
=========================================
  Files            60       60           
  Lines         16979    17011   +32     
  Branches        490      490           
=========================================
+ Hits          14742    14771   +29     
- Misses         1913     1917    +4     
+ Partials        324      323    -1     
Flag Coverage Δ
jvm 57.47% <ø> (-0.04%) ⬇️
rust 93.02% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@codspeed

codspeed Bot commented Oct 17, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ 6 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

✅ 117 untouched benchmarks
⏩ 65 skipped benchmarks1


Comparing detail/bug-fix/fix-h2-close-pending-handshake-on-peer-close-notif-b8d9a6 (04b868c) with main (26fb123)

Open in CodSpeed

Footnotes

  1. 65 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Detail Bug] HTTP/2 over TLS: partial client preface + close_notify leaks connections indefinitely

1 participant