compiler: forbid fragment probabilities of 0

We can cause panics in the compiler comparing OrdF64s which are NaN,
which we can produce by parsing ors where every child has weight 0.

Simply make it a parse error to have any zero-probability branches.
In theory somebody might want an or where some fragments have positive
probability and others are 0, indicating "maximally pessimize these".
But it has never occurred to me to do this, and it's easy to simulate
this by using 100s and 1s, say.

Author: apoelstra

.github/workflows/cron-daily-fuzz.yml

@@ -25,6 +25,7 @@ parse_descriptor,
 parse_descriptor_definite,
 parse_descriptor_priv,
 parse_descriptor_secret,
+regression_compiler,
 regression_descriptor_parse,
 regression_taptree,
 roundtrip_concrete,

fuzz/Cargo.toml

@@ -14,7 +14,7 @@ honggfuzz = { version = "0.5.56", default-features = false }
 # We shouldn't need an explicit version on the next line, but Andrew's tools
 # choke on it otherwise. See https://github.com/nix-community/crate2nix/issues/373
 miniscript = { path = "..", features = [ "compiler" ], version = "13.0" }
-old_miniscript = { package = "miniscript", version = "12.3" }
+old_miniscript = { package = "miniscript", features = [ "compiler" ], version = "12.3" }
 
 regex = "1.0"
 
@@ -46,6 +46,10 @@ path = "fuzz_targets/parse_descriptor_priv.rs"
 name = "parse_descriptor_secret"
 path = "fuzz_targets/parse_descriptor_secret.rs"
 
+[[bin]]
+name = "regression_compiler"
+path = "fuzz_targets/regression_compiler.rs"
+
 [[bin]]
 name = "regression_descriptor_parse"
 path = "fuzz_targets/regression_descriptor_parse.rs"

fuzz/fuzz_targets/regression_compiler.rs

@@ -0,0 +1,66 @@
+use descriptor_fuzz::FuzzPk;
+use honggfuzz::fuzz;
+use miniscript::{policy, ParseError, ParseNumError};
+use old_miniscript::policy as old_policy;
+
+type Policy = policy::Concrete<FuzzPk>;
+type OldPolicy = old_policy::Concrete<FuzzPk>;
+
+fn do_test(data: &[u8]) {
+    let data_str = String::from_utf8_lossy(data);
+    match (data_str.parse::<Policy>(), data_str.parse::<OldPolicy>()) {
+        (Err(_), Err(_)) => {}
+        (Ok(x), Err(e)) => panic!("new logic parses {} as {:?}, old fails with {}", data_str, x, e),
+        // These is anew parse error
+        (
+            Err(miniscript::Error::Parse(ParseError::Num(ParseNumError::IllegalZero { .. }))),
+            Ok(_),
+        ) => {}
+        (Err(e), Ok(x)) => {
+            panic!("old logic parses {} as {:?}, new fails with {:?}", data_str, x, e)
+        }
+        (Ok(new), Ok(old)) => {
+            assert_eq!(
+                old.to_string(),
+                new.to_string(),
+                "input {} (left is old, right is new)",
+                data_str
+            );
+
+            let comp = new.compile::<miniscript::Legacy>();
+            let old_comp = old.compile::<old_miniscript::Legacy>();
+
+            match (comp, old_comp) {
+                (Err(_), Err(_)) => {}
+                (Ok(x), Err(e)) => {
+                    panic!("new logic compiles {} as {:?}, old fails with {}", data_str, x, e)
+                }
+                (Err(e), Ok(x)) => {
+                    panic!("old logic compiles {} as {:?}, new fails with {}", data_str, x, e)
+                }
+                (Ok(new), Ok(old)) => {
+                    assert_eq!(
+                        old.to_string(),
+                        new.to_string(),
+                        "input {} (left is old, right is new)",
+                        data_str
+                    );
+                }
+            }
+        }
+    }
+}
+
+fn main() {
+    loop {
+        fuzz!(|data| {
+            do_test(data);
+        });
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    fn duplicate_crash() { crate::do_test(b"or(0@pk(09),0@TRIVIAL)") }
+}

fuzz/generate-files.sh

@@ -26,7 +26,7 @@ honggfuzz = { version = "0.5.56", default-features = false }
 # We shouldn't need an explicit version on the next line, but Andrew's tools
 # choke on it otherwise. See https://github.com/nix-community/crate2nix/issues/373
 miniscript = { path = "..", features = [ "compiler" ], version = "13.0" }
-old_miniscript = { package = "miniscript", version = "12.3" }
+old_miniscript = { package = "miniscript", features = [ "compiler" ], version = "12.3" }
 
 regex = "1.0"
 EOF

src/expression/error.rs

@@ -197,6 +197,13 @@ pub enum ParseNumError {
     StdParse(num::ParseIntError),
     /// Number had a leading zero, + or -.
     InvalidLeadingDigit(char),
+    /// Number was 0 in a context where zero is not allowed.
+    ///
+    /// Be aware that locktimes have their own error types and do not use this variant.
+    IllegalZero {
+        /// A description of the location where 0 was not allowed.
+        context: &'static str,
+    },
 }
 
 impl fmt::Display for ParseNumError {
@@ -206,6 +213,9 @@ impl fmt::Display for ParseNumError {
             ParseNumError::InvalidLeadingDigit(ch) => {
                 write!(f, "numbers must start with 1-9, not {}", ch)
             }
+            ParseNumError::IllegalZero { context } => {
+                write!(f, "{} may not be 0", context)
+            }
         }
     }
 }
