Skip to content

Commit

Permalink
Make quinn-proto::{Connection, Endpoint} deterministic (#1691)
Browse files Browse the repository at this point in the history
`quinn-proto::{Connection, Endpoint}` structs call
`StdRng::from_entropy()` in their implementation. It makes it hard to
reproduce errors in an otherwise deterministic test case.

This PR addresses the issue by adding a `StdRng` argument to the
respective `new` functions. For other functions that may create an
endpoint/connection, an argument of type `impl FnOnce() -> StdRng` is
added. This avoids eager calls to generate fresh entropy. e.g. the
`Endpoint::handle` function is frequently called but we'd only need the
rng when handling a new incoming connection.
  • Loading branch information
michael-yxchen committed Oct 23, 2023
1 parent 7bd92f6 commit dd34d57
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 11 deletions.
3 changes: 2 additions & 1 deletion quinn-proto/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ impl Connection {
now: Instant,
version: u32,
allow_mtud: bool,
rng_seed: [u8; 32],
) -> Self {
let side = if server_config.is_some() {
Side::Server
Expand All @@ -267,7 +268,7 @@ impl Connection {
expected_token: Bytes::new(),
client_hello: None,
});
let mut rng = StdRng::from_entropy();
let mut rng = StdRng::from_seed(rng_seed);
let path_validated = server_config.as_ref().map_or(true, |c| c.use_retry);
let mut this = Self {
endpoint_config,
Expand Down
6 changes: 5 additions & 1 deletion quinn-proto/src/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,10 @@ impl Endpoint {
config: Arc<EndpointConfig>,
server_config: Option<Arc<ServerConfig>>,
allow_mtud: bool,
rng_seed: Option<[u8; 32]>,
) -> Self {
Self {
rng: StdRng::from_entropy(),
rng: rng_seed.map_or(StdRng::from_entropy(), StdRng::from_seed),
index: ConnectionIndex::default(),
connections: Slab::new(),
local_cid_generator: (config.connection_id_generator_factory.as_ref())(),
Expand Down Expand Up @@ -589,6 +590,8 @@ impl Endpoint {
server_config: Option<Arc<ServerConfig>>,
transport_config: Arc<TransportConfig>,
) -> Connection {
let mut rng_seed = [0; 32];
self.rng.fill_bytes(&mut rng_seed);
let conn = Connection::new(
self.config.clone(),
server_config,
Expand All @@ -603,6 +606,7 @@ impl Endpoint {
now,
version,
self.allow_mtud,
rng_seed,
);

let id = self.connections.insert(ConnectionMeta {
Expand Down
27 changes: 21 additions & 6 deletions quinn-proto/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ use util::*;
fn version_negotiate_server() {
let _guard = subscribe();
let client_addr = "[::2]:7890".parse().unwrap();
let mut server = Endpoint::new(Default::default(), Some(Arc::new(server_config())), true);
let mut server = Endpoint::new(
Default::default(),
Some(Arc::new(server_config())),
true,
None,
);
let now = Instant::now();
let event = server.handle(
now,
Expand Down Expand Up @@ -63,6 +68,7 @@ fn version_negotiate_client() {
}),
None,
true,
None,
);
let (_, mut client_ch) = client
.connect(Instant::now(), client_config(), server_addr, "localhost")
Expand Down Expand Up @@ -178,7 +184,8 @@ fn server_stateless_reset() {
let mut pair = Pair::new(endpoint_config.clone(), server_config());
let (client_ch, _) = pair.connect();
pair.drive(); // Flush any post-handshake frames
pair.server.endpoint = Endpoint::new(endpoint_config, Some(Arc::new(server_config())), true);
pair.server.endpoint =
Endpoint::new(endpoint_config, Some(Arc::new(server_config())), true, None);
// Force the server to generate the smallest possible stateless reset
pair.client.connections.get_mut(&client_ch).unwrap().ping();
info!("resetting");
Expand All @@ -203,7 +210,8 @@ fn client_stateless_reset() {

let mut pair = Pair::new(endpoint_config.clone(), server_config());
let (_, server_ch) = pair.connect();
pair.client.endpoint = Endpoint::new(endpoint_config, Some(Arc::new(server_config())), true);
pair.client.endpoint =
Endpoint::new(endpoint_config, Some(Arc::new(server_config())), true, None);
// Send something big enough to allow room for a smaller stateless reset.
pair.server.connections.get_mut(&server_ch).unwrap().close(
pair.time,
Expand Down Expand Up @@ -1343,8 +1351,9 @@ fn cid_rotation() {
}),
Some(Arc::new(server_config())),
true,
None,
);
let client = Endpoint::new(Arc::new(EndpointConfig::default()), None, true);
let client = Endpoint::new(Arc::new(EndpointConfig::default()), None, true, None);

let mut pair = Pair::new_from_endpoint(client, server);
let (_, server_ch) = pair.connect();
Expand Down Expand Up @@ -1922,7 +1931,12 @@ fn big_cert_and_key() -> (rustls::Certificate, rustls::PrivateKey) {
fn malformed_token_len() {
let _guard = subscribe();
let client_addr = "[::2]:7890".parse().unwrap();
let mut server = Endpoint::new(Default::default(), Some(Arc::new(server_config())), true);
let mut server = Endpoint::new(
Default::default(),
Some(Arc::new(server_config())),
true,
None,
);
server.handle(
Instant::now(),
client_addr,
Expand Down Expand Up @@ -2024,12 +2038,13 @@ fn migrate_detects_new_mtu_and_respects_original_peer_max_udp_payload_size() {
Arc::new(server_endpoint_config),
Some(Arc::new(server_config())),
true,
None,
);
let client_endpoint_config = EndpointConfig {
max_udp_payload_size: VarInt::from(client_max_udp_payload_size),
..EndpointConfig::default()
};
let client = Endpoint::new(Arc::new(client_endpoint_config), None, true);
let client = Endpoint::new(Arc::new(client_endpoint_config), None, true, None);
let mut pair = Pair::new_from_endpoint(client, server);
pair.mtu = 1300;

Expand Down
9 changes: 7 additions & 2 deletions quinn-proto/src/tests/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,13 @@ impl Pair {
}

pub(super) fn new(endpoint_config: Arc<EndpointConfig>, server_config: ServerConfig) -> Self {
let server = Endpoint::new(endpoint_config.clone(), Some(Arc::new(server_config)), true);
let client = Endpoint::new(endpoint_config, None, true);
let server = Endpoint::new(
endpoint_config.clone(),
Some(Arc::new(server_config)),
true,
None,
);
let client = Endpoint::new(endpoint_config, None, true, None);

Self::new_from_endpoint(client, server)
}
Expand Down
7 changes: 6 additions & 1 deletion quinn/src/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,12 @@ impl Endpoint {
let allow_mtud = !socket.may_fragment();
let rc = EndpointRef::new(
socket,
proto::Endpoint::new(Arc::new(config), server_config.map(Arc::new), allow_mtud),
proto::Endpoint::new(
Arc::new(config),
server_config.map(Arc::new),
allow_mtud,
None,
),
addr.is_ipv6(),
runtime.clone(),
);
Expand Down

0 comments on commit dd34d57

Please sign in to comment.