cloudflare / cloudflare/quiche

Connection leaks state for streams reset before the local side finishes

Open
#2,524 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
11.8k
Forks
1.1k
Avg merge
21h 9m
Merged PRs (30d)
6

Description

Hello

## Summary

Android has been carrying [a memleak patch](https://android.googlesource.com/platform/external/rust/android-crates-io/+/bf11ee061e51cf69a8289a0a1b321e8526329722/crates/quiche/patches/0001-Remove-memleak.patch) for some time on top of v0.17.1.

Current master appears to handle normal finish via the [local_finished](https://github.com/cloudflare/quiche/blob/c4c0b978461aa153399a90217d85bebd1800f84d/quiche/src/h3/mod.rs#L2919) path, but still leaks if a peer resets the stream before the sending is finished.

Each such stream leaves a dangling entry in the `streams` map for the lifetime of the connection. On a long-lived server facing clients that abort request, the map grows without bound, causing a memory leak proportional to the number of aborted requests.

## Root cause

From the above-linked local_finished snippet,, we can observe the cleanup of `h3::Connection::streams` is gated on `local_finished()`:
```rust
fn remove_local_finished_stream(&mut self, stream_id: u64) {
if let hash_map::Entry::Occupied(stream) = self.streams.entry(stream_id) {
if stream.get().local_finished() { // <-- only if WE finished sending
stream.remove();
}
}
}
```

The reset path in `poll()` calls the above helper:

```rust
Err(Error::TransportError(crate::Error::StreamReset(e))) => {
self.remove_local_finished_stream(s); // <-- no-op when !local_finished
return Ok((s, Event::Reset(e)));
},
```

So when a stream is reset while the local side is still open, the `Event::Reset` is delivered correctly, but the `streams` entry is never removed. The duplicate-reset dedup is handled by the transport layer (a second RESET_STREAM yields `Done`), so the leak is silent with no symptom other than unbounded memory growth.

The poll loop even carries an acknowledging TODO at the point the free should happen:

```rust
if conn.stream_finished(s) {
self.process_finished_stream(s);
}
// TODO: check if stream is completed so it can be freed
```

The same gap exists in `pop_finished_stream()`, which can also surface a pending reset (via `Event::Reset`), after calling the gated `remove_local_finished_stream()`.

## Why existing tests miss it

`priority_update_request_collected_stopped` exercises precisely this scenario (request sent with `fin=false`, then client shuts down both directions), but only asserts on the **transport** map (`s.pipe.server.streams.is_collected(0)`); it never checks the **HTTP/3** map (`s.server.streams.len()`), so the dangling H3 entry goes unnoticed.

## Reproduction

Request without fin, no response, then peer reset, then assert the H3 map drains:

```rust
let init = s.server.streams.len();
let (stream, _req) = s.send_request(false).unwrap(); // local side NOT finished
// ... server reads headers ...
s.pipe.client.stream_shutdown(stream, Shutdown::Write, 0x100).unwrap();
s.pipe.client.stream_shutdown(stream, Shutdown::Read, 0x100).unwrap();
s.advance().ok();
assert_eq!(s.poll_server(), Ok((stream, Event::Reset(0x100))));
assert_eq!(s.server.streams.len(), init); // FAILS
```

## Potential fix

I used Claude to create [a fix which generalizes the cleanup helper](https://github.com/10ne1/quiche/commit/21610fb788a7b20f2f2f380eec51f864c4d40e7f) (`remove_local_finished_stream()`) on the latest `master` branch, adding a transport-level `conn.streams.is_collected()` check, so streams that are dead in both directions get freed even when local_finished() never became true.

I'm unsure how good this is, so I will let others review it or come up with other suggestions before opening a PR.

Contributor guide

Open the contributing guide

Research direction

Start in quiche/src/h3/mod.rs by tracing remove_local_finished_stream() through poll() and pop_finished_stream(), especially the StreamReset path. Run priority_update_request_collected_stopped and extend the scenario so it checks the HTTP/3 streams map, not only the transport map. Done means a peer reset delivered as Event::Reset no longer leaves the stream entry behind.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.