From ca3b33a1afc58b84cc7a39ea3845a82f17cee0da Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 12 Feb 2023 21:19:28 -0500 Subject: [PATCH] tor-cell: Refactor relay cells to copy much less We now manipulate raw relay cell bodies as (an alias for) `Box<[u8;509]>` rather than as (an alias for) `[u8;509]`. This enables us to do much less copying. It will become more important soon, as we defer parsing relay cell bodies even longer. Related to #7. We also use SliceWriter to avoid allocating a Vec<> for every relay message we want to encode, and instead encode directly into the cell. --- Cargo.lock | 1 + crates/tor-bytes/src/slicewriter.rs | 9 ++- crates/tor-cell/semver.md | 2 + crates/tor-cell/src/chancell.rs | 5 ++ crates/tor-cell/src/chancell/msg.rs | 19 +++--- crates/tor-cell/src/relaycell.rs | 82 ++++++++++++++++--------- crates/tor-cell/tests/test_relaycell.rs | 4 +- crates/tor-proto/Cargo.toml | 1 + crates/tor-proto/src/circuit.rs | 9 ++- crates/tor-proto/src/circuit/reactor.rs | 4 +- crates/tor-proto/src/crypto/cell.rs | 26 +++----- 11 files changed, 94 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4c8c0e64c..5abf0c3b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4136,6 +4136,7 @@ dependencies = [ "cipher", "coarsetime", "derive_builder_fork_arti", + "derive_more", "digest 0.10.6", "educe", "futures", diff --git a/crates/tor-bytes/src/slicewriter.rs b/crates/tor-bytes/src/slicewriter.rs index b3928a8e9..762f32ab9 100644 --- a/crates/tor-bytes/src/slicewriter.rs +++ b/crates/tor-bytes/src/slicewriter.rs @@ -65,8 +65,15 @@ impl SliceWriter { /// /// On failure (if we tried to write too much), return an error. pub fn try_unwrap(self) -> Result<(T, usize), SliceWriterError> { + let offset = self.offset()?; + Ok((self.data, offset)) + } + + /// Return the number of bytes written into this `SliceWriter` so far, + /// or an error if it has overflowed. + pub fn offset(&self) -> Result { if self.offset != usize::MAX { - Ok((self.data, self.offset)) + Ok(self.offset) } else { Err(SliceWriterError::Truncated) } diff --git a/crates/tor-cell/semver.md b/crates/tor-cell/semver.md index 63d94a85b..4e508fe42 100644 --- a/crates/tor-cell/semver.md +++ b/crates/tor-cell/semver.md @@ -8,3 +8,5 @@ BREAKING: Renamed ChanCell->AnyChanCell, ChanMsg->AnyChanMsg. BREAKING: Renamed RelayCell->AnyRelayCell, RelayMsg->AnyRelayMsg. BREAKING: Make ChannelCodec::decode() parameterized. BREAKING: RelayEarly is now a real type. +BREAKING: RelayCell encoding and decoding functions now expect a Box; + /// Channel-local identifier for a circuit. /// /// A circuit ID can be 2 or 4 bytes long; since version 4 of the Tor diff --git a/crates/tor-cell/src/chancell/msg.rs b/crates/tor-cell/src/chancell/msg.rs index e4d10f7b8..d428a8e62 100644 --- a/crates/tor-cell/src/chancell/msg.rs +++ b/crates/tor-cell/src/chancell/msg.rs @@ -1,6 +1,6 @@ //! Different kinds of messages that can be encoded in channel cells. -use super::{ChanCmd, RawCellBody, CELL_DATA_LEN}; +use super::{BoxedCellBody, ChanCmd, RawCellBody, CELL_DATA_LEN}; use std::net::{IpAddr, Ipv4Addr}; use tor_basic_utils::skip_fmt; use tor_bytes::{self, EncodeError, EncodeResult, Error, Readable, Reader, Result, Writer}; @@ -353,7 +353,7 @@ impl Readable for Created2 { /// /// A different protocol is defined over the relay cells; it is implemented /// in the [crate::relaycell] module. -#[derive(Clone, Educe)] +#[derive(Clone, Educe, derive_more::From)] #[educe(Debug)] pub struct Relay { /// The contents of the relay cell as encoded for transfer. @@ -364,7 +364,7 @@ pub struct Relay { /// necessary happen. We should refactor our data handling until we're mostly /// moving around pointers rather than copying data; see ticket #7. #[educe(Debug(method = "skip_fmt"))] - body: Box, + body: BoxedCellBody, } impl Relay { /// Construct a Relay message from a slice containing its contents. @@ -385,11 +385,10 @@ impl Relay { body: Box::new(body), } } - - /// Consume this Relay message and return a RelayCellBody for + /// Consume this Relay message and return a BoxedCellBody for /// encryption/decryption. - pub fn into_relay_body(self) -> RawCellBody { - *self.body + pub fn into_relay_body(self) -> BoxedCellBody { + self.body } /// Wrap this Relay message into a RelayMsg as a RELAY_EARLY cell. pub fn into_early(self) -> AnyChanMsg { @@ -426,13 +425,13 @@ impl Body for RelayEarly { } } impl RelayEarly { - /// Consume this RelayEarly message and return a RelayCellBody for + /// Consume this RelayEarly message and return a BoxedCellBody for /// encryption/decryption. // // (Since this method takes `self` by value, we can't take advantage of // Deref.) - pub fn into_relay_body(self) -> RawCellBody { - *self.0.body + pub fn into_relay_body(self) -> BoxedCellBody { + self.0.body } } diff --git a/crates/tor-cell/src/relaycell.rs b/crates/tor-cell/src/relaycell.rs index d6d53cd7e..9e0cb1705 100644 --- a/crates/tor-cell/src/relaycell.rs +++ b/crates/tor-cell/src/relaycell.rs @@ -1,7 +1,7 @@ //! Implementation for parsing and encoding relay cells -use crate::chancell::{RawCellBody, CELL_DATA_LEN}; -use tor_bytes::{EncodeResult, Error, Result}; +use crate::chancell::{BoxedCellBody, CELL_DATA_LEN}; +use tor_bytes::{EncodeError, EncodeResult, Error, Result}; use tor_bytes::{Reader, Writer}; use tor_error::internal; @@ -231,54 +231,76 @@ impl RelayCell { } /// Consume this relay message and encode it as a 509-byte padded cell /// body. - pub fn encode(self, rng: &mut R) -> crate::Result { + pub fn encode(self, rng: &mut R) -> crate::Result { /// We skip this much space before adding any random padding to the /// end of the cell const MIN_SPACE_BEFORE_PADDING: usize = 4; - // TODO: This implementation is inefficient; it copies too much. - let encoded = self.encode_to_vec()?; - let enc_len = encoded.len(); - if enc_len > CELL_DATA_LEN { - return Err(crate::Error::Internal(internal!( - "too many bytes in relay cell" - ))); - } - let mut raw = [0_u8; CELL_DATA_LEN]; - raw[0..enc_len].copy_from_slice(&encoded); - + let (mut body, enc_len) = self.encode_to_cell()?; + debug_assert!(enc_len <= CELL_DATA_LEN); if enc_len < CELL_DATA_LEN - MIN_SPACE_BEFORE_PADDING { - rng.fill_bytes(&mut raw[enc_len + MIN_SPACE_BEFORE_PADDING..]); + rng.fill_bytes(&mut body[enc_len + MIN_SPACE_BEFORE_PADDING..]); } - Ok(raw) + Ok(body) } /// Consume a relay cell and return its contents, encoded for use - /// in a RELAY or RELAY_EARLY cell - /// - /// TODO: not the best interface, as this requires copying into a cell. - fn encode_to_vec(self) -> EncodeResult> { - let mut w = Vec::new(); + /// in a RELAY or RELAY_EARLY cell. + fn encode_to_cell(self) -> EncodeResult<(BoxedCellBody, usize)> { + // NOTE: This implementation is a bit optimized, since it happens to + // literally every relay cell that we produce. + + // TODO -NM: Add a specialized implementation for making a DATA cell from + // a body? + + /// Wrap a BoxedCellBody and implement AsMut<[u8]> + struct BodyWrapper(BoxedCellBody); + impl AsMut<[u8]> for BodyWrapper { + fn as_mut(&mut self) -> &mut [u8] { + self.0.as_mut() + } + } + /// The position of the length field within a relay cell. + const LEN_POS: usize = 9; + /// The position of the body a relay cell. + const BODY_POS: usize = 11; + + let body = BodyWrapper(Box::new([0_u8; 509])); + + let mut w = tor_bytes::SliceWriter::new(body); w.write_u8(self.msg.cmd().into()); w.write_u16(0); // "Recognized" w.write_u16(self.streamid.0); w.write_u32(0); // Digest - let len_pos = w.len(); + // (It would be simpler to use NestedWriter at this point, but it uses an internal Vec that we are trying to avoid.) + debug_assert_eq!( + w.offset().expect("Overflowed a cell with just the header!"), + LEN_POS + ); w.write_u16(0); // Length. - let body_pos = w.len(); - self.msg.encode_onto(&mut w)?; - assert!(w.len() >= body_pos); // nothing was removed - let payload_len = w.len() - body_pos; - assert!(payload_len <= std::u16::MAX as usize); - *(array_mut_ref![w, len_pos, 2]) = (payload_len as u16).to_be_bytes(); - Ok(w) + debug_assert_eq!( + w.offset().expect("Overflowed a cell with just the header!"), + BODY_POS + ); + self.msg.encode_onto(&mut w)?; // body + let (mut body, written) = w.try_unwrap().map_err(|_| { + EncodeError::Bug(internal!( + "Encoding of relay message was too long to fit into a cell!" + )) + })?; + let payload_len = written - BODY_POS; + debug_assert!(payload_len < std::u16::MAX as usize); + *(array_mut_ref![body.0, LEN_POS, 2]) = (payload_len as u16).to_be_bytes(); + Ok((body.0, written)) } + /// Parse a RELAY or RELAY_EARLY cell body into a RelayCell. /// /// Requires that the cryptographic checks on the message have already been /// performed - pub fn decode(body: RawCellBody) -> Result { + #[allow(clippy::needless_pass_by_value)] // TODO this will go away soon. + pub fn decode(body: BoxedCellBody) -> Result { let mut reader = Reader::from_slice(body.as_ref()); Self::decode_from_reader(&mut reader) } diff --git a/crates/tor-cell/tests/test_relaycell.rs b/crates/tor-cell/tests/test_relaycell.rs index 404056a72..08496c261 100644 --- a/crates/tor-cell/tests/test_relaycell.rs +++ b/crates/tor-cell/tests/test_relaycell.rs @@ -34,7 +34,7 @@ impl rand::RngCore for BadRng { // I won't tell if you don't. impl rand::CryptoRng for BadRng {} -fn decode(body: &str) -> [u8; CELL_BODY_LEN] { +fn decode(body: &str) -> Box<[u8; CELL_BODY_LEN]> { let mut body = body.to_string(); body.retain(|c| !c.is_whitespace()); let mut body = hex::decode(body).unwrap(); @@ -42,7 +42,7 @@ fn decode(body: &str) -> [u8; CELL_BODY_LEN] { let mut result = [0; CELL_BODY_LEN]; result.copy_from_slice(&body[..]); - result + Box::new(result) } fn cell(body: &str, id: StreamId, msg: AnyRelayMsg) { diff --git a/crates/tor-proto/Cargo.toml b/crates/tor-proto/Cargo.toml index 573903c3c..20df80d8d 100644 --- a/crates/tor-proto/Cargo.toml +++ b/crates/tor-proto/Cargo.toml @@ -33,6 +33,7 @@ bytes = "1" cipher = { version = "0.4.1", features = ["zeroize"] } coarsetime = "0.1.20" derive_builder = { version = "0.11.2", package = "derive_builder_fork_arti" } +derive_more = "0.99.3" digest = "0.10.0" educe = "0.4.6" futures = "0.3.14" diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index 366a3508c..1fc756442 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -815,7 +815,7 @@ mod test { use hex_literal::hex; use std::time::Duration; use tor_basic_utils::test_rng::testing_rng; - use tor_cell::chancell::{msg as chanmsg, AnyChanCell}; + use tor_cell::chancell::{msg as chanmsg, AnyChanCell, BoxedCellBody}; use tor_cell::relaycell::{msg as relaymsg, AnyRelayCell, StreamId}; use tor_linkspec::OwnedCircTarget; use tor_rtcompat::{Runtime, SleepProvider}; @@ -825,11 +825,10 @@ mod test { where ID: Into, { - let body: RelayCellBody = AnyRelayCell::new(id.into(), msg) + let body: BoxedCellBody = AnyRelayCell::new(id.into(), msg) .encode(&mut testing_rng()) - .unwrap() - .into(); - let chanmsg = chanmsg::Relay::from_raw(body.into()); + .unwrap(); + let chanmsg = chanmsg::Relay::from(body); ClientCircChanMsg::Relay(chanmsg) } diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index 0f457691e..1f84ed05a 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -34,7 +34,7 @@ use crate::circuit::sendme::StreamSendWindow; use crate::crypto::handshake::ntor::{NtorClient, NtorPublicKey}; use crate::crypto::handshake::{ClientHandshake, KeyGenerator}; use safelog::sensitive as sv; -use tor_cell::chancell::{self, ChanMsg}; +use tor_cell::chancell::{self, BoxedCellBody, ChanMsg}; use tor_cell::chancell::{AnyChanCell, CircId}; use tor_linkspec::{LinkSpec, OwnedChanTarget, RelayIds}; use tor_llcrypto::pk; @@ -1005,7 +1005,7 @@ impl Reactor { let tag = self.crypto_out.encrypt(&mut body, hop)?; // NOTE(eta): Now that we've encrypted the cell, we *must* either send it or abort // the whole circuit (e.g. by returning an error). - let msg = chancell::msg::Relay::from_raw(body.into()); + let msg = chancell::msg::Relay::from(BoxedCellBody::from(body)); let msg = if early { AnyChanMsg::RelayEarly(msg.into()) } else { diff --git a/crates/tor-proto/src/crypto/cell.rs b/crates/tor-proto/src/crypto/cell.rs index acdc2c13b..82764685c 100644 --- a/crates/tor-proto/src/crypto/cell.rs +++ b/crates/tor-proto/src/crypto/cell.rs @@ -8,25 +8,15 @@ //! use crate::{Error, Result}; -use tor_cell::chancell::RawCellBody; +use tor_cell::chancell::BoxedCellBody; use tor_error::internal; use generic_array::GenericArray; /// Type for the body of a relay cell. -#[derive(Clone)] -pub(crate) struct RelayCellBody(RawCellBody); +#[derive(Clone, derive_more::From, derive_more::Into)] +pub(crate) struct RelayCellBody(BoxedCellBody); -impl From for RelayCellBody { - fn from(body: RawCellBody) -> Self { - RelayCellBody(body) - } -} -impl From for RawCellBody { - fn from(cell: RelayCellBody) -> Self { - cell.0 - } -} impl AsRef<[u8]> for RelayCellBody { fn as_ref(&self) -> &[u8] { &self.0[..] @@ -447,7 +437,7 @@ mod test { let mut rng = testing_rng(); for _ in 1..300 { // outbound cell - let mut cell = [0_u8; 509]; + let mut cell = Box::new([0_u8; 509]); let mut cell_orig = [0_u8; 509]; rng.fill_bytes(&mut cell_orig); cell.copy_from_slice(&cell_orig); @@ -461,7 +451,7 @@ mod test { assert_eq!(&cell.as_ref()[9..], &cell_orig.as_ref()[9..]); // inbound cell - let mut cell = [0_u8; 509]; + let mut cell = Box::new([0_u8; 509]); let mut cell_orig = [0_u8; 509]; rng.fill_bytes(&mut cell_orig); cell.copy_from_slice(&cell_orig); @@ -480,14 +470,14 @@ mod test { // Try a failure: sending a cell to a nonexistent hop. { - let mut cell = [0_u8; 509].into(); + let mut cell = Box::new([0_u8; 509]).into(); let err = cc_out.encrypt(&mut cell, 10.into()); assert!(matches!(err, Err(Error::NoSuchHop))); } // Try a failure: A junk cell with no correct auth from any layer. { - let mut cell = [0_u8; 509].into(); + let mut cell = Box::new([0_u8; 509]).into(); let err = cc_in.decrypt(&mut cell); assert!(matches!(err, Err(Error::BadCellAuth))); } @@ -527,7 +517,7 @@ mod test { let mut j = 0; for cellno in 0..51 { - let mut body = [0_u8; 509]; + let mut body = Box::new([0_u8; 509]); body[0] = 2; // command: data. body[4] = 1; // streamid: 1. body[9] = 1; // length: 498