From aecf0641a4917994e58e0fe7a9cfecb596f0c4f6 Mon Sep 17 00:00:00 2001 From: Vova Kryachko Date: Sat, 2 Nov 2024 17:15:07 +0000 Subject: fix(tvix/nix_compat): Fix nix-daemon handshake Existing handshake behavior assumed that the server version is always at least as new as the client. Meaning that the client's version was always picked the handshake details as well as for further communication This change removes that assumption and correctly uses min(server_version, client_version). Change-Id: Ia5dad4613dd5f69a0aeb6c9d86982f1f36fe1a4c Reviewed-on: https://cl.tvl.fyi/c/depot/+/12722 Tested-by: BuildkiteCI Reviewed-by: flokli --- tvix/nix-compat/src/nix_daemon/worker_protocol.rs | 71 ++++++++++++++++++++--- users/picnoir/tvix-daemon/src/main.rs | 6 +- 2 files changed, 66 insertions(+), 11 deletions(-) diff --git a/tvix/nix-compat/src/nix_daemon/worker_protocol.rs b/tvix/nix-compat/src/nix_daemon/worker_protocol.rs index 7e3adc0db2ff..cc99bdb54fab 100644 --- a/tvix/nix-compat/src/nix_daemon/worker_protocol.rs +++ b/tvix/nix-compat/src/nix_daemon/worker_protocol.rs @@ -1,4 +1,5 @@ use std::{ + cmp::min, collections::HashMap, io::{Error, ErrorKind}, }; @@ -209,7 +210,7 @@ pub async fn read_client_settings( /// /// # Return /// -/// The protocol version of the client. +/// The protocol version to use for further comms, min(client_version, our_version). pub async fn server_handshake_client<'a, RW: 'a>( mut conn: &'a mut RW, nix_version: &str, @@ -239,28 +240,29 @@ where format!("The nix client version {} is too old", client_version), )); } - if client_version.minor() >= 14 { + let picked_version = min(PROTOCOL_VERSION, client_version); + if picked_version.minor() >= 14 { // Obsolete CPU affinity. let read_affinity = conn.read_u64_le().await?; if read_affinity != 0 { let _cpu_affinity = conn.read_u64_le().await?; }; } - if client_version.minor() >= 11 { + if picked_version.minor() >= 11 { // Obsolete reserveSpace let _reserve_space = conn.read_u64_le().await?; } - if client_version.minor() >= 33 { + if picked_version.minor() >= 33 { // Nix version. We're plain lying, we're not Nix, but eh… // Setting it to the 2.3 lineage. Not 100% sure this is a // good idea. wire::write_bytes(&mut conn, nix_version).await?; conn.flush().await?; } - if client_version.minor() >= 35 { + if picked_version.minor() >= 35 { write_worker_trust_level(&mut conn, trusted).await?; } - Ok(client_version) + Ok(picked_version) } } @@ -330,11 +332,64 @@ mod tests { // Trusted (1 == client trusted .write(&[1, 0, 0, 0, 0, 0, 0, 0]) .build(); - let client_version = server_handshake_client(&mut test_conn, "2.18.2", Trust::Trusted) + let picked_version = server_handshake_client(&mut test_conn, "2.18.2", Trust::Trusted) .await .unwrap(); - assert_eq!(client_version, PROTOCOL_VERSION) + assert_eq!(picked_version, PROTOCOL_VERSION) + } + + #[tokio::test] + async fn test_init_hanshake_with_newer_client_should_use_older_version() { + let mut test_conn = tokio_test::io::Builder::new() + .read(&WORKER_MAGIC_1.to_le_bytes()) + .write(&WORKER_MAGIC_2.to_le_bytes()) + .write(&[37, 1, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) + // Client is newer than us. + .read(&[38, 1, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) + // cpu affinity + .read(&[0; 8]) + // reservespace + .read(&[0; 8]) + // version (size) + .write(&[0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) + // version (data == 2.18.2 + padding) + .write(&[50, 46, 49, 56, 46, 50, 0, 0]) + // Trusted (1 == client trusted + .write(&[1, 0, 0, 0, 0, 0, 0, 0]) + .build(); + let picked_version = server_handshake_client(&mut test_conn, "2.18.2", Trust::Trusted) + .await + .unwrap(); + + assert_eq!(picked_version, PROTOCOL_VERSION) + } + + #[tokio::test] + async fn test_init_hanshake_with_older_client_should_use_older_version() { + let mut test_conn = tokio_test::io::Builder::new() + .read(&WORKER_MAGIC_1.to_le_bytes()) + .write(&WORKER_MAGIC_2.to_le_bytes()) + .write(&[37, 1, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) + // Client is newer than us. + .read(&[24, 1, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) + // cpu affinity + .read(&[0; 8]) + // reservespace + .read(&[0; 8]) + // NOTE: we are not writing version and trust since the client is too old. + // version (size) + //.write(&[0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) + // version (data == 2.18.2 + padding) + //.write(&[50, 46, 49, 56, 46, 50, 0, 0]) + // Trusted (1 == client trusted + //.write(&[1, 0, 0, 0, 0, 0, 0, 0]) + .build(); + let picked_version = server_handshake_client(&mut test_conn, "2.18.2", Trust::Trusted) + .await + .unwrap(); + + assert_eq!(picked_version, ProtocolVersion::from_parts(1, 24)) } #[tokio::test] diff --git a/users/picnoir/tvix-daemon/src/main.rs b/users/picnoir/tvix-daemon/src/main.rs index dc49b209e009..580f07652097 100644 --- a/users/picnoir/tvix-daemon/src/main.rs +++ b/users/picnoir/tvix-daemon/src/main.rs @@ -67,14 +67,14 @@ where R: AsyncReadExt + AsyncWriteExt + Unpin + std::fmt::Debug, { match server_handshake_client(&mut conn, "2.18.2", Trust::Trusted).await { - Ok(client_protocol_version) => { + Ok(picked_protocol_version) => { let mut client_connection = ClientConnection { conn, - version: client_protocol_version, + version: picked_protocol_version, client_settings: None, }; debug!("Client hanshake succeeded"); - debug!(client_protocol_version = ?client_protocol_version); + debug!(picked_protocol_version = ?picked_protocol_version); // TODO: implement logging. For now, we'll just send // STDERR_LAST, which is good enough to get Nix respond to // us. -- cgit 1.4.1