tor-proto: use InternalError for internal errors.

Cargo.lock

@@ -3256,6 +3256,7 @@ dependencies = [
  "tor-cell",
  "tor-cert",
  "tor-checkable",
+ "tor-error",
  "tor-linkspec",
  "tor-llcrypto",
  "tor-protover",

crates/tor-proto/Cargo.toml

@@ -21,6 +21,7 @@ traffic-timestamp = ["coarsetime"]
 tor-llcrypto = { path = "../tor-llcrypto", version = "0.0.3"}
 tor-bytes = { path = "../tor-bytes", version = "0.0.3"}
 tor-cert = { path = "../tor-cert", version = "0.0.3"}
+tor-error = { path = "../tor-error", version = "0.0.1"}
 tor-linkspec = { path = "../tor-linkspec", version = "0.0.3"}
 tor-checkable = { path = "../tor-checkable", version = "0.0.3"}
 tor-protover = { path = "../tor-protover", version = "0.0.3"}

crates/tor-proto/src/channel.rs

@@ -70,6 +70,7 @@ use crate::util::ts::OptTimestamp;
 use crate::{Error, Result};
 use std::pin::Pin;
 use tor_cell::chancell::{msg, ChanCell, CircId};
+use tor_error::internal;
 use tor_linkspec::ChanTarget;
 use tor_llcrypto::pk::ed25519::Ed25519Identity;
 use tor_llcrypto::pk::rsa::RsaIdentity;
@@ -344,12 +345,12 @@ impl Channel {
         use msg::ChanMsg::*;
         let msg = cell.msg();
         match msg {
-            Created(_) | Created2(_) | CreatedFast(_) => Err(Error::InternalError(format!(
+            Created(_) | Created2(_) | CreatedFast(_) => Err(Error::Internal(internal!(
                 "Can't send {} cell on client channel",
                 msg.cmd()
             ))),
             Certs(_) | Versions(_) | Authenticate(_) | Authorize(_) | AuthChallenge(_)
-            | Netinfo(_) => Err(Error::InternalError(format!(
+            | Netinfo(_) => Err(Error::Internal(internal!(
                 "Can't send {} cell after handshake is done",
                 msg.cmd()
             ))),
@@ -474,17 +475,13 @@ pub(crate) mod test {
             let cell = ChanCell::new(7.into(), msg::Created2::new(&b"hihi"[..]).into());
             let e = chan.check_cell(&cell);
             assert!(e.is_err());
-            assert_eq!(
+            assert!(format!("{}", e.unwrap_err())
-                format!("{}", e.unwrap_err()),
+                .contains("Can't send CREATED2 cell on client channel"));
-                "Internal programming error: Can't send CREATED2 cell on client channel"
-            );
             let cell = ChanCell::new(0.into(), msg::Certs::new_empty().into());
             let e = chan.check_cell(&cell);
             assert!(e.is_err());
-            assert_eq!(
+            assert!(format!("{}", e.unwrap_err())
-                format!("{}", e.unwrap_err()),
+                .contains("Can't send CERTS cell after handshake is done"));
-                "Internal programming error: Can't send CERTS cell after handshake is done"
-            );
 
             let cell = ChanCell::new(5.into(), msg::Create2::new(2, &b"abc"[..]).into());
             let e = chan.check_cell(&cell);

crates/tor-proto/src/channel/handshake.rs

@@ -5,6 +5,7 @@ use asynchronous_codec as futures_codec;
 use futures::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
 use futures::sink::SinkExt;
 use futures::stream::StreamExt;
+use tor_error::internal;
 
 use crate::channel::codec::ChannelCodec;
 use crate::channel::UniqId;
@@ -178,9 +179,9 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static> OutboundClientHandshake
                     if netinfo.is_some() {
                         // This should be impossible, since we would
                         // exit this loop on the first netinfo cell.
-                        return Err(Error::InternalError(
+                        return Err(Error::Internal(internal!(
-                            "Somehow tried to record a duplicate NETINFO cell".into(),
+                            "Somehow tried to record a duplicate NETINFO cell"
-                        ));
+                        )));
                     }
                     netinfo = Some(n);
                     break;

crates/tor-proto/src/channel/reactor.rs

@@ -18,6 +18,7 @@ use futures::channel::{mpsc, oneshot};
 use futures::sink::SinkExt;
 use futures::stream::Stream;
 use futures::Sink;
+use tor_error::internal;
 
 use std::convert::TryInto;
 use std::fmt;
@@ -312,7 +313,9 @@ impl Reactor {
         // TODO(nickm) I think that this one actually means the other side
         // is closed. See arti#269.
         target.send(created).map_err(|_| {
-            Error::InternalError("Circuit queue rejected created message. Is it closing?".into())
+            Error::Internal(internal!(
+                "Circuit queue rejected created message. Is it closing?"
+            ))
         })
     }
 
@@ -332,9 +335,7 @@ impl Reactor {
                     // TODO(nickm) I think that this one actually means the other side
                     // is closed. See arti#269.
                     .map_err(|_| {
-                        Error::InternalError(
+                        internal!("pending circuit wasn't interested in destroy cell?").into()
-                            "pending circuit wasn't interested in Destroy cell?".into(),
-                        )
                     })
             }
             // It's an open circuit: tell it that it got a DESTROY cell.
@@ -345,7 +346,7 @@ impl Reactor {
                     // TODO(nickm) I think that this one actually means the other side
                     // is closed. See arti#269.
                     .map_err(|_| {
-                        Error::InternalError("circuit wasn't interested in destroy cell?".into())
+                        internal!("open circuit wasn't interested in destroy cell?").into()
                     })
             }
             // We've sent a destroy; we can leave this circuit removed.

crates/tor-proto/src/circuit.rs

@@ -60,6 +60,7 @@ use tor_cell::{
     relaycell::msg::{Begin, RelayMsg, Resolve, Resolved, ResolvedVal},
 };
 
+use tor_error::internal;
 use tor_linkspec::{CircTarget, LinkSpec};
 
 use futures::channel::{mpsc, oneshot};
@@ -240,9 +241,9 @@ impl ClientCirc {
 
         let num_hops = self.hops.load(Ordering::SeqCst);
         if num_hops == 0 {
-            return Err(Error::InternalError(
+            return Err(Error::Internal(internal!(
-                "Number of hops specified is zero, cannot continue".into(),
+                "Can't begin a stream at the 0th hop"
-            ));
+            )));
         }
         let hop_num: HopNum = (num_hops - 1).into();
         let (sender, receiver) = mpsc::channel(STREAM_READER_BUFFER);

crates/tor-proto/src/circuit/halfstream.rs

@@ -6,6 +6,7 @@
 use crate::circuit::sendme::{StreamRecvWindow, StreamSendWindow};
 use crate::{Error, Result};
 use tor_cell::relaycell::msg::RelayMsg;
+use tor_error::internal;
 
 /// Type to track state of half-closed streams.
 ///
@@ -66,9 +67,9 @@ impl HalfStream {
                     ))
                 }
             }
-            RelayMsg::End(_) => Err(Error::InternalError(
+            RelayMsg::End(_) => Err(Error::Internal(internal!(
-                "END cell in HalfStream::handle_msg().".into(),
+                "END cell in HalfStream::handle_msg()"
-            )),
+            ))),
             _ => Err(Error::CircProto(format!(
                 "Bad {} cell on a closed stream!",
                 msg.cmd()

crates/tor-proto/src/circuit/reactor.rs

@@ -22,6 +22,7 @@ use tor_cell::relaycell::{RelayCell, RelayCmd, StreamId};
 use futures::channel::{mpsc, oneshot};
 use futures::Sink;
 use futures::Stream;
+use tor_error::internal;
 
 use std::sync::atomic::{AtomicU8, Ordering};
 use std::sync::Arc;
@@ -375,7 +376,13 @@ where
 
         let msg = match msg {
             RelayMsg::Extended2(e) => e,
-            _ => return Err(Error::InternalError("Body didn't match cmd".into())),
+            _ => {
+                return Err(Error::Internal(internal!(
+                    "Message body {:?} didn't match cmd {:?}",
+                    msg,
+                    msg.cmd()
+                )))
+            }
         };
         let relay_handshake = msg.into_body();
 
@@ -1033,9 +1040,9 @@ impl Reactor {
             self.meta_handler = Some((handler, done));
             Ok(())
         } else {
-            Err(Error::InternalError(
+            Err(Error::Internal(internal!(
-                "Tried to install a meta-cell handler before the old one was gone.".into(),
+                "Tried to install a meta-cell handler before the old one was gone."
-            ))
+            )))
         }
     }
 
@@ -1113,9 +1120,10 @@ impl Reactor {
                 let _ = done.send(if let Some(hop) = self.hop_mut(hop) {
                     Ok(hop.sendwindow.window_and_expected_tags())
                 } else {
-                    Err(Error::InternalError(
+                    Err(Error::Internal(internal!(
-                        "received QuerySendWindow for unknown hop".into(),
+                        "received QuerySendWindow for unknown hop {:?}",
-                    ))
+                        hop
+                    )))
                 });
             }
             #[cfg(test)]
@@ -1138,7 +1146,7 @@ impl Reactor {
     ) -> Result<StreamId> {
         let hop = self
             .hop_mut(hopnum)
-            .ok_or_else(|| Error::InternalError(format!("No such hop {:?}", hopnum)))?;
+            .ok_or_else(|| Error::Internal(internal!("No such hop {:?}", hopnum)))?;
         let send_window = StreamSendWindow::new(SEND_WINDOW_INIT);
         let r = hop.map.add_ent(sender, rx, send_window)?;
         let cell = RelayCell::new(r, message);
@@ -1153,7 +1161,10 @@ impl Reactor {
     fn close_stream(&mut self, cx: &mut Context<'_>, hopnum: HopNum, id: StreamId) -> Result<()> {
         // Mark the stream as closing.
         let hop = self.hop_mut(hopnum).ok_or_else(|| {
-            Error::InternalError("Tried to close a stream on a hop that wasn't there?".into())
+            Error::Internal(internal!(
+                "Tried to close a stream on a hop {:?} that wasn't there?",
+                hopnum
+            ))
         })?;
 
         let should_send_end = hop.map.terminate(id)?;
@@ -1238,7 +1249,10 @@ impl Reactor {
             self.send_relay_cell(cx, hopnum, false, cell)?;
             self.hop_mut(hopnum)
                 .ok_or_else(|| {
-                    Error::InternalError("Trying to send SENDME to nonexistent hop".to_string())
+                    Error::Internal(internal!(
+                        "Trying to send SENDME to nonexistent hop {:?}",
+                        hopnum
+                    ))
                 })?
                 .recvwindow
                 .put();

crates/tor-proto/src/circuit/sendme.rs

@@ -14,6 +14,7 @@ use std::collections::VecDeque;
 
 use tor_cell::relaycell::msg::RelayMsg;
 use tor_cell::relaycell::RelayCell;
+use tor_error::internal;
 
 use crate::{Error, Result};
 
@@ -187,7 +188,7 @@ where
         let v = self
             .window
             .checked_add(P::increment())
-            .ok_or_else(|| Error::InternalError("Overflow on SENDME window".into()))?;
+            .ok_or_else(|| Error::Internal(internal!("Overflow on SENDME window")))?;
         self.window = v;
         Ok(v)
     }

crates/tor-proto/src/circuit/streammap.rs

@@ -11,6 +11,7 @@ use tor_cell::relaycell::{msg::RelayMsg, StreamId};
 use futures::channel::mpsc;
 use std::collections::hash_map::Entry;
 use std::collections::HashMap;
+use tor_error::internal;
 
 use rand::Rng;
 
@@ -180,7 +181,7 @@ impl StreamMap {
     pub(super) fn terminate(&mut self, id: StreamId) -> Result<ShouldSendEnd> {
         // Progress the stream's state machine accordingly
         match self.m.remove(&id).ok_or_else(|| {
-            Error::InternalError("Somehow we terminated a nonexistent connection‽".into())
+            Error::Internal(internal!("Somehow we terminated a nonexistent stream?"))
         })? {
             StreamEnt::EndReceived => Ok(ShouldSendEnd::DontSend),
             StreamEnt::Open {

crates/tor-proto/src/crypto/cell.rs

@@ -10,6 +10,7 @@
 use crate::{Error, Result};
 use std::convert::TryInto;
 use tor_cell::chancell::RawCellBody;
+use tor_error::internal;
 
 use generic_array::GenericArray;
 
@@ -258,7 +259,10 @@ pub(crate) mod tor1 {
         }
         fn initialize(seed: &[u8]) -> Result<Self> {
             if seed.len() != Self::seed_len() {
-                return Err(Error::InternalError("length is invalid".to_string()));
+                return Err(Error::Internal(internal!(
+                    "seed length {} was invalid",
+                    seed.len()
+                )));
             }
             let keylen = SC::KeySize::to_usize();
             let dlen = D::OutputSize::to_usize();

crates/tor-proto/src/crypto/handshake/hs_ntor.rs

@@ -38,6 +38,7 @@ use digest::Digest;
 use generic_array::GenericArray;
 use rand_core::{CryptoRng, RngCore};
 use std::convert::TryInto;
+use tor_error::into_internal;
 use tor_llcrypto::cipher::aes::Aes256Ctr;
 use zeroize::Zeroizing;
 
@@ -371,7 +372,8 @@ fn hs_ntor_mac(key: &[u8], message: &[u8]) -> Result<MacTag> {
     let result = d.finalize();
     result
         .try_into()
-        .map_err(|_| Error::InternalError("failed MAC computation".into()))
+        .map_err(into_internal!("failed MAC computation"))
+        .map_err(Error::from)
 }
 
 /// Helper function: Compute the part of the HS ntor handshake that generates
@@ -418,10 +420,12 @@ fn get_introduce1_key_material(
     // Extract the keys into arrays
     let enc_key = hs_keys[0..32]
         .try_into()
-        .map_err(|_| Error::InternalError("converting enc_key".into()))?;
+        .map_err(into_internal!("converting enc_key"))
+        .map_err(Error::from)?;
     let mac_key = hs_keys[32..64]
         .try_into()
-        .map_err(|_| Error::InternalError("converting mac_key".into()))?;
+        .map_err(into_internal!("converting mac_key"))
+        .map_err(Error::from)?;
 
     Ok((enc_key, mac_key))
 }

crates/tor-proto/src/stream/data.rs

@@ -21,6 +21,7 @@ use std::pin::Pin;
 use crate::circuit::StreamTarget;
 use crate::stream::StreamReader;
 use tor_cell::relaycell::msg::{Data, RelayMsg};
+use tor_error::internal;
 
 /// An anonymized stream over the Tor network.
 ///
@@ -188,9 +189,10 @@ impl DataStream {
             self.r.state = Some(DataReaderState::Ready(imp));
             result
         } else {
-            Err(Error::InternalError(
+            Err(Error::Internal(internal!(
-                "Expected ready state of a new stream.".to_owned(),
+                "Expected ready state, got {:?}",
-            ))
+                state
+            )))
         }
     }
 }
@@ -451,6 +453,16 @@ enum DataReaderState {
     ReadingCell(Pin<Box<dyn Future<Output = (DataReaderImpl, Result<()>)> + Send>>),
 }
 
+impl std::fmt::Debug for DataReaderState {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::Closed => write!(f, "Closed"),
+            Self::Ready(_) => write!(f, "Ready"),
+            Self::ReadingCell(_) => write!(f, "ReadingCell"),
+        }
+    }
+}
+
 /// Wrapper for the read part of a DataStream
 struct DataReaderImpl {
     /// The underlying StreamReader object.

crates/tor-proto/src/util/err.rs

@@ -2,6 +2,7 @@
 use std::sync::Arc;
 use thiserror::Error;
 use tor_cell::relaycell::msg::EndReason;
+use tor_error::InternalError;
 
 /// An error type for the tor-proto crate.
 ///
@@ -32,7 +33,7 @@ pub enum Error {
     NoSuchHop,
     /// There was a programming error somewhere in the code.
     #[error("Internal programming error: {0}")]
-    InternalError(String),
+    Internal(#[from] InternalError),
     /// The authentication information on this cell was completely wrong,
     /// or the cell was corrupted.
     #[error("bad relay cell authentication")]
@@ -125,7 +126,7 @@ impl From<Error> for std::io::Error {
             BytesErr(_) | MissingKey | BadCellAuth | BadHandshake | ChanProto(_) | CircProto(_)
             | CellErr(_) | ChanMismatch(_) | StreamProto(_) => ErrorKind::InvalidData,
 
-            InternalError(_) | IdRangeFull | CircExtend(_) | BadConfig(_) | ResolveError(_) => {
+            Internal(_) | IdRangeFull | CircExtend(_) | BadConfig(_) | ResolveError(_) => {
                 ErrorKind::Other
             }
         };