Skip to content

Commit

Permalink
Fix race condition between 0-RTT and Incoming
Browse files Browse the repository at this point in the history
Closes quinn-rs#1820

The fix:

- Endpoint now maintains a slab with an entry for each pending Incoming
  to buffer received data.
- ConnectionIndex now maps initial DCID to that slab key immediately
  upon construction of Incoming.
- If Incoming is accepted, association is overridden with association
  with ConnectionHandle, and all buffered datagrams are fed to newly
  constructed Connection.
- If Incoming is refused/retried/ignored, or accepting errors,
  association and slab entry are cleaned up to prevent memory leak.

Additional considerations:

- The Incoming::ignore operation can no longer be implemented as just
  dropping it. To help prevent incorrect API usage, proto::Incoming is
  modified to log a warning if it is dropped without being passed to
  Endpoint::accept/refuse/retry/ignore.
- Three things protect against memory exhaustion attacks here:

  1. The MAX_INCOMING_CONNECTIONS limit is moved from quinn to proto,
     limiting the number of concurrent incoming connections for which
     datagrams will be buffered before the application decides what to
     do with them.
  2. Per-incoming buffered data is limited to the maximum initially
     receivable stream data as per the transport parameters, plus one
     datagram, after which subsequent packets are discarded if received
     in these conditions.
  3. The sum total of all incoming buffered data is limited to a new
     MAX_ALL_INCOMING_BUFFERED constant, after which subsequence packets
     are discarded if received in these conditions.
  • Loading branch information
gretchenfrage committed Apr 15, 2024
1 parent 82a67db commit bfc9b75
Show file tree
Hide file tree
Showing 7 changed files with 198 additions and 52 deletions.
12 changes: 6 additions & 6 deletions quinn-proto/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ use crate::{
packet::{Header, InitialHeader, InitialPacket, LongType, Packet, PartialDecode, SpaceId},
range_set::ArrayRangeSet,
shared::{
ConnectionEvent, ConnectionEventInner, ConnectionId, EcnCodepoint, EndpointEvent,
EndpointEventInner,
ConnectionEvent, ConnectionEventInner, ConnectionId, DatagramConnectionEvent, EcnCodepoint,
EndpointEvent, EndpointEventInner,
},
token::ResetToken,
transport_parameters::TransportParameters,
Expand Down Expand Up @@ -989,13 +989,13 @@ impl Connection {
pub fn handle_event(&mut self, event: ConnectionEvent) {
use self::ConnectionEventInner::*;
match event.0 {
Datagram {
Datagram(DatagramConnectionEvent {
now,
remote,
ecn,
first_decode,
remaining,
} => {
}) => {
// If this packet could initiate a migration and we're a client or a server that
// forbids migration, drop the datagram. This could be relaxed to heuristically
// permit NAT-rebinding-like migration.
Expand Down Expand Up @@ -3346,11 +3346,11 @@ impl Connection {
#[cfg(test)]
pub(crate) fn decode_packet(&self, event: &ConnectionEvent) -> Option<Vec<u8>> {
let (first_decode, remaining) = match &event.0 {
ConnectionEventInner::Datagram {
ConnectionEventInner::Datagram(DatagramConnectionEvent {
first_decode,
remaining,
..
} => (first_decode, remaining),
}) => (first_decode, remaining),
_ => return None,
};

Expand Down
Loading

0 comments on commit bfc9b75

Please sign in to comment.