vincenzopalazzo
/
arti
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Resolve roughly half of the XXXXs. 

We want to only use TODO in the codebase for non-blockers, and open
tickets for anything that is a bigger blocker than a TODO.  These
XXXXs seem like definite non-blockers to me.

Part of arti#231.
This commit is contained in:
Nick Mathewson 2021-12-06 15:01:29 -05:00
parent e25c2d991e
commit 31b385c5b2
27 changed files with 71 additions and 62 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
crates/arti-client/src/client.rs
View File

@ -453,7 +453,7 @@ async fn update_persistent_state<R: Runtime>(
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

4
crates/arti-config/src/cmdline.rs
View File

@ -16,7 +16,9 @@ type Result<T> = std::result::Result<T, ConfigError>;
/// 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.

2
crates/tor-bytes/src/impls.rs
View File

@ -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())
}
}

6
crates/tor-cell/src/chancell/msg.rs
View File

@ -596,7 +596,7 @@ fn take_one_netinfo_addr(r: &mut Reader<'_>) -> Result<Option<IpAddr>> {
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 {

4
crates/tor-cert/src/lib.rs
View File

@ -536,7 +536,9 @@ pub struct UncheckedCert {
/// The signed text of the certificate. (Checking ed25519 signatures
/// forces us to store this.
text: Vec<u8>, // 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<u8>,
/// The alleged signature
signature: ed25519::Signature,

2
crates/tor-cert/src/rsa.rs
View File

@ -44,7 +44,7 @@ impl RsaCrosscert {
/// Decode a slice of bytes into an RSA crosscert.
pub fn decode(bytes: &[u8]) -> tor_bytes::Result<UncheckedRsaCrosscert> {
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()?;

5
crates/tor-chanmgr/src/mgr.rs
View File

@ -122,8 +122,7 @@ impl<CF: ChannelFactory> AbstractChanMgr<CF> {
/// 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!(

4
crates/tor-circmgr/src/path/exitpath.rs
View File

@ -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)
}

3
crates/tor-circmgr/src/timeouts.rs
View File

@ -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;

12
crates/tor-dirclient/src/lib.rs
View File

@ -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);

2
crates/tor-dirmgr/src/lib.rs
View File

@ -115,7 +115,7 @@ pub struct DirMgr<R: Runtime> {
/// 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<SqliteStore>,
/// Our latest sufficiently bootstrapped directory, if we have one.

10
crates/tor-dirmgr/src/storage/sqlite.rs
View File

@ -500,7 +500,7 @@ impl SqliteStore {
certs: &[AuthCertKeyIds],
) -> Result<HashMap<AuthCertKeyIds, String>> {
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

4
crates/tor-guardmgr/src/lib.rs
View File

@ -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 {

7
crates/tor-llcrypto/src/pk/rsa.rs
View File

@ -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<u8> {
// 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};

4
crates/tor-llcrypto/src/util.rs
View File

@ -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<crate::pk::rsa::PublicKey> {
//use ASN1Block::*;
let blocks = simple_asn1::from_der(der).ok()?;

2
crates/tor-netdoc/src/doc/netstatus.rs
View File

@ -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

10
crates/tor-netdoc/src/err.rs
View File

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

2
crates/tor-proto/src/channel/handshake.rs
View File

@ -284,7 +284,7 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static> UnverifiedChannel<T> {
// 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

4
crates/tor-proto/src/circuit/reactor.rs
View File

@ -629,7 +629,7 @@ impl Reactor {
params: &CircParameters,
) -> Result<()>
where
L: CryptInit + ClientLayer<FWD, REV> + 'static + Send, // need all this?XXXX
L: CryptInit + ClientLayer<FWD, REV> + '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

13
crates/tor-proto/src/circuit/sendme.rs
View File

@ -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];

5
crates/tor-proto/src/crypto/cell.rs
View File

@ -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.

7
crates/tor-proto/src/crypto/handshake/ntor.rs
View File

@ -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);

3
crates/tor-proto/src/crypto/handshake/ntor_v3.rs
View File

@ -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.

4
crates/tor-proto/src/crypto/ll/kdf.rs
View File

@ -89,7 +89,7 @@ impl<'a, 'b> Ntor1Kdf<'a, 'b> {
impl Kdf for Ntor1Kdf<'_, '_> {
fn derive(&self, seed: &[u8], n_bytes: usize) -> Result<SecretBytes> {
// XXX mark as zero-on-free?
// XXXX mark as zero-on-free?
let hkdf = hkdf::Hkdf::<Sha256>::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<SecretBytes> {
// 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]);

4
crates/tor-rtcompat/src/impls/async_std.rs
View File

@ -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<IncomingStreamsState>,
}
/// The result type returned by [`take_and_poll`].

5
crates/tor-rtmock/src/net.rs
View File

@ -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() {

3
crates/tor-rtmock/src/net_runtime.rs
View File

@ -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};