Skip to content

Commit 883ca63

Browse files
authored
fix(proto): Avoid double-emitting Draining event, causing an active_connections underflow (#748)
## Description - Fixes a bug where the `Draining` event was emitted twice when a stateless reset token was duplicated and received twice - Adds a regression test for the above scenario This is one of those bugs that triggers an underflow in `active_connections` in noq. ## Breaking Changes None ## Notes & open questions I'm still investigating whether there are further such cases. ## Change checklist - [x] Self-review. - [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [x] Tests if relevant. - [x] All breaking changes documented.
1 parent d0456ce commit 883ca63

5 files changed

Lines changed: 82 additions & 3 deletions

File tree

noq-proto/src/connection/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4408,7 +4408,7 @@ impl Connection {
44084408
..
44094409
}) => {
44104410
let was_draining = self.state.move_to_drained(Some(conn_err));
4411-
if !was_draining {
4411+
if !was_draining && !was_drained {
44124412
self.endpoint_events.push_back(EndpointEventInner::Draining);
44134413
}
44144414
}

noq-proto/src/tests/mod.rs

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,77 @@ fn stateless_reset_limit() {
309309
assert!(matches!(event, Some(DatagramEvent::Response(_))));
310310
}
311311

312+
/// Regression test to ensure a connection that is already `Drained` doesn't emit a
313+
/// duplicate `Draining` endpoint event when a second stateless-reset datagram is processed.
314+
#[test]
315+
fn duplicate_stateless_reset_emits_single_draining() {
316+
let _guard = subscribe();
317+
let mut key_material = vec![0; 64];
318+
let mut rng = rand::rng();
319+
rng.fill_bytes(&mut key_material);
320+
let reset_key = hmac::Key::new(hmac::HMAC_SHA256, &key_material);
321+
rng.fill_bytes(&mut key_material);
322+
323+
let mut endpoint_config = EndpointConfig::new(Arc::new(reset_key));
324+
endpoint_config.cid_generator(Arc::new(move || {
325+
Box::new(HashedConnectionIdGenerator::from_key(0))
326+
}));
327+
let endpoint_config = Arc::new(endpoint_config);
328+
329+
let mut pair = Pair::new(endpoint_config.clone(), server_config());
330+
let (client_ch, _) = pair.connect();
331+
pair.drive(); // Flush any post-handshake frames
332+
333+
// Recreate the server endpoint so it loses all connection state but keeps the same
334+
// reset key, causing it to respond to the client's packets with stateless resets.
335+
pair.server.endpoint = Endpoint::new(endpoint_config, Some(Arc::new(server_config())), true);
336+
// Force the server to generate the smallest possible stateless reset
337+
pair.client.connections.get_mut(&client_ch).unwrap().ping();
338+
pair.drive_client();
339+
pair.drive_server();
340+
341+
// Capture the stateless reset datagram delivered to the client before it is processed.
342+
let (_, captured_stateless_reset) = pair
343+
.client
344+
.inbound
345+
.pop_first()
346+
.expect("server should have sent a stateless reset");
347+
pair.client.inbound.clear();
348+
349+
let now = pair.time;
350+
351+
// Duplicate the captured stateless reset token:
352+
pair.client
353+
.inbound
354+
.push(now, captured_stateless_reset.clone());
355+
pair.client.inbound.push(now, captured_stateless_reset);
356+
357+
// Only call drive_incoming instead of drive_client so we can manually count
358+
// endpoint events below.
359+
pair.client.drive_incoming(now);
360+
361+
// Apply the connection events produced by the endpoint and collect all endpoint events
362+
// emitted by the connection.
363+
let conn = pair.client.connections.get_mut(&client_ch).unwrap();
364+
for (_, mut events) in pair.client.conn_events.drain() {
365+
for event in events.drain(..) {
366+
conn.handle_event(event);
367+
}
368+
}
369+
370+
// A drained connection receiving a second stateless reset must not emit a duplicate
371+
// `Draining` event.
372+
let mut draining = 0;
373+
let mut drained = 0;
374+
while let Some(event) = conn.poll_endpoint_events() {
375+
draining += event.is_draining() as usize;
376+
drained += event.is_drained() as usize;
377+
}
378+
379+
assert_eq!(draining, 1, "expected exactly one Draining event");
380+
assert_eq!(drained, 1, "expected exactly one Drained event");
381+
}
382+
312383
#[test]
313384
fn export_keying_material() {
314385
let _guard = subscribe();

noq-proto/src/tests/util.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1097,15 +1097,15 @@ pub(super) struct TestEndpoint {
10971097
pub(super) accepted: Option<Result<ConnectionHandle, ConnectionError>>,
10981098
pub(super) connections: HashMap<ConnectionHandle, Connection>,
10991099
pub(super) draining_connections: HashSet<ConnectionHandle>,
1100-
conn_events: HashMap<ConnectionHandle, VecDeque<ConnectionEvent>>,
1100+
pub(super) conn_events: HashMap<ConnectionHandle, VecDeque<ConnectionEvent>>,
11011101
pub(super) captured_packets: Vec<Vec<u8>>,
11021102
pub(super) capture_inbound_packets: bool,
11031103
#[debug("handle_incoming")]
11041104
pub(super) handle_incoming: Box<dyn FnMut(&Incoming) -> IncomingConnectionBehavior>,
11051105
pub(super) waiting_incoming: Vec<Incoming>,
11061106
}
11071107

1108-
#[derive(Debug)]
1108+
#[derive(Debug, Clone)]
11091109
pub(super) struct Inbound {
11101110
pub(super) ecn: Option<EcnCodepoint>,
11111111
pub(super) packet: BytesMut,

noq-udp/build.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
use cfg_aliases::cfg_aliases;
22

3+
#[allow(
4+
semicolon_in_expressions_from_macros,
5+
reason = "cfg_aliases needs an update: https://github.com/katharostech/cfg_aliases/pull/15"
6+
)]
37
fn main() {
48
// Setup cfg aliases
59
cfg_aliases! {

noq/build.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
use cfg_aliases::cfg_aliases;
22

3+
#[allow(
4+
semicolon_in_expressions_from_macros,
5+
reason = "cfg_aliases needs an update: https://github.com/katharostech/cfg_aliases/pull/15"
6+
)]
37
fn main() {
48
// Setup cfg aliases
59
cfg_aliases! {

0 commit comments

Comments
 (0)