Skip to content

Commit

Permalink
Don't double-count buffer consumption in close length checks
Browse files Browse the repository at this point in the history
PacketBuilder::max_size previously subtracted out the start index and
header size of the packet, and therefore described the admissible size
of the packet's frames. However, most of our logic operates in terms
of absolute buffer positions instead. This was confusing, and led to
erroneous double-counting of space use in close packets.
  • Loading branch information
Ralith committed Feb 25, 2024
1 parent 1ed8e97 commit cd69fa5
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 11 deletions.
16 changes: 6 additions & 10 deletions quinn-proto/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -759,25 +759,26 @@ impl Connection {
"ACKs should leave space for ConnectionClose"
);
if buf.len() + frame::ConnectionClose::SIZE_BOUND < builder.max_size {
let max_frame_size = builder.max_size - buf.len();
match self.state {
State::Closed(state::Closed { ref reason }) => {
if space_id == SpaceId::Data || reason.is_transport_layer() {
reason.encode(buf, builder.max_size)
reason.encode(buf, max_frame_size)
} else {
frame::ConnectionClose {
error_code: TransportErrorCode::APPLICATION_ERROR,
frame_type: None,
reason: Bytes::new(),
}
.encode(buf, builder.max_size)
.encode(buf, max_frame_size)
}
}
State::Draining => frame::ConnectionClose {
error_code: TransportErrorCode::NO_ERROR,
frame_type: None,
reason: Bytes::new(),
}
.encode(buf, builder.max_size),
.encode(buf, max_frame_size),
_ => unreachable!(
"tried to make a close packet when the connection wasn't closed"
),
Expand Down Expand Up @@ -829,13 +830,8 @@ impl Connection {
}
}

let sent = self.populate_packet(
now,
space_id,
buf,
buf_capacity - builder.tag_len,
builder.exact_number,
);
let sent =
self.populate_packet(now, space_id, buf, builder.max_size, builder.exact_number);

// ACK-only packets should only be sent when explicitly allowed. If we write them due
// to any other reason, there is a bug which leads to one component announcing write
Expand Down
6 changes: 5 additions & 1 deletion quinn-proto/src/connection/packet_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ pub(super) struct PacketBuilder {
pub(super) ack_eliciting: bool,
pub(super) exact_number: u64,
pub(super) short_header: bool,
/// Smallest absolute position in the associated buffer that must be occupied by this packet's
/// frames
pub(super) min_size: usize,
/// Largest absolute position in the associated buffer that may be occupied by this packet's
/// frames
pub(super) max_size: usize,
pub(super) tag_len: usize,
pub(super) span: tracing::Span,
Expand Down Expand Up @@ -147,7 +151,7 @@ impl PacketBuilder {
buffer.len() + (sample_size + 4).saturating_sub(number.len() + tag_len),
partial_encode.start + conn.rem_cids.active().len() + 6,
);
let max_size = buffer_capacity - partial_encode.start - partial_encode.header_len - tag_len;
let max_size = buffer_capacity - tag_len;

Some(Self {
datagram_start,
Expand Down

0 comments on commit cd69fa5

Please sign in to comment.