Skip to content

Commit

Permalink
chore: Use MIN_INITIAL_PACKET_SIZE instead of literal 1200 (#1895)
Browse files Browse the repository at this point in the history
  • Loading branch information
larseggert authored May 13, 2024
1 parent 9d05b40 commit d7f33e2
Show file tree
Hide file tree
Showing 15 changed files with 64 additions and 48 deletions.
6 changes: 3 additions & 3 deletions fuzz/fuzz_targets/client_initial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ fuzz_target!(|data: &[u8]| {
let (aead, hp) = initial_aead_and_hp(d_cid, Role::Client);
let (_, pn) = remove_header_protection(&hp, header, payload);

let mut payload_enc = Encoder::with_capacity(1200);
let mut payload_enc = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE);
payload_enc.encode(data); // Add fuzzed data.

// Make a new header with a 1 byte packet number length.
Expand All @@ -49,8 +49,8 @@ fuzz_target!(|data: &[u8]| {
)
.unwrap();
assert_eq!(header_enc.len() + v.len(), ciphertext.len());
// Pad with zero to get up to 1200.
ciphertext.resize(1200, 0);
// Pad with zero to get up to MIN_INITIAL_PACKET_SIZE.
ciphertext.resize(MIN_INITIAL_PACKET_SIZE, 0);

apply_header_protection(
&hp,
Expand Down
6 changes: 3 additions & 3 deletions fuzz/fuzz_targets/server_initial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ fuzz_target!(|data: &[u8]| {
let (aead, hp) = initial_aead_and_hp(d_cid, Role::Server);
let (_, pn) = remove_header_protection(&hp, header, payload);

let mut payload_enc = Encoder::with_capacity(1200);
let mut payload_enc = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE);
payload_enc.encode(data); // Add fuzzed data.

// Make a new header with a 1 byte packet number length.
Expand All @@ -55,8 +55,8 @@ fuzz_target!(|data: &[u8]| {
)
.unwrap();
assert_eq!(header_enc.len() + v.len(), ciphertext.len());
// Pad with zero to get up to 1200.
ciphertext.resize(1200, 0);
// Pad with zero to get up to MIN_INITIAL_PACKET_SIZE.
ciphertext.resize(MIN_INITIAL_PACKET_SIZE, 0);

apply_header_protection(
&hp,
Expand Down
7 changes: 4 additions & 3 deletions neqo-http3/src/connection_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1292,7 +1292,7 @@ mod tests {
use neqo_qpack::{encoder::QPackEncoder, QpackSettings};
use neqo_transport::{
CloseReason, ConnectionEvent, ConnectionParameters, Output, State, StreamId, StreamType,
Version, RECV_BUFFER_SIZE, SEND_BUFFER_SIZE,
Version, MIN_INITIAL_PACKET_SIZE, RECV_BUFFER_SIZE, SEND_BUFFER_SIZE,
};
use test_fixture::{
anti_replay, default_server_h3, fixture_init, new_server, now,
Expand Down Expand Up @@ -7157,8 +7157,9 @@ mod tests {
#[test]
fn priority_update_during_full_buffer() {
// set a lower MAX_DATA on the server side to restrict the data the client can send
let (mut client, mut server) =
connect_with_connection_parameters(ConnectionParameters::default().max_data(1200));
let (mut client, mut server) = connect_with_connection_parameters(
ConnectionParameters::default().max_data(MIN_INITIAL_PACKET_SIZE.try_into().unwrap()),
);

let request_stream_id = make_request_and_exchange_pkts(&mut client, &mut server, false);
let data_writable = |e| matches!(e, Http3ClientEvent::DataWritable { .. });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::{cell::RefCell, rc::Rc, time::Duration};

use neqo_common::event::Provider;
use neqo_crypto::AuthenticationStatus;
use neqo_transport::{ConnectionParameters, StreamId, StreamType};
use neqo_transport::{ConnectionParameters, StreamId, StreamType, MIN_INITIAL_PACKET_SIZE};
use test_fixture::{
anti_replay, fixture_init, now, CountingConnectionIdGenerator, DEFAULT_ADDR, DEFAULT_ALPN_H3,
DEFAULT_KEYS, DEFAULT_SERVER_NAME,
Expand All @@ -25,7 +25,7 @@ use crate::{
WebTransportServerEvent, WebTransportSessionAcceptAction,
};

const DATAGRAM_SIZE: u64 = 1200;
const DATAGRAM_SIZE: u64 = MIN_INITIAL_PACKET_SIZE as u64;

pub fn wt_default_parameters() -> Http3Parameters {
Http3Parameters::default()
Expand Down
18 changes: 11 additions & 7 deletions neqo-transport/src/connection/tests/datagram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ use crate::{
packet::PacketBuilder,
quic_datagrams::MAX_QUIC_DATAGRAM,
send_stream::{RetransmissionPriority, TransmissionPriority},
CloseReason, Connection, ConnectionParameters, Error, StreamType,
CloseReason, Connection, ConnectionParameters, Error, StreamType, MIN_INITIAL_PACKET_SIZE,
};

const DATAGRAM_LEN_MTU: u64 = 1310;
const DATA_MTU: &[u8] = &[1; 1310];
const DATA_BIGGER_THAN_MTU: &[u8] = &[0; 2620];
const DATAGRAM_LEN_SMALLER_THAN_MTU: u64 = 1200;
const DATA_SMALLER_THAN_MTU: &[u8] = &[0; 1200];
const DATAGRAM_LEN_SMALLER_THAN_MTU: u64 = MIN_INITIAL_PACKET_SIZE as u64;
const DATA_SMALLER_THAN_MTU: &[u8] = &[0; MIN_INITIAL_PACKET_SIZE];
const DATA_SMALLER_THAN_MTU_2: &[u8] = &[0; 600];
const OUTGOING_QUEUE: usize = 2;

Expand Down Expand Up @@ -259,7 +259,9 @@ fn datagram_after_stream_data() {

// Create a stream with normal priority and send some data.
let stream_id = client.stream_create(StreamType::BiDi).unwrap();
client.stream_send(stream_id, &[6; 1200]).unwrap();
client
.stream_send(stream_id, &[6; MIN_INITIAL_PACKET_SIZE])
.unwrap();

assert!(
matches!(send_packet_and_get_server_event(&mut client, &mut server), ConnectionEvent::RecvStreamReadable { stream_id: s } if s == stream_id)
Expand Down Expand Up @@ -289,7 +291,9 @@ fn datagram_before_stream_data() {
RetransmissionPriority::default(),
)
.unwrap();
client.stream_send(stream_id, &[6; 1200]).unwrap();
client
.stream_send(stream_id, &[6; MIN_INITIAL_PACKET_SIZE])
.unwrap();

// Write a datagram.
let dgram_sent = client.stats().frame_tx.datagram;
Expand Down Expand Up @@ -440,7 +444,7 @@ fn send_datagram(sender: &mut Connection, receiver: &mut Connection, data: &[u8]

#[test]
fn multiple_datagram_events() {
const DATA_SIZE: usize = 1200;
const DATA_SIZE: usize = MIN_INITIAL_PACKET_SIZE;
const MAX_QUEUE: usize = 3;
const FIRST_DATAGRAM: &[u8] = &[0; DATA_SIZE];
const SECOND_DATAGRAM: &[u8] = &[1; DATA_SIZE];
Expand Down Expand Up @@ -486,7 +490,7 @@ fn multiple_datagram_events() {

#[test]
fn too_many_datagram_events() {
const DATA_SIZE: usize = 1200;
const DATA_SIZE: usize = MIN_INITIAL_PACKET_SIZE;
const MAX_QUEUE: usize = 2;
const FIRST_DATAGRAM: &[u8] = &[0; DATA_SIZE];
const SECOND_DATAGRAM: &[u8] = &[1; DATA_SIZE];
Expand Down
6 changes: 3 additions & 3 deletions neqo-transport/src/connection/tests/vn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use super::{
use crate::{
packet::PACKET_BIT_LONG,
tparams::{self, TransportParameter},
ConnectionParameters, Error, Version,
ConnectionParameters, Error, Version, MIN_INITIAL_PACKET_SIZE,
};

// The expected PTO duration after the first Initial is sent.
Expand All @@ -30,7 +30,7 @@ fn unknown_version() {
mem::drop(client.process(None, now()).dgram());

let mut unknown_version_packet = vec![0x80, 0x1a, 0x1a, 0x1a, 0x1a];
unknown_version_packet.resize(1200, 0x0);
unknown_version_packet.resize(MIN_INITIAL_PACKET_SIZE, 0x0);
mem::drop(client.process(Some(&datagram(unknown_version_packet)), now()));
assert_eq!(1, client.stats().dropped_rx);
}
Expand All @@ -40,7 +40,7 @@ fn server_receive_unknown_first_packet() {
let mut server = default_server();

let mut unknown_version_packet = vec![0x80, 0x1a, 0x1a, 0x1a, 0x1a];
unknown_version_packet.resize(1200, 0x0);
unknown_version_packet.resize(MIN_INITIAL_PACKET_SIZE, 0x0);

assert_eq!(
server.process(Some(&datagram(unknown_version_packet,)), now(),),
Expand Down
9 changes: 6 additions & 3 deletions neqo-transport/src/connection/tests/zerortt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ use super::{
super::Connection, connect, default_client, default_server, exchange_ticket, new_server,
resumed_server, CountingConnectionIdGenerator,
};
use crate::{events::ConnectionEvent, ConnectionParameters, Error, StreamType, Version};
use crate::{
events::ConnectionEvent, ConnectionParameters, Error, StreamType, Version,
MIN_INITIAL_PACKET_SIZE,
};

#[test]
fn zero_rtt_negotiate() {
Expand Down Expand Up @@ -57,8 +60,8 @@ fn zero_rtt_send_recv() {
client.stream_send(client_stream_id, &[1, 2, 3]).unwrap();
let client_0rtt = client.process(None, now());
assert!(client_0rtt.as_dgram_ref().is_some());
// 0-RTT packets on their own shouldn't be padded to 1200.
assert!(client_0rtt.as_dgram_ref().unwrap().len() < 1200);
// 0-RTT packets on their own shouldn't be padded to MIN_INITIAL_PACKET_SIZE.
assert!(client_0rtt.as_dgram_ref().unwrap().len() < MIN_INITIAL_PACKET_SIZE);

let server_hs = server.process(client_hs.as_dgram_ref(), now());
assert!(server_hs.as_dgram_ref().is_some()); // ServerHello, etc...
Expand Down
1 change: 1 addition & 0 deletions neqo-transport/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ pub use self::{
},
events::{ConnectionEvent, ConnectionEvents},
frame::CloseError,
packet::MIN_INITIAL_PACKET_SIZE,
quic_datagrams::DatagramTracking,
recv_stream::{RecvStreamStats, RECV_BUFFER_SIZE},
send_stream::{SendStreamStats, SEND_BUFFER_SIZE},
Expand Down
4 changes: 4 additions & 0 deletions neqo-transport/src/packet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ use crate::{
Error, Res,
};

/// `MIN_INITIAL_PACKET_SIZE` is the smallest packet that can be used to establish
/// a new connection across all QUIC versions this server supports.
pub const MIN_INITIAL_PACKET_SIZE: usize = 1200;

pub const PACKET_BIT_LONG: u8 = 0x80;
const PACKET_BIT_SHORT: u8 = 0x00;
const PACKET_BIT_FIXED_QUIC: u8 = 0x40;
Expand Down
5 changes: 1 addition & 4 deletions neqo-transport/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use crate::{
addr_valid::{AddressValidation, AddressValidationResult},
cid::{ConnectionId, ConnectionIdDecoder, ConnectionIdGenerator, ConnectionIdRef},
connection::{Connection, Output, State},
packet::{PacketBuilder, PacketType, PublicPacket},
packet::{PacketBuilder, PacketType, PublicPacket, MIN_INITIAL_PACKET_SIZE},
ConnectionParameters, Res, Version,
};

Expand All @@ -43,9 +43,6 @@ pub enum InitialResult {
Retry(Vec<u8>),
}

/// `MIN_INITIAL_PACKET_SIZE` is the smallest packet that can be used to establish
/// a new connection across all QUIC versions this server supports.
const MIN_INITIAL_PACKET_SIZE: usize = 1200;
/// The size of timer buckets. This is higher than the actual timer granularity
/// as this depends on there being some distribution of events.
const TIMER_GRANULARITY: Duration = Duration::from_millis(4);
Expand Down
3 changes: 2 additions & 1 deletion neqo-transport/src/tparams.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use neqo_crypto::{

use crate::{
cid::{ConnectionId, ConnectionIdEntry, CONNECTION_ID_SEQNO_PREFERRED, MAX_CONNECTION_ID_LEN},
packet::MIN_INITIAL_PACKET_SIZE,
version::{Version, VersionConfig, WireVersion},
Error, Res,
};
Expand Down Expand Up @@ -278,7 +279,7 @@ impl TransportParameter {
},

MAX_UDP_PAYLOAD_SIZE => match d.decode_varint() {
Some(v) if v >= 1200 => Self::Integer(v),
Some(v) if v >= MIN_INITIAL_PACKET_SIZE.try_into()? => Self::Integer(v),
_ => return Err(Error::TransportParameterError),
},

Expand Down
10 changes: 6 additions & 4 deletions neqo-transport/tests/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
mod common;

use neqo_common::{Datagram, Decoder, Encoder, Role};
use neqo_transport::{CloseReason, ConnectionParameters, Error, State, Version};
use neqo_transport::{
CloseReason, ConnectionParameters, Error, State, Version, MIN_INITIAL_PACKET_SIZE,
};
use test_fixture::{
default_client, default_server,
header_protection::{
Expand Down Expand Up @@ -104,7 +106,7 @@ fn reorder_server_initial() {

// And rebuild a packet.
let mut packet = header.clone();
packet.resize(1200, 0);
packet.resize(MIN_INITIAL_PACKET_SIZE, 0);
aead.encrypt(pn, &header, &plaintext, &mut packet[header.len()..])
.unwrap();
apply_header_protection(&hp, &mut packet, protected_header.len()..header.len());
Expand Down Expand Up @@ -237,7 +239,7 @@ fn overflow_crypto() {
let plen = payload.len();
payload.pad_to(plen + 1000, 44);

let mut packet = Encoder::with_capacity(1200);
let mut packet = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE);
packet
.encode_byte(0xc1) // Initial with packet number length of 2.
.encode_uint(4, Version::Version1.wire_version())
Expand All @@ -254,7 +256,7 @@ fn overflow_crypto() {
aead.encrypt(pn, &header, payload.as_ref(), &mut packet[header.len()..])
.unwrap();
apply_header_protection(&hp, &mut packet, pn_offset..(pn_offset + 2));
packet.resize(1200, 0); // Initial has to be 1200 bytes!
packet.resize(MIN_INITIAL_PACKET_SIZE, 0); // Initial has to be MIN_INITIAL_PACKET_SIZE bytes!

let dgram = Datagram::new(
server_initial.source(),
Expand Down
14 changes: 8 additions & 6 deletions neqo-transport/tests/retry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ use std::{
use common::{connected_server, default_server, generate_ticket};
use neqo_common::{hex_with_len, qdebug, qtrace, Datagram, Encoder, Role};
use neqo_crypto::AuthenticationStatus;
use neqo_transport::{server::ValidateAddress, CloseReason, Error, State, StreamType};
use neqo_transport::{
server::ValidateAddress, CloseReason, Error, State, StreamType, MIN_INITIAL_PACKET_SIZE,
};
use test_fixture::{
assertions, datagram, default_client,
header_protection::{
Expand Down Expand Up @@ -331,15 +333,15 @@ fn retry_after_pto() {
// Let PTO fire on the client and then let it exhaust its PTO packets.
now += Duration::from_secs(1);
let pto = client.process(None, now).dgram();
assert!(pto.unwrap().len() >= 1200);
assert!(pto.unwrap().len() >= MIN_INITIAL_PACKET_SIZE);
let cb = client.process(None, now).callback();
assert_ne!(cb, Duration::new(0, 0));

let retry = server.process(ci.as_ref(), now).dgram();
assertions::assert_retry(retry.as_ref().unwrap());

let ci2 = client.process(retry.as_ref(), now).dgram();
assert!(ci2.unwrap().len() >= 1200);
assert!(ci2.unwrap().len() >= MIN_INITIAL_PACKET_SIZE);
}

#[test]
Expand Down Expand Up @@ -430,11 +432,11 @@ fn mitm_retry() {
qtrace!("notoken_header={}", hex_with_len(&notoken_header));

// Encrypt.
let mut notoken_packet = Encoder::with_capacity(1200)
let mut notoken_packet = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE)
.encode(&notoken_header)
.as_ref()
.to_vec();
notoken_packet.resize_with(1200, u8::default);
notoken_packet.resize_with(MIN_INITIAL_PACKET_SIZE, u8::default);
aead.encrypt(
pn,
&notoken_header,
Expand All @@ -443,7 +445,7 @@ fn mitm_retry() {
)
.unwrap();
// Unlike with decryption, don't truncate.
// All 1200 bytes are needed to reach the minimum datagram size.
// All MIN_INITIAL_PACKET_SIZE bytes are needed to reach the minimum datagram size.

apply_header_protection(&hp, &mut notoken_packet, pn_offset..(pn_offset + pn_len));
qtrace!("packet={}", hex_with_len(&notoken_packet));
Expand Down
15 changes: 8 additions & 7 deletions neqo-transport/tests/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use neqo_crypto::{
use neqo_transport::{
server::{ActiveConnectionRef, Server, ValidateAddress},
CloseReason, Connection, ConnectionParameters, Error, Output, State, StreamType, Version,
MIN_INITIAL_PACKET_SIZE,
};
use test_fixture::{
assertions, datagram, default_client,
Expand Down Expand Up @@ -227,14 +228,14 @@ fn drop_non_initial() {
let mut server = default_server();

// This is big enough to look like an Initial, but it uses the Retry type.
let mut header = neqo_common::Encoder::with_capacity(1200);
let mut header = neqo_common::Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE);
header
.encode_byte(0xfa)
.encode_uint(4, Version::default().wire_version())
.encode_vec(1, CID)
.encode_vec(1, CID);
let mut bogus_data: Vec<u8> = header.into();
bogus_data.resize(1200, 66);
bogus_data.resize(MIN_INITIAL_PACKET_SIZE, 66);

let bogus = datagram(bogus_data);
assert!(server.process(Some(&bogus), now()).dgram().is_none());
Expand Down Expand Up @@ -425,8 +426,8 @@ fn bad_client_initial() {
)
.unwrap();
assert_eq!(header_enc.len() + v.len(), ciphertext.len());
// Pad with zero to get up to 1200.
ciphertext.resize(1200, 0);
// Pad with zero to get up to MIN_INITIAL_PACKET_SIZE.
ciphertext.resize(MIN_INITIAL_PACKET_SIZE, 0);

apply_header_protection(
&hp,
Expand Down Expand Up @@ -488,7 +489,7 @@ fn bad_client_initial_connection_close() {
let (aead, hp) = initial_aead_and_hp(d_cid, Role::Client);
let (_, pn) = remove_header_protection(&hp, header, payload);

let mut payload_enc = Encoder::with_capacity(1200);
let mut payload_enc = Encoder::with_capacity(MIN_INITIAL_PACKET_SIZE);
payload_enc.encode(&[0x1c, 0x01, 0x00, 0x00]); // Add a CONNECTION_CLOSE frame.

// Make a new header with a 1 byte packet number length.
Expand All @@ -513,8 +514,8 @@ fn bad_client_initial_connection_close() {
)
.unwrap();
assert_eq!(header_enc.len() + v.len(), ciphertext.len());
// Pad with zero to get up to 1200.
ciphertext.resize(1200, 0);
// Pad with zero to get up to MIN_INITIAL_PACKET_SIZE.
ciphertext.resize(MIN_INITIAL_PACKET_SIZE, 0);

apply_header_protection(
&hp,
Expand Down
Loading

0 comments on commit d7f33e2

Please sign in to comment.