diff --git a/crates/arti-client/src/client.rs b/crates/arti-client/src/client.rs index e5c36bbbb..b7379a853 100644 --- a/crates/arti-client/src/client.rs +++ b/crates/arti-client/src/client.rs @@ -453,7 +453,7 @@ async fn update_persistent_state( debug!("Circmgr has disappeared; task exiting."); return; } - // XXXX This delay is probably too small. + // TODO(nickm): This delay is probably too small. // // Also, we probably don't even want a fixed delay here. Instead, // we should be updating more frequently when the data is volatile diff --git a/crates/arti-config/src/cmdline.rs b/crates/arti-config/src/cmdline.rs index db99ccddf..7f2efea18 100644 --- a/crates/arti-config/src/cmdline.rs +++ b/crates/arti-config/src/cmdline.rs @@ -16,7 +16,9 @@ type Result = std::result::Result; /// the bareword is quoted for convenience. #[derive(Debug, Clone)] pub struct CmdLine { - /// String for decorating Values. (XXXX not yet used). + /// String for decorating Values. + // + // TODO(nickm): not yet used. #[allow(dead_code)] name: String, /// List of toml lines as given on the command line. diff --git a/crates/tor-bytes/src/impls.rs b/crates/tor-bytes/src/impls.rs index 3da2452bb..071bd60a7 100644 --- a/crates/tor-bytes/src/impls.rs +++ b/crates/tor-bytes/src/impls.rs @@ -85,7 +85,7 @@ where for _ in 0..N::to_usize() { v.push(T::take_from(b)?); } - // XXXX I wish I didn't have to clone this. + // TODO(nickm) I wish I didn't have to clone this. Ok(Self::from_slice(v.as_slice()).clone()) } } diff --git a/crates/tor-cell/src/chancell/msg.rs b/crates/tor-cell/src/chancell/msg.rs index 8b7924068..1f80697d8 100644 --- a/crates/tor-cell/src/chancell/msg.rs +++ b/crates/tor-cell/src/chancell/msg.rs @@ -596,7 +596,7 @@ fn take_one_netinfo_addr(r: &mut Reader<'_>) -> Result> { Ok(Some(IpAddr::V4(bytes.into()))) } (0x06, 16) => { - // XXXX is there a better way? + // TODO(nickm) is there a better way? let mut bytes = [0_u8; 16]; (&mut bytes[..]).copy_from_slice(abody); Ok(Some(IpAddr::V6(bytes.into()))) @@ -755,10 +755,10 @@ pub struct PaddingNegotiate { /// Whether to start or stop padding command: u8, /// Suggested lower-bound value for inter-packet timeout in msec. - // XXXX is that right? + // TODO(nickm) is that right? ito_low_ms: u16, /// Suggested upper-bound value for inter-packet timeout in msec. - // XXXX is that right? + // TODO(nickm) is that right? ito_high_ms: u16, } impl PaddingNegotiate { diff --git a/crates/tor-cert/src/lib.rs b/crates/tor-cert/src/lib.rs index 2f1ffc55f..60b27c088 100644 --- a/crates/tor-cert/src/lib.rs +++ b/crates/tor-cert/src/lib.rs @@ -536,7 +536,9 @@ pub struct UncheckedCert { /// The signed text of the certificate. (Checking ed25519 signatures /// forces us to store this. - text: Vec, // XXXX It would be better to store a hash here. + // TODO(nickm) It would be better to store a hash here, but we + // don't have the right Ed25519 API. + text: Vec, /// The alleged signature signature: ed25519::Signature, diff --git a/crates/tor-cert/src/rsa.rs b/crates/tor-cert/src/rsa.rs index c45fc113c..f33b2f66c 100644 --- a/crates/tor-cert/src/rsa.rs +++ b/crates/tor-cert/src/rsa.rs @@ -44,7 +44,7 @@ impl RsaCrosscert { /// Decode a slice of bytes into an RSA crosscert. pub fn decode(bytes: &[u8]) -> tor_bytes::Result { let mut r = Reader::from_slice(bytes); - let signed_portion = r.peek(36)?; // a bit ugly XXXX + let signed_portion = r.peek(36)?; // TODO(nickm): a bit ugly. let subject_key = r.extract()?; let exp_hours = r.take_u32()?; let siglen = r.take_u8()?; diff --git a/crates/tor-chanmgr/src/mgr.rs b/crates/tor-chanmgr/src/mgr.rs index 3d2c81713..d6b8cde1f 100644 --- a/crates/tor-chanmgr/src/mgr.rs +++ b/crates/tor-chanmgr/src/mgr.rs @@ -122,8 +122,7 @@ impl AbstractChanMgr { /// How many times do we try? const N_ATTEMPTS: usize = 2; - // XXXX It would be neat to use tor_retry instead, but it's - // too tied to anyhow right now. + // TODO(nickm): It would be neat to use tor_retry instead. let mut last_err = Err(Error::Internal("Error was never set!?")); for _ in 0..N_ATTEMPTS { @@ -332,7 +331,7 @@ mod test { let cf = FakeChannelFactory::new(runtime); let mgr = AbstractChanMgr::new(cf); - // TODO XXXX: figure out how to make these actually run + // TODO(nickm): figure out how to make these actually run // concurrently. Right now it seems that they don't actually // interact. let (ch3a, ch3b, ch44a, ch44b, ch86a, ch86b) = join!( diff --git a/crates/tor-circmgr/src/path/exitpath.rs b/crates/tor-circmgr/src/path/exitpath.rs index b088cf3bd..edd2c9187 100644 --- a/crates/tor-circmgr/src/path/exitpath.rs +++ b/crates/tor-circmgr/src/path/exitpath.rs @@ -195,10 +195,6 @@ impl<'a> ExitPathBuilder<'a> { /// Returns true if both relays can appear together in the same circuit. fn relays_can_share_circuit(a: &Relay<'_>, b: &Relay<'_>, subnet_config: SubnetConfig) -> bool { - // XXX: features missing from original implementation: - // - option NodeFamilySets - // see: src/feature/nodelist/nodelist.c:nodes_in_same_family() - !a.in_same_family(b) && !a.in_same_subnet(b, &subnet_config) } diff --git a/crates/tor-circmgr/src/timeouts.rs b/crates/tor-circmgr/src/timeouts.rs index 95c2700fe..c313066e7 100644 --- a/crates/tor-circmgr/src/timeouts.rs +++ b/crates/tor-circmgr/src/timeouts.rs @@ -4,7 +4,8 @@ //! user experience. If user wait too long for their circuits, or if //! they use exceptionally slow circuits, then Tor will feel really //! slow. Second, these timeouts are actually a security -//! property. (XXXX explain why!) +//! property. +// TODO(nickm): explain why! use std::time::Duration; diff --git a/crates/tor-dirclient/src/lib.rs b/crates/tor-dirclient/src/lib.rs index 215e2e5f9..8c5687bcd 100644 --- a/crates/tor-dirclient/src/lib.rs +++ b/crates/tor-dirclient/src/lib.rs @@ -109,14 +109,14 @@ where { let circuit = circ_mgr.get_or_launch_dir(dirinfo).await?; - // XXXX should be an option, and is too long. + // TODO(nickm) This should be an option, and is too long. let begin_timeout = Duration::from_secs(5); let source = SourceInfo::new(circuit.unique_id()); // Launch the stream. let mut stream = runtime .timeout(begin_timeout, circuit.begin_dir_stream()) - .await??; // XXXX handle fatalities here too + .await??; // TODO(nickm) handle fatalities here too // TODO: Perhaps we want separate timeouts for each phase of this. // For now, we just use higher-level timeouts in `dirmgr`. @@ -209,7 +209,7 @@ where // response. But this should be fast enough. let n = read_until_limited(stream, b'\n', 2048, &mut buf).await?; - // XXXX Better maximum and/or let this expand. + // TODO(nickm): Better maximum and/or let this expand. let mut headers = [httparse::EMPTY_HEADER; 32]; let mut response = httparse::Response::new(&mut headers); @@ -222,7 +222,7 @@ where return Err(Error::TruncatedHeaders); } - // XXXX Pick a better maximum + // TODO(nickm): Pick a better maximum if buf.len() >= 16384 { return Err(httparse::Error::TooManyHeaders.into()); } @@ -292,8 +292,8 @@ where { let buffer_window_size = 1024; let mut written_total: usize = 0; - // XXXX should be an option and is maybe too long. Though for some - // users this may be too short? + // TODO(nickm): This should be an option, and is maybe too long. + // Though for some users it may be too short? let read_timeout = Duration::from_secs(10); let timer = runtime.sleep(read_timeout).fuse(); futures::pin_mut!(timer); diff --git a/crates/tor-dirmgr/src/lib.rs b/crates/tor-dirmgr/src/lib.rs index eb618ed21..f98fdb3ce 100644 --- a/crates/tor-dirmgr/src/lib.rs +++ b/crates/tor-dirmgr/src/lib.rs @@ -115,7 +115,7 @@ pub struct DirMgr { /// validate them, and so on. config: DirMgrConfig, /// Handle to our sqlite cache. - // XXXX I'd like to use an rwlock, but that's not feasible, since + // TODO(nickm): I'd like to use an rwlock, but that's not feasible, since // rusqlite::Connection isn't Sync. store: Mutex, /// Our latest sufficiently bootstrapped directory, if we have one. diff --git a/crates/tor-dirmgr/src/storage/sqlite.rs b/crates/tor-dirmgr/src/storage/sqlite.rs index a35e81ac9..7928c1da0 100644 --- a/crates/tor-dirmgr/src/storage/sqlite.rs +++ b/crates/tor-dirmgr/src/storage/sqlite.rs @@ -500,7 +500,7 @@ impl SqliteStore { certs: &[AuthCertKeyIds], ) -> Result> { let mut result = HashMap::new(); - // XXXX Do I need to get a transaction here for performance? + // TODO(nickm): Do I need to get a transaction here for performance? let mut stmt = self.conn.prepare(FIND_AUTHCERT)?; for ids in certs { @@ -525,8 +525,8 @@ impl SqliteStore { let mut result = HashMap::new(); let mut stmt = self.conn.prepare(FIND_MD)?; - // XXXX Should I speed this up with a transaction, or does it not - // matter for queries? + // TODO(nickm): Should I speed this up with a transaction, or + // does it not matter for queries? for md_digest in input.into_iter() { let h_digest = hex::encode(md_digest); if let Some(contents) = stmt @@ -551,8 +551,8 @@ impl SqliteStore { let mut result = HashMap::new(); let mut stmt = self.conn.prepare(FIND_RD)?; - // XXXX Should I speed this up with a transaction, or does it not - // matter for queries? + // TODO(nickm): Should I speed this up with a transaction, or + // does it not matter for queries? for rd_digest in input.into_iter() { let h_digest = hex::encode(rd_digest); if let Some(contents) = stmt diff --git a/crates/tor-guardmgr/src/lib.rs b/crates/tor-guardmgr/src/lib.rs index be3fd79b8..494b687b5 100644 --- a/crates/tor-guardmgr/src/lib.rs +++ b/crates/tor-guardmgr/src/lib.rs @@ -1001,7 +1001,9 @@ mod test { #[test] fn simple_waiting() { - // XXXX This test fails in rare cases; I suspect a race condition somewhere. + // TODO(nickm): This test fails in rare cases; I suspect a + // race condition somewhere. + // // I've doubled up on the queue flushing in order to try to make the // race less likely, but we should investigate. test_with_all_runtimes!(|rt| async move { diff --git a/crates/tor-llcrypto/src/pk/rsa.rs b/crates/tor-llcrypto/src/pk/rsa.rs index bb8c334a5..11ba87ca4 100644 --- a/crates/tor-llcrypto/src/pk/rsa.rs +++ b/crates/tor-llcrypto/src/pk/rsa.rs @@ -13,8 +13,7 @@ //! RSA signing is only needed for relays and authorities, and //! RSA-OAEP padding is only needed for the (obsolete) TAP protocol. //! -//! -//! XXXX This module should expose RustCrypto trait-based wrappers, +//! This module should expose RustCrypto trait-based wrappers, //! but the [`rsa`] crate didn't support them as of initial writing. use arrayref::array_ref; use rsa::pkcs1::{FromRsaPrivateKey, FromRsaPublicKey}; @@ -222,8 +221,8 @@ impl PublicKey { /// /// The result is an RsaPublicKey, not a PublicKeyInfo. pub fn to_der(&self) -> Vec { - // There seem to be version issues with these two versions of - // bigint. XXXX + // There seem to be version issues with these two + // versions of bigint: yuck! use rsa::BigUint; // not the same as the one in simple_asn1. use rsa::PublicKeyParts; use simple_asn1::{ASN1Block, BigInt}; diff --git a/crates/tor-llcrypto/src/util.rs b/crates/tor-llcrypto/src/util.rs index cda598a3b..021b2c271 100644 --- a/crates/tor-llcrypto/src/util.rs +++ b/crates/tor-llcrypto/src/util.rs @@ -10,8 +10,8 @@ pub mod rand_compat; /// is an RSA key. /// /// WARNING: Does not validate the X.509 certificate at all! -/// -/// XXXXX This is a massive kludge. +// +// TODO(nickm): This is a massive kludge. pub fn x509_extract_rsa_subject_kludge(der: &[u8]) -> Option { //use ASN1Block::*; let blocks = simple_asn1::from_der(der).ok()?; diff --git a/crates/tor-netdoc/src/doc/netstatus.rs b/crates/tor-netdoc/src/doc/netstatus.rs index 1b58bf9f0..dc474bc03 100644 --- a/crates/tor-netdoc/src/doc/netstatus.rs +++ b/crates/tor-netdoc/src/doc/netstatus.rs @@ -442,7 +442,7 @@ struct Footer { /// Trait to parse a single relay as listed in a consensus document. /// -/// XXXX: I'd rather not have this trait be public, but I haven't yet +/// TODO(nickm): I'd rather not have this trait be public, but I haven't yet /// figured out how to make it private. pub trait ParseRouterStatus: Sized + Sealed { /// Parse this object from a `Section` object containing its diff --git a/crates/tor-netdoc/src/err.rs b/crates/tor-netdoc/src/err.rs index 90d9c7cf1..92ca34887 100644 --- a/crates/tor-netdoc/src/err.rs +++ b/crates/tor-netdoc/src/err.rs @@ -232,13 +232,13 @@ pub enum Error { MissingArgument(Pos), /// We found an argument that couldn't be parsed. #[error("bad argument for entry{0}: {1}")] - BadArgument(Pos, String), // converting to a string doesn't sit well with me. XXXX + BadArgument(Pos, String), /// We found an object that couldn't be parsed after it was decoded. #[error("bad object for entry{0}: {1}")] - BadObjectVal(Pos, String), // converting to a string doesn't sit well with me. XXXX + BadObjectVal(Pos, String), /// There was some signature that we couldn't validate. #[error("couldn't validate signature{0}")] - BadSignature(Pos), // say which kind of signature. TODO + BadSignature(Pos), // TODO(nickm): say which kind of signature. /// There was a tor version we couldn't parse. #[error("couldn't parse Tor version{0}")] BadTorVersion(Pos), @@ -316,8 +316,8 @@ impl Error { /// Helper: return this error's position. pub(crate) fn pos(&self) -> Pos { - // XXXX This duplicate code is yucky. We should refactor this error - // type to use an ErrorKind pattern. + // TODO(nickm): This duplicate code is yucky. We should + // refactor this error type to use an ErrorKind pattern. use Error::*; let pos = match self { Internal(p) => Some(p), diff --git a/crates/tor-proto/src/channel/handshake.rs b/crates/tor-proto/src/channel/handshake.rs index 69c494c71..ee7c695f2 100644 --- a/crates/tor-proto/src/channel/handshake.rs +++ b/crates/tor-proto/src/channel/handshake.rs @@ -284,7 +284,7 @@ impl UnverifiedChannel { // Now look at the signing->TLS cert and check it against the // peer certificate. let (sk_tls, sk_tls_sig) = sk_tls - .check_key(&Some(*signing_key))? // this is a bad interface XXXX + .check_key(&Some(*signing_key))? // TODO(nickm): this is a bad interface .dangerously_split()?; sigs.push(&sk_tls_sig); let sk_tls = sk_tls diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index eb9a4bd57..a2cf3fc54 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -629,7 +629,7 @@ impl Reactor { params: &CircParameters, ) -> Result<()> where - L: CryptInit + ClientLayer + 'static + Send, // need all this?XXXX + L: CryptInit + ClientLayer + 'static + Send, FWD: OutboundClientLayer + 'static + Send, REV: InboundClientLayer + 'static + Send, H: ClientHandshake, @@ -1093,7 +1093,7 @@ impl Reactor { // copy it, but I don't see a way around it right now. let tag = { let mut tag_copy = [0_u8; 20]; - // XXXX This could crash if the tag length changes. We'll + // TODO(nickm): This could crash if the tag length changes. We'll // have to refactor it then. (&mut tag_copy).copy_from_slice(tag); tag_copy diff --git a/crates/tor-proto/src/circuit/sendme.rs b/crates/tor-proto/src/circuit/sendme.rs index 13090d8f1..c7ede51c6 100644 --- a/crates/tor-proto/src/circuit/sendme.rs +++ b/crates/tor-proto/src/circuit/sendme.rs @@ -17,12 +17,13 @@ use tor_cell::relaycell::RelayCell; use crate::{Error, Result}; -// XXXX Three problems with this tag: -// XXXX - First, we need to support unauthenticated flow control. -// XXXX - Second, this tag type could be different for each layer, if we -// XXXX eventually have an authenticator that isn't 20 bytes long. -// XXXX - Third, we want the comparison to happen with a constant-time -// XXXX operation. +// TODO(nickm): +// Three problems with this tag: +// - First, we need to support unauthenticated flow control. +// - Second, this tag type could be different for each layer, if we +// eventually have an authenticator that isn't 20 bytes long. +// - Third, we want the comparison to happen with a constant-time +// operation. XXXX /// Tag type used in regular v1 sendme cells. pub(crate) type CircTag = [u8; 20]; diff --git a/crates/tor-proto/src/crypto/cell.rs b/crates/tor-proto/src/crypto/cell.rs index badca0761..04037b5f7 100644 --- a/crates/tor-proto/src/crypto/cell.rs +++ b/crates/tor-proto/src/crypto/cell.rs @@ -195,7 +195,7 @@ impl InboundClientCrypt { /// Decrypt an incoming cell that is coming to the client. /// /// On success, return which hop was the originator of the cell. - // XXXX use real tag type + // TODO(nickm): Use a real type for the tag, not just `&[u8]`. pub(crate) fn decrypt(&mut self, cell: &mut RelayCellBody) -> Result<(HopNum, &[u8])> { for (hopnum, layer) in self.layers.iter_mut().enumerate() { if let Some(tag) = layer.decrypt_inbound(cell) { @@ -343,7 +343,8 @@ pub(crate) mod tor1 { self.0[8] = 0; d.update(&self.0[..]); - *used_digest = d.clone().finalize(); // XXX can I avoid this clone? + // TODO(nickm) can we avoid this clone? Probably not. + *used_digest = d.clone().finalize(); self.0[5..9].copy_from_slice(&used_digest[0..4]); } /// Check a cell to see whether its recognized field is set. diff --git a/crates/tor-proto/src/crypto/handshake/ntor.rs b/crates/tor-proto/src/crypto/handshake/ntor.rs index 48910a797..8409555cc 100644 --- a/crates/tor-proto/src/crypto/handshake/ntor.rs +++ b/crates/tor-proto/src/crypto/handshake/ntor.rs @@ -254,9 +254,10 @@ where R: RngCore + CryptoRng, T: AsRef<[u8]>, { - // XXXX we generate this key whether or not we are actually going to - // find our nodeid or keyid. Perhaps we should delay that till later. - // But if we do, we'll need to refactor a bit to keep our tests working. + // TODO(nickm): we generate this key whether or not we are + // actually going to find our nodeid or keyid. Perhaps we should + // delay that till later? It shouldn't matter for most cases, + // though. let ephem = EphemeralSecret::new(rng.rng_compat()); let ephem_pub = PublicKey::from(&ephem); diff --git a/crates/tor-proto/src/crypto/handshake/ntor_v3.rs b/crates/tor-proto/src/crypto/handshake/ntor_v3.rs index bbd356165..9d78456c1 100644 --- a/crates/tor-proto/src/crypto/handshake/ntor_v3.rs +++ b/crates/tor-proto/src/crypto/handshake/ntor_v3.rs @@ -326,7 +326,8 @@ fn client_handshake_ntor_v3_no_keygen( /// Trait for an object that handle and incoming client message and /// return a server's reply. /// -// XXXX I wanted to use a closure here, but the lifetimes didn't work. +// TODO(nickm): I wanted to use a closure here, but the lifetimes didn't work, +// and I couldn't figure out why. pub(crate) trait MsgReply { /// Given a message received from a client, parse it and decide /// how (and whether) to reply. diff --git a/crates/tor-proto/src/crypto/ll/kdf.rs b/crates/tor-proto/src/crypto/ll/kdf.rs index 91ab10335..8ec1ba88b 100644 --- a/crates/tor-proto/src/crypto/ll/kdf.rs +++ b/crates/tor-proto/src/crypto/ll/kdf.rs @@ -89,7 +89,7 @@ impl<'a, 'b> Ntor1Kdf<'a, 'b> { impl Kdf for Ntor1Kdf<'_, '_> { fn derive(&self, seed: &[u8], n_bytes: usize) -> Result { - // XXX mark as zero-on-free? + // XXXX mark as zero-on-free? let hkdf = hkdf::Hkdf::::new(Some(self.t_key), seed); let mut result = Zeroizing::new(vec![0; n_bytes]); @@ -107,7 +107,7 @@ impl ShakeKdf { } impl Kdf for ShakeKdf { fn derive(&self, seed: &[u8], n_bytes: usize) -> Result { - // XXX mark as zero-on-free? + // XXXX mark as zero-on-free? let mut xof = Shake256::default(); xof.update(seed); let mut result = Zeroizing::new(vec![0; n_bytes]); diff --git a/crates/tor-rtcompat/src/impls/async_std.rs b/crates/tor-rtcompat/src/impls/async_std.rs index bd150e302..24dbf0da8 100644 --- a/crates/tor-rtcompat/src/impls/async_std.rs +++ b/crates/tor-rtcompat/src/impls/async_std.rs @@ -28,7 +28,9 @@ mod net { pub struct IncomingStreams { /// A state object, stored in an Option so we can take ownership of it /// while poll is being called. - // XXXX I hate using this trick. + // TODO(nickm): I hate using this trick. At some point in the + // future, once Rust has nice support for async traits, maybe + // we can refactor it. state: Option, } /// The result type returned by [`take_and_poll`]. diff --git a/crates/tor-rtmock/src/net.rs b/crates/tor-rtmock/src/net.rs index b6a09d06d..952c527bb 100644 --- a/crates/tor-rtmock/src/net.rs +++ b/crates/tor-rtmock/src/net.rs @@ -179,8 +179,9 @@ impl MockNetwork { }; if let Some(mut entry) = entry { if entry.tls_cert.is_some() != want_tls { - // XXXX This is not what you'd really see on a mismatched - // connection. + // TODO(nickm): This is not what you'd really see on a + // mismatched connection. Maybe we should change this + // to give garbage, or a warning, or something? return Err(err(ErrorKind::ConnectionRefused)); } if entry.send.send((peer_stream, source_addr)).await.is_ok() { diff --git a/crates/tor-rtmock/src/net_runtime.rs b/crates/tor-rtmock/src/net_runtime.rs index a0ec0d746..b3adf63a3 100644 --- a/crates/tor-rtmock/src/net_runtime.rs +++ b/crates/tor-rtmock/src/net_runtime.rs @@ -1,6 +1,7 @@ //! Declare MockNetRuntime. -// XXXX this is mostly copy-paste from MockSleepRuntime. +// TODO(nickm): This is mostly copy-paste from MockSleepRuntime. If possible, +// we should make it so that more code is more shared. use crate::net::MockNetProvider; use tor_rtcompat::{Runtime, SleepProvider, SpawnBlocking, TcpProvider, TlsProvider};