fix(expression): range check numbers before bigdecimal arithmetic and comparison

crates/core/src/expression/evaluator.rs

@@ -14,7 +14,9 @@ use crate::error::DynamoDbError;
 use crate::types::AttributeValue;
 
 use super::ast::{CompareOp, Expr, PathElement};
-use super::resolver::{ExpressionMaps, attribute_type_code, resolve_path};
+use super::resolver::{
+    ExpressionMaps, ParsedNumber, attribute_type_code, number_value, resolve_path,
+};
 
 /// Evaluate a condition expression against an item.
 ///
@@ -127,65 +129,59 @@ fn resolve_to_value<'a>(
     }
 }
 
-/// Look up a pre-parsed `BigDecimal` for a placeholder expression.
+/// Look up a pre-parsed number for a placeholder expression.
 ///
-/// Returns `None` if the expression is not a placeholder or has no pre-parsed value.
-fn placeholder_numeric<'a>(
-    expr: &Expr,
-    maps: &'a ExpressionMaps,
-) -> Option<&'a bigdecimal::BigDecimal> {
+/// Returns `None` if the expression is not a placeholder or has no pre-parsed
+/// value.
+fn placeholder_numeric<'a>(expr: &Expr, maps: &'a ExpressionMaps) -> Option<ParsedNumber<'a>> {
     if let Expr::Placeholder(name) = expr {
         maps.get_parsed_numeric(name)
     } else {
         None
     }
 }
 
+/// One side of a numeric comparison as a decimal: the placeholder's
+/// pre-parsed value when there is one, else `n` read with [`number_value`].
+/// `None` when the number is not one the service can store.
+fn comparison_number<'a>(
+    n: &str,
+    parsed: Option<ParsedNumber<'a>>,
+) -> Option<Cow<'a, bigdecimal::BigDecimal>> {
+    match parsed {
+        Some(ParsedNumber::Number(d)) => Some(Cow::Borrowed(d)),
+        Some(ParsedNumber::Refused) => None,
+        None => number_value(n).map(Cow::Owned),
+    }
+}
+
 /// Compare two `AttributeValue`s using the given operator.
 ///
 /// `DynamoDB` comparison rules:
 /// - Same-type comparisons only (except N vs N which is numeric)
 /// - S: lexicographic UTF-8
-/// - N: numeric comparison via `BigDecimal` (pre-parsed values used when available)
+/// - N: numeric comparison via `BigDecimal` (pre-parsed values used when available).
+///   An `N` that is not a number the service can store compares as a value of
+///   another type: only `<>` is true, so `a <> b` agrees with `NOT (a = b)`.
 /// - B: lexicographic byte comparison
 /// - BOOL, NULL, L, M, SS, NS, BS: only equality
