Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Use MIN_INITIAL_PACKET_SIZE instead of literal 1200 #1895

Merged
merged 1 commit into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 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 @@
},

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),

Check warning on line 282 in neqo-transport/src/tparams.rs

View check run for this annotation

Codecov / codecov/patch

neqo-transport/src/tparams.rs#L282

Added line #L282 was not covered by tests
_ => 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
Loading