openpgp: Include the path in the MessageValidator error

  - To ease debugging, include the path of the packet that caused the
    parse error.

3 files changed, 49 insertions, 30 deletions

diff --git a/openpgp/src/message/mod.rs b/openpgp/src/message/mod.rs
index a776f761..d2b76426 100644
--- a/openpgp/src/message/mod.rs
+++ b/openpgp/src/message/mod.rs
@@ -170,23 +170,28 @@ impl MessageValidator {
         self.tokens.push(token);
     }
 
-    /// Add the token `token` at depth `depth` to the token stream.
+    /// Add the token `token` at position `path` to the token stream.
     ///
-    /// Note: top-level packets are at depth 0, their immediate
-    /// children are a depth 1, etc.
+    /// Note: top-level packets are at `[ n ]`, their immediate
+    /// children are at `[ n, m ]`, etc.
     ///
-    /// The token *must* correspond to a packet; this function will
-    /// panic if `token` is Token::Pop.
-    pub fn push_token(&mut self, token: Token, depth: isize) {
+    /// This function pushes any required `Token::Pop` tokens based on
+    /// changes in the `path`.
+    ///
+    /// Note: the token *must* correspond to a packet; this function
+    /// will panic if `token` is `Token::Pop`.
+    pub fn push_token(&mut self, token: Token, path: &[usize]) {
         assert!(!self.finished);
         assert!(self.depth.is_some());
         assert!(token != Token::Pop);
+        assert!(path.len() > 0);
 
         if self.error.is_some() {
             return;
         }
 
         // We popped one or more containers.
+        let depth = path.len() as isize - 1;
         if self.depth.unwrap() > depth {
             for _ in 1..self.depth.unwrap() - depth + 1 {
                 self.tokens.push(Token::Pop);
@@ -197,11 +202,20 @@ impl MessageValidator {
         self.tokens.push(token);
     }
 
-    /// Add a packet of type `tag` at depth `depth` to the token
+    /// Add a packet of type `tag` at position `path` to the token
     /// stream.
     ///
-    /// Note: top-level packets are at depth 0.
-    pub fn push(&mut self, tag: Tag, depth: isize) {
+    /// Note: top-level packets are at `[ n ]`, their immediate
+    /// children are at `[ n, m ]`, etc.
+    ///
+    /// Unlike `push_token`, this function does not automatically
+    /// account for changes in the depth.  If you use this function
+    /// directly, you must push any required `Token::Pop` tokens.
+    pub fn push(&mut self, tag: Tag, path: &[usize]) {
+        if self.error.is_some() {
+            return;
+        }
+
         let token = match tag {
             Tag::Literal => Token::Literal,
             Tag::CompressedData => Token::CompressedData,
@@ -221,14 +235,15 @@ impl MessageValidator {
                 // Unknown token.
                 self.error = Some(MessageParserError::OpenPGP(
                     Error::MalformedMessage(
-                        format!("Invalid OpenPGP message: unexpected packet: {:?}",
-                                tag).into())));
+                        format!("Invalid OpenPGP message: \
+                                 {:?} packet (at {:?}) not expected",
+                                tag, path).into())));
                 self.tokens.clear();
                 return;
             }
         };
 
-        self.push_token(token, depth)
+        self.push_token(token, path)
     }
 
     /// Note that the entire message has been seen.
@@ -338,15 +353,16 @@ impl Message {
     ///   [Section 11.3 of RFC 4880]: https://tools.ietf.org/html/rfc4880#section-11.3
     pub fn from_packet_pile(pile: PacketPile) -> Result<Self> {
         let mut v = MessageValidator::new();
-        for (path, packet) in pile.descendants().paths() {
+        for (mut path, packet) in pile.descendants().paths() {
             match packet {
                 Packet::Unknown(ref u) =>
                     return Err(MessageParserError::OpenPGP(
                         Error::MalformedMessage(
                             format!("Invalid OpenPGP message: \
-                                     malformed {:?} packet: {}",
-                                    u.tag(), u.error()).into())).into()),
-                _ => v.push(packet.tag(), path.len() as isize - 1),
+                                     {:?} packet (at {:?}) not expected: {}",
+                                    u.tag(), path, u.error()).into()))
+                               .into()),
+                _ => v.push(packet.tag(), &path),
             }
 
             match packet {
@@ -355,9 +371,9 @@ impl Message {
                     // If a container's content is not unpacked, then
                     // we treat the content as an opaque message.
 
+                    path.push(0);
                     if packet.children.is_none() && packet.body.is_some() {
-                        v.push_token(Token::OpaqueContent,
-                                     path.len() as isize - 1 + 1);
+                        v.push_token(Token::OpaqueContent, &path);
                     }
                 }
                 _ => {}
@@ -678,7 +694,10 @@ mod tests {
         for v in test_vectors.into_iter() {
             let mut l = MessageValidator::new();
             for (token, depth) in v.s.iter() {
-                l.push(*token, *depth);
+                l.push(*token,
+                       &(0..1 + *depth)
+                           .map(|x| x as usize)
+                           .collect::<Vec<_>>()[..]);
                 if v.result {
                     assert_match!(MessageValidity::MessagePrefix = l.check());
                 }
diff --git a/openpgp/src/parse/packet_parser_builder.rs b/openpgp/src/parse/packet_parser_builder.rs
index a9818b25..514cb6f2 100644
--- a/openpgp/src/parse/packet_parser_builder.rs
+++ b/openpgp/src/parse/packet_parser_builder.rs
@@ -184,7 +184,7 @@ impl<'a> PacketParserBuilder<'a> {
         match PacketParser::parse(Box::new(self.bio), state, vec![ 0 ])? {
             ParserResult::Success(mut pp) => {
                 // We successfully parsed the first packet's header.
-                pp.state.message_validator.push(pp.packet.tag(), 0);
+                pp.state.message_validator.push(pp.packet.tag(), &[0]);
                 pp.state.keyring_validator.push(pp.packet.tag());
                 pp.state.tpk_validator.push(pp.packet.tag());
                 Ok(PacketParserResult::Some(pp))
diff --git a/openpgp/src/parse/parse.rs b/openpgp/src/parse/parse.rs
index a706e0c5..c023515d 100644
--- a/openpgp/src/parse/parse.rs
+++ b/openpgp/src/parse/parse.rs
@@ -3303,8 +3303,8 @@ impl <'a> PacketParser<'a> {
                     }
                 },
                 ParserResult::Success(mut pp) => {
-                    pp.state.message_validator.push(
-                        pp.packet.tag(), recursion_depth);
+                    let path = pp.path().to_vec();
+                    pp.state.message_validator.push(pp.packet.tag(), &path);
                     pp.state.keyring_validator.push(pp.packet.tag());
                     pp.state.tpk_validator.push(pp.packet.tag());
 
@@ -3365,16 +3365,15 @@ impl <'a> PacketParser<'a> {
                     let mut path = self.path;
                     path.push(0);
 
-                    let recursion_depth = path.len() as isize - 1;
-
-                    match PacketParser::parse(self.reader, self.state, path)? {
+                    match PacketParser::parse(self.reader, self.state,
+                                              path.clone())?
+                    {
                         ParserResult::Success(mut pp) => {
                             t!("Recursed into the {:?} packet, got a {:?}.",
                                self.packet.tag(), pp.packet.tag());
 
                             pp.state.message_validator.push(
-                                pp.packet.tag(),
-                                recursion_depth);
+                                pp.packet.tag(), &path);
                             pp.state.keyring_validator.push(pp.packet.tag());
                             pp.state.tpk_validator.push(pp.packet.tag());
 
@@ -3497,9 +3496,10 @@ impl <'a> PacketParser<'a> {
                 Tag::SEIP | Tag::AED | Tag::SED | Tag::CompressedData => {
                     // We didn't (fully) process a container's content.  Add
                     // this as opaque conent to the message validator.
+                    let mut path = self.path().to_vec();
+                    path.push(0);
                     self.state.message_validator.push_token(
-                        message::Token::OpaqueContent,
-                        recursion_depth + 1);
+                        message::Token::OpaqueContent, &path);
                 }
                 _ => {},
             }
@@ -4249,7 +4249,7 @@ mod test {
             }
             assert!(! saw_literal);
             if let PacketParserResult::EOF(eof) = ppr {
-                eprintln!("eof: {:?}", eof);
+                eprintln!("eof: {:?}; message: {:?}", eof, eof.is_message());
                 assert!(eof.is_message().is_ok());
             } else {
                 unreachable!();