expression: add explicit well-formedness check to parser

When we convert the parser to be non-recursive, it will be simpler if we
first do a pass to confirm that the expression is well-formed (all
parens match and are balanced, commas in the right place, etc.). After
that we can assume the string is well-formed and write an algorithm that
basically just translates it into a data structure.

Conveniently, because this check is independent of the parsing
algorithm, we can add it to the existing code as a pre-check.

Since all errors during parsing should occur during the pre-check, we
can then turn the existing stringly-typed errors into unreachable!()s.
By fuzzing the resulting code we can guarantee that our new pre-check
is at least as strict as the existing parser.

In a later PR we will generalize the pre-check a bit so that it treats
*both* () and {} as parentheses, and markes tree nodes based on which
parens are used. But until we change the Taproot parser (which is is an
ad-hoc mess of combination manual string parsing and tree parsing, and
expects tapleaves to show up as the "name"s of childless nodes in a tree
of {} combinators) we can't effectively do this.

There is a new unit test `parse_tree_taproot` that will track this
progress.

Author: apoelstra

src/expression/error.rs

@@ -7,6 +7,105 @@ use core::fmt;
 use crate::prelude::*;
 use crate::ThresholdError;
 
+/// An error parsing an expression tree.
+#[derive(Debug, PartialEq, Eq)]
+pub enum ParseTreeError {
+    /// Expression tree had depth exceeding our hard cap.
+    MaxRecursionDepthExceeded {
+        /// The depth of the tree that was attempted to be parsed.
+        actual: usize,
+        /// The maximum depth.
+        maximum: u32,
+    },
+    /// Character occurred which was not part of the valid descriptor character set.
+    InvalidCharacter {
+        /// The character in question.
+        ch: char,
+        /// Its byte-index into the string.
+        pos: usize,
+    },
+    /// After a close-paren, the only valid next characters are close-parens and commas. Got
+    /// something else.
+    ExpectedParenOrComma {
+        /// What we got instead.
+        ch: char,
+        /// Its byte-index into the string.
+        pos: usize,
+    },
+    /// An open-parenthesis had no corresponding close-parenthesis.
+    UnmatchedOpenParen {
+        /// The character in question ('(' or '{')
+        ch: char,
+        /// Its byte-index into the string.
+        pos: usize,
+    },
+    /// A close-parenthesis had no corresponding open-parenthesis.
+    UnmatchedCloseParen {
+        /// The character in question (')' or '}')
+        ch: char,
+        /// Its byte-index into the string.
+        pos: usize,
+    },
+    /// A `(` was matched with a `}` or vice-versa.
+    MismatchedParens {
+        /// The opening parenthesis ('(' or '{')
+        open_ch: char,
+        /// The position of the opening parethesis.
+        open_pos: usize,
+        /// The closing parenthesis (')' or '}')
+        close_ch: char,
+        /// The position of the closing parethesis.
+        close_pos: usize,
+    },
+    /// Data occurred after the final ).
+    TrailingCharacter {
+        /// The first trailing character.
+        ch: char,
+        /// Its byte-index into the string.
+        pos: usize,
+    },
+}
+
+impl fmt::Display for ParseTreeError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            ParseTreeError::MaxRecursionDepthExceeded { actual, maximum } => {
+                write!(f, "maximum recursion depth exceeded (max {}, got {})", maximum, actual)
+            }
+            ParseTreeError::InvalidCharacter { ch, pos } => {
+                write!(f, "character `{}` (position {}) not allowed in descriptor", ch, pos)
+            }
+            ParseTreeError::ExpectedParenOrComma { ch, pos } => {
+                write!(
+                    f,
+                    "invalid character `{}` (position {}); expected comma or close-paren",
+                    ch, pos
+                )
+            }
+            ParseTreeError::UnmatchedOpenParen { ch, pos } => {
+                write!(f, "`{}` (position {}) not closed", ch, pos)
+            }
+            ParseTreeError::UnmatchedCloseParen { ch, pos } => {
+                write!(f, "`{}` (position {}) not opened", ch, pos)
+            }
+            ParseTreeError::MismatchedParens { open_ch, open_pos, close_ch, close_pos } => {
+                write!(
+                    f,
+                    "`{}` (position {}) closed by `{}` (position {})",
+                    open_ch, open_pos, close_ch, close_pos
+                )
+            }
+            ParseTreeError::TrailingCharacter { ch, pos } => {
+                write!(f, "trailing data `{}...` (position {})", ch, pos)
+            }
+        }
+    }
+}
+#[cfg(feature = "std")]
+impl std::error::Error for ParseTreeError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { None }
+}
+
 /// Error parsing a threshold expression.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum ParseThresholdError {

