cloudflare / cloudflare/quiche

zero-length STREAM FIN can be dropped before it is sent

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

Description

`stream_send(id, b"", true)` can set `fin_off` at an already ACKed data
offset, then the stream can be collected before any STREAM frame carrying
`FIN` is emitted.

The peer receives all bytes but never receives final size / EOF.

## Relevant code

`src/stream/send_buf.rs`

```rust
if fin {
self.fin_off = Some(max_off);
}

if self.ack_off() >= max_off {
return Ok(SendReserve {
inner: self,
reserved: 0,
fin,
});
}
```

```rust
pub fn is_complete(&self) -> bool {
if let Some(fin_off) = self.fin_off {
if self.acked == (0..fin_off) {
return true;
}
}

false
}
```

`src/lib.rs`

```rust
stream.send.ack_and_drop(offset, length)
```

The ACK path ignores whether the ACKed STREAM frame carried `FIN`.

## RFC 9000

Exact text:

- Section 3.1: "STREAM frame containing the FIN bit is sent"
- Section 3.1: "all stream data has been successfully acknowledged"
- Section 4.5: "A sender always communicates the final size"
- Section 13.3: "sending ceases when a packet containing that information is acknowledged"
- Section 19.8: "length of 0"
- Section 19.8: "offset of the next byte that would be sent"

For `STREAM offset=N length=0 FIN=true`, the FIN-bearing STREAM frame is the
packet information that communicates final size `N`. ACKing an earlier
data-only STREAM frame for `0..N` is not an ACK of that later FIN frame.

## Reproducer

Paste into `src/tests.rs`:

```rust
#[rstest]
fn stream_empty_fin_is_not_complete_until_fin_acked(
#[values("cubic", "bbr2_gcongestion")] cc_algorithm_name: &str,
) {
let mut buf = [0; 65535];

let mut pipe = test_utils::Pipe::new(cc_algorithm_name).unwrap();
assert_eq!(pipe.handshake(), Ok(()));

assert_eq!(pipe.client.stream_send(4, b"hello", true), Ok(5));
assert_eq!(pipe.advance(), Ok(()));
assert_eq!(pipe.server.stream_recv(4, &mut buf), Ok((5, true)));

assert_eq!(pipe.server.stream_send(4, b"response", false), Ok(8));

let flight = test_utils::emit_flight(&mut pipe.server).unwrap();
test_utils::process_flight(&mut pipe.client, flight).unwrap();

assert_eq!(pipe.client.stream_recv(4, &mut buf), Ok((8, false)));

assert_eq!(pipe.server.stream_send(4, b"", true), Ok(0));

let flight = test_utils::emit_flight(&mut pipe.client).unwrap();
test_utils::process_flight(&mut pipe.server, flight).unwrap();

assert!(!pipe.server.streams.is_collected(4));

let flight = test_utils::emit_flight(&mut pipe.server).unwrap();
let mut frames = Vec::new();

for (mut pkt, _) in flight {
frames.extend(
test_utils::decode_pkt(&mut pipe.client, &mut pkt).unwrap(),
);
}

assert!(frames.iter().any(|f| matches!(
f,
frame::Frame::Stream { stream_id: 4, data }
if data.off() == 8 && data.len() == 0 && data.fin()
)));
}
```

Result on `0.29.2`:

```text
assertion failed: !pipe.server.streams.is_collected(4)
```

## Fix suggestion

Track final-size delivery separately from byte-range ACKs. In particular,
an empty FIN should remain flushable/retransmittable until a FIN-bearing
STREAM frame at `fin_off` has been emitted and ACKed.

Contributor guide

Open the contributing guide

Research direction

Start with `src/stream/send_buf.rs`, especially the FIN setup, completion check, and ACK handling shown in the issue; then inspect the ACK path in `src/lib.rs`. Add the reproducer to `src/tests.rs` and run it with both listed congestion-control algorithms. Done means the stream remains uncollected until the empty FIN at the final offset is emitted and acknowledged.

Written by the indexing model from the issue text.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.