Fix snafu backtraces, add backtrace to some errors

NoRoom, SSHProto, RanOut get backtraces
mkj · May 30, 2024 · 557003d · 557003d
1 parent de161d1
commit 557003d
Show file tree

Hide file tree

Showing 10 changed files with 48 additions and 43 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -21,7 +21,7 @@ members = [
 [dependencies]
 sunset-sshwire-derive = { version = "0.2", path = "sshwire-derive" }
 
-snafu = { version = "0.8", default-features = false, features = ["rust_1_61"] }
+snafu = { version = "0.8", default-features = false, features = ["rust_1_65"] }
 # TODO: check that log macro calls disappear in no_std builds
 log = { version = "0.4" }
 heapless = "0.8"

diff --git a/src/channel.rs b/src/channel.rs
@@ -332,7 +332,7 @@ impl Channels {
                     }
                     _ => {
                         trace!("Bad channel state {:?}", ch.state);
-                        return Err(Error::SSHProtoError)
+                        return error::SSHProto.fail()
                     }
                 }
             }
@@ -342,7 +342,7 @@ impl Channels {
                 if ch.send.is_some() {
                     // TODO: or just warn?
                     trace!("open failure late?");
-                    return Err(Error::SSHProtoError);
+                    return error::SSHProto.fail();
                 } else {
                     self.remove(ChanNum(p.num))?;
                     // TODO event

diff --git a/src/cliauth.rs b/src/cliauth.rs
@@ -232,7 +232,7 @@ impl CliAuth {
                 if let Req::PubKey { ref key } = last_req {
                     if key.pubkey() != pkok.key.0 {
                         trace!("Received pkok for a different key");
-                        return Err(Error::SSHProtoError);
+                        return error::SSHProto.fail()
                     }
 
                     // Sign the packet without the signature
@@ -248,7 +248,7 @@ impl CliAuth {
             _ => (),
         }
         trace!("Unexpected userauth60");
-        Err(Error::SSHProtoError)
+        error::SSHProto.fail()
     }
 
     fn change_password(&self) -> Result<()> {

diff --git a/src/conn.rs b/src/conn.rs
@@ -256,7 +256,7 @@ impl Conn {
                 let num = p.message_num() as u8;
                 let a = self.dispatch_packet(p, s);
                 match a {
-                    | Err(Error::SSHProtoError)
+                    | Err(Error::SSHProto { .. })
                     | Err(Error::PacketWrong)
                     => debug!("Error handling {num} packet"),
                     _ => (),
@@ -283,7 +283,7 @@ impl Conn {
                 packets::Category::Kex => Ok(()),
                 _ => {
                     debug!("Non-kex packet during strict kex");
-                    Err(Error::SSHProtoError)
+                    error::SSHProto.fail()
                 },
             }
         } else if !matches!(self.kex, Kex::Idle) {
@@ -293,7 +293,7 @@ impl Conn {
                 packets::Category::Kex => Ok(()),
                 _ => {
                     debug!("Invalid packet during kex");
-                    Err(Error::SSHProtoError)
+                    error::SSHProto.fail()
                 },
             }
         } else {
@@ -306,14 +306,14 @@ impl Conn {
                         | ConnState::PreAuth
                         | ConnState::Authed
                         => Ok(()),
-                        _ => Err(Error::SSHProtoError),
+                        _ => error::SSHProto.fail(),
                     }
                 }
                 packets::Category::Sess => {
                     match self.state {
                         ConnState::Authed
                         => Ok(()),
-                        _ => Err(Error::SSHProtoError),
+                        _ => error::SSHProto.fail(),
                     }
                 }
             }
@@ -354,7 +354,7 @@ impl Conn {
                 if self.cliserv.is_client() {
                     // TODO: client/server validity checks should move somewhere more general
                     trace!("kexdhinit not server");
-                    return Err(Error::SSHProtoError);
+                    return error::SSHProto.fail();
                 }
 
                 disp.event = self.kex.handle_kexdhinit()?;
@@ -363,7 +363,7 @@ impl Conn {
                 if !self.cliserv.is_client() {
                     // TODO: client/server validity checks should move somewhere more general
                     trace!("kexdhreply not server");
-                    return Err(Error::SSHProtoError);
+                    return error::SSHProto.fail();
                 }
 
                 disp.event = self.kex.handle_kexdhreply();
@@ -382,7 +382,7 @@ impl Conn {
                     serv.service_request(&p, s)?;
                 } else {
                     debug!("Server sent a service request");
-                    return Err(Error::SSHProtoError)
+                    return error::SSHProto.fail()
                 }
             }
             Packet::ServiceAccept(p) => {
@@ -413,15 +413,15 @@ impl Conn {
                     disp.event = serv.auth.request(sess_id, s, p)?;
                 } else {
                     debug!("Server sent an auth request");
-                    return Err(Error::SSHProtoError)
+                    return error::SSHProto.fail()
                 }
             }
             Packet::UserauthFailure(p) => {
                 if let ClientServer::Client(cli) = &mut self.cliserv {
                     disp.event = cli.auth.failure(&p, &mut self.parse_ctx, s)?;
                 } else {
                     debug!("Received UserauthFailure as a server");
-                    return Err(Error::SSHProtoError)
+                    return error::SSHProto.fail()
                 }
             }
             Packet::UserauthSuccess(_) => {
@@ -434,15 +434,15 @@ impl Conn {
                     }
                 } else {
                     debug!("Received UserauthSuccess as a server");
-                    return Err(Error::SSHProtoError)
+                    return error::SSHProto.fail()
                 }
             }
             Packet::UserauthBanner(p) => {
                 if let ClientServer::Client(cli) = &mut self.cliserv {
                     cli.banner(&p);
                 } else {
                     debug!("Received banner as a server");
-                    return Err(Error::SSHProtoError)
+                    return error::SSHProto.fail()
                 }
             }
             Packet::Userauth60(p) => {
@@ -452,7 +452,7 @@ impl Conn {
                     cli.auth.auth60(&p, sess_id, &mut self.parse_ctx, s)?;
                 } else {
                     debug!("Received userauth60 as a server");
-                    return Err(Error::SSHProtoError)
+                    return error::SSHProto.fail()
                 }
             }
             | Packet::ChannelOpen(_)

diff --git a/src/encrypt.rs b/src/encrypt.rs
@@ -283,7 +283,7 @@ impl Keys {
 
         if buf.len() < size_block + size_integ {
             debug!("Bad packet, {} smaller than block size", buf.len());
-            return Err(Error::SSHProtoError);
+            return error::SSHProto.fail();
         }
         // "MUST be a multiple of the cipher block size".
         // encrypted length for aead ciphers doesn't include the length prefix.
@@ -292,7 +292,7 @@ impl Keys {
 
         if len % size_block != 0 {
             debug!("Bad packet, not multiple of block size");
-            return Err(Error::SSHProtoError);
+            return error::SSHProto.fail();
         }
 
         let (data, mac) = buf.split_at_mut(buf.len() - size_integ);
@@ -327,15 +327,15 @@ impl Keys {
         let padlen = data[SSH_LENGTH_SIZE] as usize;
         if padlen < SSH_MIN_PADLEN {
             debug!("Packet padding too short");
-            return Err(Error::SSHProtoError);
+            return error::SSHProto.fail();
         }
 
         let payload_len = buf
             .len()
             .checked_sub(SSH_LENGTH_SIZE + 1 + size_integ + padlen)
             .ok_or_else(|| {
                 debug!("Bad padding length");
-                Error::SSHProtoError
+                error::SSHProto.build()
             })?;
 
         Ok(payload_len)
@@ -385,7 +385,7 @@ impl Keys {
 
         if len + size_integ > buf.len() {
             error!("Output buffer {} is too small for packet", buf.len());
-            return Err(Error::NoRoom);
+            return error::NoRoom.fail();
         }
 
         // write the length

diff --git a/src/error.rs b/src/error.rs
@@ -5,7 +5,7 @@ use log::{debug, error, info, log, trace, warn};
 use core::fmt::Arguments;
 use core::fmt;
 
-use snafu::{prelude::*, Location};
+use snafu::{prelude::*, Backtrace, Location};
 
 use heapless::String;
 
@@ -21,10 +21,14 @@ use crate::channel::ChanNum;
 #[snafu(visibility(pub))]
 pub enum Error {
     /// Output buffer ran out of room
-    NoRoom,
+    NoRoom {
+        backtrace: Backtrace,
+    },
 
     /// Input buffer ran out
-    RanOut,
+    RanOut {
+        backtrace: Backtrace,
+    },
 
     /// Not a UTF-8 string
     BadString,
@@ -45,7 +49,9 @@ pub enum Error {
     BadNumber,
 
     /// Error in received SSH protocol. Will disconnect.
-    SSHProtoError,
+    SSHProto {
+        backtrace: Backtrace,
+    },
 
     /// Peer sent something we don't handle. Will disconnect.
     ///
@@ -85,7 +91,6 @@ pub enum Error {
     ///   (millions of times) and not released.
     // TODO: /// #[snafu(display("Failure from application: {msg}"))]
     BadUsage {
-        #[snafu(implicit)]
         backtrace: snafu::Backtrace,
         // TODO
         // msg: &'static str,

diff --git a/src/ident.rs b/src/ident.rs
@@ -1,4 +1,4 @@
-use crate::error::{Error,TrapBug, Result};
+use crate::error::{self, Error,TrapBug, Result};
 
 pub(crate) const OUR_VERSION: &[u8] = b"SSH-2.0-Sunset-1";
 
@@ -16,7 +16,7 @@ pub(crate) fn write_version(buf: &mut [u8]) -> Result<usize> {
 
     let total_len = OUR_VERSION.len() + 2;
     if total_len > buf.len() {
-        return Err(Error::NoRoom);
+        return error::NoRoom.fail();
     }
 
     let (d, b) = buf.split_at_mut(OUR_VERSION.len());
@@ -94,7 +94,7 @@ impl RemoteVersion {
 
             match self.st {
                 VersPars::Start(ref mut pos) => {
-                    let w = self.storage.get_mut(*pos).ok_or(Error::NoRoom)?;
+                    let w = self.storage.get_mut(*pos).ok_or(error::NoRoom.build())?;
                     *w = b;
                     *pos += 1;
                     // Check if line so far matches SSH-2.0-
@@ -134,7 +134,7 @@ impl RemoteVersion {
                         return Err(Error::msg("bad remote version"));
                     }
                     _ => {
-                        let w = self.storage.get_mut(*pos).ok_or(Error::NoRoom)?;
+                        let w = self.storage.get_mut(*pos).ok_or(error::NoRoom.build())?;
                         *w = b;
                         *pos += 1;
                     }

diff --git a/src/server.rs b/src/server.rs
@@ -30,7 +30,7 @@ impl Server {
             s.send(ServiceAccept { name: p.name })
         } else {
             warn!("Received unexpected service request '{}'", p.name);
-            Err(Error::SSHProtoError)
+            error::SSHProto.fail()
         }
     }
 }
diff --git a/src/sshwire.rs b/src/sshwire.rs
@@ -86,7 +86,7 @@ pub enum WireError {
 
     PacketWrong,
 
-    SSHProtoError,
+    SSHProto,
 
     BadKeyFormat,
 
@@ -96,11 +96,11 @@ pub enum WireError {
 impl From<WireError> for Error {
     fn from(w: WireError) -> Self {
         match w {
-            WireError::NoRoom => Error::NoRoom,
-            WireError::RanOut => Error::RanOut,
+            WireError::NoRoom => error::NoRoom.build(),
+            WireError::RanOut => error::RanOut.build(),
             WireError::BadString => Error::BadString,
             WireError::BadName => Error::BadName,
-            WireError::SSHProtoError => Error::SSHProtoError,
+            WireError::SSHProto => error::SSHProto.build(),
             WireError::PacketWrong => Error::PacketWrong,
             WireError::BadKeyFormat => Error::BadKeyFormat,
             WireError::UnknownVariant => Error::bug_err_msg("Can't encode Unknown"),
@@ -415,7 +415,7 @@ impl<'de, B: SSHDecode<'de>> SSHDecode<'de> for Blob<B> {
         // Sanity check the length matched
         let used_len = pos2 - pos1;
         if used_len != len {
-            let extra = len.checked_sub(used_len).ok_or(WireError::SSHProtoError)?;
+            let extra = len.checked_sub(used_len).ok_or(WireError::SSHProto)?;
 
             if s.ctx().seen_unknown {
                 // Skip over unconsumed bytes in the blob.
@@ -425,7 +425,7 @@ impl<'de, B: SSHDecode<'de>> SSHDecode<'de> for Blob<B> {
                 trace!("SSH blob length differs. \
                     Expected {} bytes, got {} bytes {}..{}",
                     len, used_len, pos1, pos2);
-                return Err(WireError::SSHProtoError)
+                return Err(WireError::SSHProto)
             }
         }
         Ok(Blob(inner))
@@ -768,7 +768,7 @@ pub(crate) mod tests {
         // make the length one extra
         buf1[3] += 1;
         let r: Result<Blob<BinString>, _> = read_ssh(&buf1, None);
-        assert!(matches!(r.unwrap_err(), Error::SSHProtoError));
+        assert!(matches!(r.unwrap_err(), Error::SSHProto { .. }));
 
         let mut buf1 = vec![88; 1000];
         let l = write_ssh(&mut buf1, &p1).unwrap();
@@ -777,7 +777,7 @@ pub(crate) mod tests {
         // make the length one short
         buf1[3] -= 1;
         let r: Result<Blob<BinString>, _> = read_ssh(&buf1, None);
-        assert!(matches!(r.unwrap_err(), Error::SSHProtoError));
+        assert!(matches!(r.unwrap_err(), Error::SSHProto { .. }));
     }
 
     #[test]
@@ -801,7 +801,7 @@ pub(crate) mod tests {
         // short
         buf1.truncate(l-1);
         let r = packet_from_bytes(&buf1, &ctx);
-        assert!(matches!(r.unwrap_err(), Error::RanOut));
+        assert!(matches!(r.unwrap_err(), Error::RanOut { .. }));
 
     }
 

diff --git a/src/traffic.rs b/src/traffic.rs
@@ -345,7 +345,7 @@ impl<'a> TrafOut<'a> {
         // after the length and padding bytes which get filled by encrypt()
         let wbuf = &mut self.buf[len..];
         if wbuf.len() < SSH_PAYLOAD_START {
-            return Err(Error::NoRoom)
+            return error::NoRoom.fail()
         }
         let plen = sshwire::write_ssh(&mut wbuf[SSH_PAYLOAD_START..], &p)?;
         trace!("Sending {p:?}");