src/expression/mod.rs

@@ -8,7 +8,7 @@ mod error;
 use core::fmt;
 use core::str::FromStr;
 
-pub use self::error::ParseThresholdError;
+pub use self::error::{ParseThresholdError, ParseTreeError};
 use crate::prelude::*;
 use crate::{errstr, Error, Threshold, MAX_RECURSION_DEPTH};
 
@@ -145,13 +145,107 @@ impl<'a> Tree<'a> {
         Self::from_slice_delim(sl, 0u32, '(')
     }
 
+    fn parse_pre_check(s: &str, open: u8, close: u8) -> Result<(), ParseTreeError> {
+        // First, scan through string to make sure it is well-formed.
+        let mut max_depth = 0;
+        // Do ASCII check first; after this we can use .bytes().enumerate() rather
+        // than .char_indices(), which is *significantly* faster.
+        for (pos, ch) in s.char_indices() {
+            if !(32..128).contains(&u32::from(ch)) {
+                return Err(ParseTreeError::InvalidCharacter { ch, pos });
+            }
+        }
+
+        let mut open_paren_stack = Vec::with_capacity(128);
+
+        for (pos, ch) in s.bytes().enumerate() {
+            if ch == open {
+                open_paren_stack.push((ch, pos));
+                if max_depth < open_paren_stack.len() {
+                    max_depth = open_paren_stack.len();
+                }
+            } else if ch == close {
+                if let Some((open_ch, open_pos)) = open_paren_stack.pop() {
+                    if (open_ch == b'(' && ch == b'}') || (open_ch == b'{' && ch == b')') {
+                        return Err(ParseTreeError::MismatchedParens {
+                            open_ch: open_ch.into(),
+                            open_pos,
+                            close_ch: ch.into(),
+                            close_pos: pos,
+                        });
+                    }
+
+                    if let Some(&(paren_ch, paren_pos)) = open_paren_stack.last() {
+                        // not last paren; this should not be the end of the string,
+                        // and the next character should be a , ) or }.
+                        if pos == s.len() - 1 {
+                            return Err(ParseTreeError::UnmatchedOpenParen {
+                                ch: paren_ch.into(),
+                                pos: paren_pos,
+                            });
+                        } else {
+                            let next_byte = s.as_bytes()[pos + 1];
+                            if next_byte != b')' && next_byte != b'}' && next_byte != b',' {
+                                return Err(ParseTreeError::ExpectedParenOrComma {
+                                    ch: next_byte.into(),
+                                    pos: pos + 1,
+                                });
+                                //
+                            }
+                        }
+                    } else {
+                        // last paren; this SHOULD be the end of the string
+                        if pos < s.len() - 1 {
+                            return Err(ParseTreeError::TrailingCharacter {
+                                ch: s.as_bytes()[pos + 1].into(),
+                                pos: pos + 1,
+                            });
+                        }
+                    }
+                } else {
+                    // In practice, this is only hit if there are no open parens at all.
+                    // If there are open parens, like in "())", then on the first ), we
+                    // would have returned TrailingCharacter in the previous clause.
+                    //
+                    // From a user point of view, UnmatchedCloseParen would probably be
+                    // a clearer error to get, but it complicates the parser to do this,
+                    // and "TralingCharacter" is technically correct, so we leave it for
+                    // now.
+                    return Err(ParseTreeError::UnmatchedCloseParen { ch: ch.into(), pos });
+                }
+            } else if ch == b',' && open_paren_stack.is_empty() {
+                // We consider commas outside of the tree to be "trailing characters"
+                return Err(ParseTreeError::TrailingCharacter { ch: ch.into(), pos });
+            }
+        }
+        // Catch "early end of string"
+        if let Some((ch, pos)) = open_paren_stack.pop() {
+            return Err(ParseTreeError::UnmatchedOpenParen { ch: ch.into(), pos });
+        }
+
+        // FIXME should be able to remove this once we eliminate all recursion
+        // in the library.
+        if u32::try_from(max_depth).unwrap_or(u32::MAX) > MAX_RECURSION_DEPTH {
+            return Err(ParseTreeError::MaxRecursionDepthExceeded {
+                actual: max_depth,
+                maximum: MAX_RECURSION_DEPTH,
+            });
+        }
+
+        Ok(())
+    }
+
     pub(crate) fn from_slice_delim(
         mut sl: &'a str,
         depth: u32,
         delim: char,
     ) -> Result<(Tree<'a>, &'a str), Error> {
-        if depth >= MAX_RECURSION_DEPTH {
-            return Err(Error::MaxRecursiveDepthExceeded);
+        if depth == 0 {
+            if delim == '{' {
+                Self::parse_pre_check(sl, b'{', b'}').map_err(Error::ParseTree)?;
+            } else {
+                Self::parse_pre_check(sl, b'(', b')').map_err(Error::ParseTree)?;
+            }
         }
 
         match next_expr(sl, delim) {
@@ -171,7 +265,7 @@ impl<'a> Tree<'a> {
                     ret.args.push(arg);
 
                     if new_sl.is_empty() {
-                        return Err(Error::ExpectedChar(closing_delim(delim)));
+                        unreachable!()
                     }
 
                     sl = &new_sl[1..];
@@ -181,7 +275,7 @@ impl<'a> Tree<'a> {
                             if last_byte == closing_delim(delim) as u8 {
                                 break;
                             } else {
-                                return Err(Error::ExpectedChar(closing_delim(delim)));
+                                unreachable!()
                             }
                         }
                     }
@@ -200,7 +294,7 @@ impl<'a> Tree<'a> {
         if rem.is_empty() {
             Ok(top)
         } else {
-            Err(errstr(rem))
+            unreachable!()
         }
     }
 
@@ -337,36 +431,88 @@ mod tests {
     fn parse_tree_basic() {
         assert_eq!(Tree::from_str("thresh").unwrap(), leaf("thresh"));
 
-        assert!(matches!(Tree::from_str("thresh,"), Err(Error::Unexpected(s)) if s == ","));
+        assert!(matches!(
+            Tree::from_str("thresh,").unwrap_err(),
+            Error::ParseTree(ParseTreeError::TrailingCharacter { ch: ',', pos: 6 }),
+        ));
 
         assert!(matches!(
-            Tree::from_str("thresh,thresh"),
-            Err(Error::Unexpected(s)) if s == ",thresh",
+            Tree::from_str("thresh,thresh").unwrap_err(),
+            Error::ParseTree(ParseTreeError::TrailingCharacter { ch: ',', pos: 6 }),
         ));
 
         assert!(matches!(
-            Tree::from_str("thresh()thresh()"),
-            Err(Error::Unexpected(s)) if s == "thresh()",
+            Tree::from_str("thresh()thresh()").unwrap_err(),
+            Error::ParseTree(ParseTreeError::TrailingCharacter { ch: 't', pos: 8 }),
         ));
 
         assert_eq!(Tree::from_str("thresh()").unwrap(), paren_node("thresh", vec![leaf("")]));
 
-        // FIXME even for our current crappy error handling, this one is pretty bad
-        assert!(matches!(Tree::from_str("thresh(a()b)"), Err(Error::ExpectedChar(')'))));
+        assert!(matches!(
+            Tree::from_str("thresh(a()b)"),
+            Err(Error::ParseTree(ParseTreeError::ExpectedParenOrComma { ch: 'b', pos: 10 })),
+        ));
 
-        assert!(matches!(Tree::from_str("thresh()xyz"), Err(Error::Unexpected(s)) if s == "xyz"));
+        assert!(matches!(
+            Tree::from_str("thresh()xyz"),
+            Err(Error::ParseTree(ParseTreeError::TrailingCharacter { ch: 'x', pos: 8 })),
+        ));
     }
 
     #[test]
     fn parse_tree_parens() {
-        assert!(matches!(Tree::from_str("a("), Err(Error::ExpectedChar(')'))));
+        assert!(matches!(
+            Tree::from_str("a(").unwrap_err(),
+            Error::ParseTree(ParseTreeError::UnmatchedOpenParen { ch: '(', pos: 1 }),
+        ));
+
+        assert!(matches!(
+            Tree::from_str(")").unwrap_err(),
+            Error::ParseTree(ParseTreeError::UnmatchedCloseParen { ch: ')', pos: 0 }),
+        ));
+
+        assert!(matches!(
+            Tree::from_str("x(y))").unwrap_err(),
+            Error::ParseTree(ParseTreeError::TrailingCharacter { ch: ')', pos: 4 }),
+        ));
+
+        /* Will be enabled in a later PR which unifies TR and non-TR parsing.
+        assert!(matches!(
+            Tree::from_str("a{").unwrap_err(),
+            Error::ParseTree(ParseTreeError::UnmatchedOpenParen { ch: '{', pos: 1 }),
+        ));
+
+        assert!(matches!(
+            Tree::from_str("}").unwrap_err(),
+            Error::ParseTree(ParseTreeError::UnmatchedCloseParen { ch: '}', pos: 0 }),
+        ));
+        */
 
-        assert!(matches!(Tree::from_str(")"), Err(Error::Unexpected(s)) if s == ")"));
+        assert!(matches!(
+            Tree::from_str("x(y)}").unwrap_err(),
+            Error::ParseTree(ParseTreeError::TrailingCharacter { ch: '}', pos: 4 }),
+        ));
 
-        assert!(matches!(Tree::from_str("x(y))"), Err(Error::Unexpected(s)) if s == ")"));
+        /* Will be enabled in a later PR which unifies TR and non-TR parsing.
+        assert!(matches!(
+            Tree::from_str("x{y)").unwrap_err(),
+            Error::ParseTree(ParseTreeError::MismatchedParens {
+                open_ch: '{',
+                open_pos: 1,
+                close_ch: ')',
+                close_pos: 3,
+            }),
+        ));
+        */
+    }
 
-        // In next commit will add tests related to {}s; currently we ignore
-        // these except in Taproot mode.
+    #[test]
+    fn parse_tree_taproot() {
+        // This test will change in a later PR which unifies TR and non-TR parsing.
+        assert!(matches!(
+            Tree::from_str("a{b(c),d}").unwrap_err(),
+            Error::ParseTree(ParseTreeError::TrailingCharacter { ch: ',', pos: 6 }),
+        ));
     }
 
     #[test]

src/lib.rs

@@ -137,7 +137,7 @@ use bitcoin::{script, Opcode};
 
 pub use crate::blanket_traits::FromStrKey;
 pub use crate::descriptor::{DefiniteDescriptorKey, Descriptor, DescriptorPublicKey};
-pub use crate::expression::ParseThresholdError;
+pub use crate::expression::{ParseThresholdError, ParseTreeError};
 pub use crate::interpreter::Interpreter;
 pub use crate::miniscript::analyzable::{AnalysisError, ExtParams};
 pub use crate::miniscript::context::{BareCtx, Legacy, ScriptContext, Segwitv0, SigType, Tap};
@@ -494,6 +494,8 @@ pub enum Error {
     ParseThreshold(ParseThresholdError),
     /// Checksum error parsing a descriptor.
     Checksum(descriptor::checksum::Error),
+    /// Invalid expression tree.
+    ParseTree(ParseTreeError),
 }
 
 // https://github.com/sipa/miniscript/pull/5 for discussion on this number
@@ -556,6 +558,7 @@ impl fmt::Display for Error {
             Error::Threshold(ref e) => e.fmt(f),
             Error::ParseThreshold(ref e) => e.fmt(f),
             Error::Checksum(ref e) => e.fmt(f),
+            Error::ParseTree(ref e) => e.fmt(f),
         }
     }
 }
@@ -607,6 +610,7 @@ impl error::Error for Error {
             Threshold(e) => Some(e),
             ParseThreshold(e) => Some(e),
             Checksum(e) => Some(e),
+            ParseTree(e) => Some(e),
         }
     }
 }