Merge rust-bitcoin/rust-bitcoin#6886: units: preserve original err summing `NumOpResult`
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6886: units: preserve original err summing `NumOpResult`
d0d2f2ee23959e0a970a90ed6ec3f8d9632af8b1 units: test that Sum keeps the error it was given (satsfy (Renato Britto))
ebbeb23687e5170cf847cd53f3d208acaffdcfc4 units: preserve original err summing NumOpResult (satsfy (Renato Britto))
Pull request description:
Closes https://github.com/project-loupe/audit-rust-bitcoin/issues/201
The Sum impls replaced any error in the iterator with an addition overflow, so, say, a division by zero came out labelled MathOp::Add.
Also need to put `*amount` because a binding must have the same type in all alternatives, amount was a ref not owned while acc was, the compiler complained.
ACKs for top commit:
apoelstra:
ACK d0d2f2ee23959e0a970a90ed6ec3f8d9632af8b1; successfully ran local tests
tcharding:
ACK d0d2f2ee23959e0a970a90ed6ec3f8d9632af8b1
Kixunil:
ACK d0d2f2ee23959e0a970a90ed6ec3f8d9632af8b1
Tree-SHA512: dc4189b3279f468e42b97a1c9fe694873121edda3623192d5684076ddda33df967f18af603b21ed0e90cdbe836e2f946017c42eff1a7592a81233730a465791b
What changed, and why it matters
This patch fixes a bug in how rust-bitcoin adds up a list of numeric operation results. Previously, if any item in the list already carried an error (for example, dividing by zero), the summing code would overwrite that error and falsely report it as an addition overflow. The fix preserves the original error so callers see the true cause of the failure. It is a correctness fix rather than a direct exploit, but misreported errors can mislead downstream code or users about what went wrong.
Review any downstream code that inspects `NumOpError` after summing `NumOpResult` iterators; prior behavior may have masked non-overflow errors. The patch should be backported if the affected release series is supported, and the regression test should be kept in the test suite.
Security signals we found
Error-type confusion / misattribution in arithmetic result aggregation
Loss of original failure context across iterator fold
Potential for downstream logic to act on wrong `MathOp`/`MathErrorKind`
Evidence from the diff
The core::iter::Sum implementations for NumOpResult<Amount> and NumOpResult<SignedAmount> used a catch-all arm that replaced any encountered error with a freshly constructed NumOpError labelled MathOp::Add / Overflow. The patch changes the match arms to propagate an existing Self::Error(e) unchanged, only performing the actual addition when both operands are Valid. It also dereferences amount in the reference-taking impls so the match scrutinee has the same type in all arms. A regression test verifies that a division-by-zero error survives summation and still reports is_division().
Changed components
units/src/amount/ops.rsNumOpResult<Amount>NumOpResult<SignedAmount>core::iter::Sum implementations for amount result typesInspect captured patch +23 / −19
### units/src/amount/ops.rs
@@ -12,7 +12,7 @@ use crate::internal_macros::{
impl_add_assign_for_results, impl_div_assign, impl_mul_assign, impl_rem_assign,
impl_sub_assign_for_results,
};
-use crate::result::{MathErrorKind, MathOp, NumOpError, NumOpResult, OptionExt};
+use crate::result::{MathErrorKind, MathOp, NumOpResult, OptionExt};
impl From<Amount> for NumOpResult<Amount> {
#[inline]
@@ -295,10 +295,7 @@ impl<T: Into<Self>> core::iter::Sum<T> for NumOpResult<Amount> {
{
iter.fold(Self::Valid(Amount::ZERO), |acc, amount| match (acc, amount.into()) {
(Self::Valid(lhs), Self::Valid(rhs)) => lhs + rhs,
- (_, _) => Self::Error(NumOpError::while_doing(MathErrorKind::Overflow {
- op: MathOp::Add,
- is_negative: false,
- })),
+ (Self::Error(e), _) | (_, Self::Error(e)) => Self::Error(e),
})
}
}
@@ -307,12 +304,9 @@ impl<'a> core::iter::Sum<&'a Self> for NumOpResult<Amount> {
where
I: Iterator<Item = &'a Self>,
{
- iter.fold(Self::Valid(Amount::ZERO), |acc, amount| match (acc, amount) {
+ iter.fold(Self::Valid(Amount::ZERO), |acc, amount| match (acc, *amount) {
(Self::Valid(lhs), Self::Valid(rhs)) => lhs + rhs,
- (_, _) => Self::Error(NumOpError::while_doing(MathErrorKind::Overflow {
- op: MathOp::Add,
- is_negative: false,
- })),
+ (Self::Error(e), _) | (_, Self::Error(e)) => Self::Error(e),
})
}
}
@@ -324,10 +318,7 @@ impl<T: Into<Self>> core::iter::Sum<T> for NumOpResult<SignedAmount> {
{
iter.fold(Self::Valid(SignedAmount::ZERO), |acc, amount| match (acc, amount.into()) {
(Self::Valid(lhs), Self::Valid(rhs)) => lhs + rhs,
- (_, _) => Self::Error(NumOpError::while_doing(MathErrorKind::Overflow {
- op: MathOp::Add,
- is_negative: false,
- })),
+ (Self::Error(e), _) | (_, Self::Error(e)) => Self::Error(e),
})
}
}
@@ -336,18 +327,16 @@ impl<'a> core::iter::Sum<&'a Self> for NumOpResult<SignedAmount> {
where
I: Iterator<Item = &'a Self>,
{
- iter.fold(Self::Valid(SignedAmount::ZERO), |acc, amount| match (acc, amount) {
+ iter.fold(Self::Valid(SignedAmount::ZERO), |acc, amount| match (acc, *amount) {
(Self::Valid(lhs), Self::Valid(rhs)) => lhs + rhs,
- (_, _) => Self::Error(NumOpError::while_doing(MathErrorKind::Overflow {
- op: MathOp::Add,
- is_negative: false,
- })),
+ (Self::Error(e), _) | (_, Self::Error(e)) => Self::Error(e),
})
}
}
#[cfg(test)]
mod tests {
use super::*;
+ use crate::result::NumOpError;
#[test]
fn sum_amounts() {
@@ -358,6 +347,21 @@ mod tests {
assert_eq!(sum, NumOpResult::Valid(Amount::from_sat_u32(600)));
}
+ #[test]
+ fn sum_preserves_division_by_zero_error() {
+ let err = [Amount::ONE_SAT / 0_u64];
+ let sum: NumOpResult<Amount> = err.into_iter().sum();
+ assert!(sum.unwrap_err().operation().is_division());
+ let sum: NumOpResult<Amount> = err.iter().sum();
+ assert!(sum.unwrap_err().operation().is_division());
+
+ let err = [SignedAmount::ONE_SAT / 0_i64];
+ let sum: NumOpResult<SignedAmount> = err.into_iter().sum();
+ assert!(sum.unwrap_err().operation().is_division());
+ let sum: NumOpResult<SignedAmount> = err.iter().sum();
+ assert!(sum.unwrap_err().operation().is_division());
+ }
+
#[test]
fn sum_amount_results() {
let amounts = [Why this scored 38/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.