@@ -216,6 +226,7 @@ impl std::error::Error for ParseNumError {
         match self {
             ParseNumError::StdParse(ref e) => Some(e),
             ParseNumError::InvalidLeadingDigit(..) => None,
+            ParseNumError::IllegalZero { .. } => None,
         }
     }
 }

src/expression/mod.rs

@@ -678,11 +678,10 @@ impl<'a> Tree<'a> {
     }
 }
 
-/// Parse a string as a u32, for timelocks or thresholds
-pub fn parse_num(s: &str) -> Result<u32, ParseNumError> {
+/// Parse a string as a u32, forbidding zero.
+pub fn parse_num_nonzero(s: &str, context: &'static str) -> Result<u32, ParseNumError> {
     if s == "0" {
-        // Special-case 0 since it is the only number which may start with a leading zero.
-        return Ok(0);
+        return Err(ParseNumError::IllegalZero { context });
     }
     if let Some(ch) = s.chars().next() {
         if !('1'..='9').contains(&ch) {
@@ -692,6 +691,15 @@ pub fn parse_num(s: &str) -> Result<u32, ParseNumError> {
     u32::from_str(s).map_err(ParseNumError::StdParse)
 }
 
+/// Parse a string as a u32, for timelocks or thresholds
+pub fn parse_num(s: &str) -> Result<u32, ParseNumError> {
+    if s == "0" {
+        // Special-case 0 since it is the only number which may start with a leading zero.
+        return Ok(0);
+    }
+    parse_num_nonzero(s, "")
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -772,6 +780,7 @@ mod tests {
     #[test]
     fn test_parse_num() {
         assert!(parse_num("0").is_ok());
+        assert!(parse_num_nonzero("0", "").is_err());
         assert!(parse_num("00").is_err());
         assert!(parse_num("0000").is_err());
         assert!(parse_num("06").is_err());

src/policy/concrete.rs

@@ -883,7 +883,7 @@ impl<Pk: FromStrKey> expression::FromTree for Policy<Pk> {
 
             let frag_prob = match frag_prob {
                 None => 1,
-                Some(s) => expression::parse_num(s)
+                Some(s) => expression::parse_num_nonzero(s, "fragment probability")
                     .map_err(From::from)
                     .map_err(Error::Parse)? as usize,
             };
@@ -1231,4 +1231,11 @@ mod tests {
         // This implicitly tests the check_timelocks API (has height and time locks).
         let _ = Policy::<String>::from_str("and(after(10),after(500000000))").unwrap();
     }
+
+    #[test]
+    fn parse_zero_disjunction() {
+        "or(0@pk(09),0@TRIVIAL)"
+            .parse::<Policy<String>>()
+            .unwrap_err();
+    }
 }