-// Allow single_match_else: the match arms bind an owned fallback value
-// and return a reference, which doesn't simplify to if-let.
-#[allow(clippy::single_match_else)]
 fn compare_values(
     left: &AttributeValue,
     right: &AttributeValue,
     op: CompareOp,
-    left_parsed: Option<&bigdecimal::BigDecimal>,
-    right_parsed: Option<&bigdecimal::BigDecimal>,
+    left_parsed: Option<ParsedNumber<'_>>,
+    right_parsed: Option<ParsedNumber<'_>>,
 ) -> bool {
     match (left, right) {
         (AttributeValue::S(l), AttributeValue::S(r)) => apply_op(l.cmp(r), op),
         (AttributeValue::N(l), AttributeValue::N(r)) => {
-            // Use pre-parsed values when available; fall back to parsing.
-            let l_owned;
-            let ld = match left_parsed {
-                Some(d) => d,
-                None => {
-                    let Ok(d) = l.parse::<bigdecimal::BigDecimal>() else {
-                        return false;
-                    };
-                    l_owned = d;
-                    &l_owned
-                }
+            let Some(ld) = comparison_number(l, left_parsed) else {
+                return matches!(op, CompareOp::Ne);
             };
-            let r_owned;
-            let rd = match right_parsed {
-                Some(d) => d,
-                None => {
-                    let Ok(d) = r.parse::<bigdecimal::BigDecimal>() else {
-                        return false;
-                    };
-                    r_owned = d;
-                    &r_owned
-                }
+            let Some(rd) = comparison_number(r, right_parsed) else {
+                return matches!(op, CompareOp::Ne);
             };
-            apply_op(ld.cmp(rd), op)
+            apply_op(ld.as_ref().cmp(rd.as_ref()), op)
         }
         (AttributeValue::B(l), AttributeValue::B(r)) => apply_op(l.cmp(r), op),
         // For non-orderable types, only equality is meaningful

crates/core/src/expression/evaluator_tests.rs

@@ -438,3 +438,116 @@ fn size_of_null_attribute_yields_no_value() {
     values.insert("zero".into(), AttributeValue::N("0".into()));
     assert!(!eval("size(nu) > :zero", &item, HashMap::new(), values).unwrap());
 }
+
+/// Evaluate `expr_str` on another thread and return its answer, or fail the
+/// test after 10 seconds.
+fn eval_within_10_s(
+    expr_str: &'static str,
+    item: BTreeMap<String, AttributeValue>,
+    values: HashMap<String, AttributeValue>,
+) -> Result<bool, DynamoDbError> {
+    let (tx, rx) = std::sync::mpsc::channel();
+    let worker = std::thread::spawn(move || {
+        let _ = tx.send(eval(expr_str, &item, HashMap::new(), values));
+    });
+    match rx.recv_timeout(std::time::Duration::from_secs(10)) {
+        Ok(answer) => answer,
+        Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => {
+            std::panic::resume_unwind(worker.join().expect_err("no answer and no panic"))
+        }
+        Err(std::sync::mpsc::RecvTimeoutError::Timeout) => panic!("`{expr_str}` ran for 10 s"),
+    }
+}
+
+/// A number with millions of digits, in a placeholder or in the item, is not
+/// parsed as written: bigdecimal's parse is quadratic in the digit count, and
+/// neither the placeholders nor a stored item are checked for numbers on
+/// every path before a comparison. Such a number is not one the service can
+/// store, and only `<>` is true for it.
+#[test]
+fn a_number_with_millions_of_digits_is_not_parsed() {
+    let long = format!("1{}", "0".repeat(2_000_000));
+    let item = BTreeMap::from([("n".to_owned(), AttributeValue::N("1".to_owned()))]);
+    let values = HashMap::from([("v".to_owned(), AttributeValue::N(long.clone()))]);
+    assert!(!eval_within_10_s("n < :v", item.clone(), values.clone()).unwrap());
+    assert!(eval_within_10_s("n <> :v", item, values).unwrap());
+
+    let item = BTreeMap::from([("n".to_owned(), AttributeValue::N(long))]);
+    let values = HashMap::from([("v".to_owned(), AttributeValue::N("1".to_owned()))]);
+    assert!(!eval_within_10_s("n > :v", item, values).unwrap());
+}
+
+/// A placeholder refused when the maps are built stays refused: a filter
+/// does not read its text again for each item it compares.
+#[test]
+fn a_refused_placeholder_is_read_once_per_request() {
+    let long = format!("1{}", "0".repeat(2_000_000));
+    let (tx, rx) = std::sync::mpsc::channel();
+    let worker = std::thread::spawn(move || {
+        let tokens = tokenize("n < :v").unwrap();
+        let expr = parse_condition(&tokens).unwrap();
+        let values = HashMap::from([("v".to_owned(), AttributeValue::N(long))]);
+        let maps = ExpressionMaps::new(HashMap::new(), values);
+        // The refusal is recorded, whatever the build's speed.
+        assert_eq!(maps.get_parsed_numeric("v"), Some(ParsedNumber::Refused));
+        let matched = (0..100)
+            .map(|i| BTreeMap::from([("n".to_owned(), AttributeValue::N(i.to_string()))]))
+            .filter(|item| evaluate_condition(&expr, item, &maps).unwrap())
+            .count();
+        let _ = tx.send(matched);
+    });
+    match rx.recv_timeout(std::time::Duration::from_secs(10)) {
+        Ok(matched) => assert_eq!(matched, 0),
+        Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => {
+            std::panic::resume_unwind(worker.join().expect_err("no answer and no panic"))
+        }
+        Err(std::sync::mpsc::RecvTimeoutError::Timeout) => {
+            panic!("100 items against one refused placeholder ran for 10 s")
+        }
+    }
+}
+
+/// A stored number the service cannot store, as an older release could
+/// store it, compares as a value of another type does: only `<>` is true, so
+/// `<>` and `NOT (... = ...)` agree.
+#[test]
+fn a_stored_number_out_of_range_compares_only_as_not_equal() {
+    for stored in ["1E126", "12345678901234567890123456789012345678901", "abc"] {
+        let item = BTreeMap::from([("n".to_owned(), AttributeValue::N(stored.to_owned()))]);
+        for (expr, expected) in [
+            ("n = :v", false),
+            ("n <> :v", true),
+            ("NOT (n = :v)", true),
+            ("n > :v", false),
+            ("n < :v", false),
+            ("n IN (:v)", false),
+            ("n BETWEEN :v AND :v", false),
+        ] {
+            let values = HashMap::from([("v".to_owned(), AttributeValue::N("0".to_owned()))]);
+            assert_eq!(
+                eval(expr, &item, HashMap::new(), values).unwrap(),
+                expected,
+                "{expr}, n = {stored}"
+            );
+        }
+    }
+}
+
+/// A number the service can store still compares numerically, in any
+/// spelling.
+#[test]
+fn numbers_in_any_spelling_compare_by_value() {
+    let item = BTreeMap::from([("n".to_owned(), AttributeValue::N("150".to_owned()))]);
+    for (expr, v) in [
+        ("n = :v", "1.5E2"),
+        ("n = :v", "+150.00"),
+        ("n < :v", "1E125"),
+        ("n > :v", "1E-130"),
+    ] {
+        let values = HashMap::from([("v".to_owned(), AttributeValue::N(v.to_owned()))]);
+        assert!(
+            eval(expr, &item, HashMap::new(), values).unwrap(),
+            "{expr} with :v = {v}"
+        );
+    }
+}

crates/core/src/expression/resolver.rs

@@ -26,9 +26,19 @@ use super::ast::{CompareOp, Expr, PathElement};
 pub struct ExpressionMaps {
     pub names: HashMap<String, String>,
     pub values: HashMap<String, AttributeValue>,
-    /// Pre-parsed `BigDecimal` values for numeric placeholders.
+    /// Pre-parsed `BigDecimal` values for numeric placeholders, `None` for a
+    /// placeholder whose `N` is not a number the service can store.
     /// Populated eagerly by `new()` and refreshed by `pre_parse_numerics()`.
-    parsed_numerics: HashMap<String, bigdecimal::BigDecimal>,
+    parsed_numerics: HashMap<String, Option<bigdecimal::BigDecimal>>,
+}
+
+/// A numeric placeholder as [`ExpressionMaps`] pre-parsed it.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum ParsedNumber<'a> {
+    /// The placeholder's `N` is a number the service can store.
+    Number(&'a bigdecimal::BigDecimal),
+    /// The placeholder's `N` is not a number the service can store.
+    Refused,
 }
 
 impl ExpressionMaps {
@@ -88,28 +98,47 @@ impl ExpressionMaps {
     /// Pre-parse all numeric placeholder values into `BigDecimal`.
     ///
     /// Call once per request before evaluating filter expressions against
-    /// multiple items. Invalid numeric strings are silently skipped — they
-    /// will produce `false` comparisons at evaluation time, matching the
-    /// existing behavior.
+    /// multiple items. A value that is not a number the service can store is
+    /// recorded as refused (see [`number_value`]), so a filter does not read
+    /// it again for each item.
     pub fn pre_parse_numerics(&mut self) {
         for (key, value) in &self.values {
-            if let AttributeValue::N(n) = value
-                && let Ok(d) = n.parse::<bigdecimal::BigDecimal>()
-            {
-                self.parsed_numerics.insert(key.clone(), d);
+            if let AttributeValue::N(n) = value {
+                self.parsed_numerics.insert(key.clone(), number_value(n));
             }
         }
     }
 
-    /// Look up a pre-parsed `BigDecimal` for a numeric placeholder.
-    ///
-    /// Returns `None` if the placeholder was not numeric or failed to parse.
+    /// Look up a numeric placeholder as it was pre-parsed, or `None` when the
+    /// placeholder is not an `N` in these maps.
     #[must_use]
-    pub fn get_parsed_numeric(&self, placeholder: &str) -> Option<&bigdecimal::BigDecimal> {
-        self.parsed_numerics.get(placeholder)
+    pub(crate) fn get_parsed_numeric(&self, placeholder: &str) -> Option<ParsedNumber<'_>> {
+        self.parsed_numerics.get(placeholder).map(|parsed| {
+            parsed
+                .as_ref()
+                .map_or(ParsedNumber::Refused, ParsedNumber::Number)
+        })
     }
 }
 
+/// A number in an expression as a decimal, or `None` when `n` is not a number
+/// the service can store.
+///
+/// The decimal is parsed from the number check's normalized text, which is
+/// at most 170 characters for every number the service accepts. Parsing the
+/// text as written takes time quadratic in its length: a million digits took
+/// about a second (bigdecimal 0.4.10, release build), and a request body can
+/// carry 16 MiB. `UpdateItem` and `TransactWriteItems` build their
+/// `ExpressionMaps` before they check numbers, `Query` and `Scan` do not check
+/// them (#544, #562), and a stored item can hold a number an older release
+/// stored without the check (#618).
+pub(crate) fn number_value(n: &str) -> Option<bigdecimal::BigDecimal> {
+    crate::validation::number::validate_and_normalize_number(n)
+        .ok()?
+        .parse()
+        .ok()
+}
+
 /// Resolve a `#name` reference or return the bare name.
 ///
 /// Shared by the condition evaluator and update evaluator for resolving