Merge rust-bitcoin/rust-bitcoin#6862: units: Preserve error in NumOpResult add and sub
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6862: units: Preserve error in NumOpResult add and sub
b0d240127f78cffa4cfdba72c5b85bcfcd0a0810 units: Test add and sub preserve div-by-zero error (Jamil Lambert, PhD)
0004180290e08500d3833e7289451fdf78b8049a units: Preserve error in NumOpResult add and sub (Jamil Lambert, PhD)
Pull request description:
Adding or subtracting `NumOpResults` replaced any error operand with a fresh overflow error.
Propagate the original error instead of constructing a new overflow error.
Closes project-loupe/audit-rust-bitcoin#167
Closes project-loupe/audit-rust-bitcoin#168
ACKs for top commit:
tcharding:
ACK b0d240127f78cffa4cfdba72c5b85bcfcd0a0810
Kixunil:
ACK b0d240127f78cffa4cfdba72c5b85bcfcd0a0810
apoelstra:
ACK b0d240127f78cffa4cfdba72c5b85bcfcd0a0810; successfully ran local tests
Tree-SHA512: f4f8ae1aaa011842fa0e350a43f1dff3b6fd3a944f971beeae85f6422a4b81d3e8f9c23df9c42ec870a72eeb09f13d16508eb1c977790e7b78147f702640d960
What changed, and why it matters
This commit fixes a bug in how the library handles math errors. Previously, if you added or subtracted two values and one of them already had an error (for example, dividing by zero), the library would silently replace that original error with a misleading 'overflow' error. Now it correctly keeps and reports the original error. This is a defensive correctness fix: it prevents error details from being lost, which could hide the true cause of a failure in downstream software.
Review whether other binary operations on NumOpResult (Mul, Div, Rem, etc.) have the same error-replacement behavior and apply consistent propagation if needed. Ensure downstream callers do not rely on the previous overflow-error behavior. No urgent deployment action is required; treat as a routine correctness/security-hardening fix.
Security signals we found
Error-state information loss in arithmetic wrapper type
Incorrect error propagation could mask prior failures such as division by zero
Defensive correctness fix in numeric operation result handling
No input validation bypass or memory-safety issue present in diff
Evidence from the diff
In units/src/result.rs, the Add and Sub implementations for NumOpResult
Changed components
units/src/result.rsNumOpResult<T>core::ops::Add implementation for NumOpResultcore::ops::Sub implementation for NumOpResultInspect captured patch +26 / −4
### units/src/result.rs
@@ -85,11 +85,11 @@ pub use self::error::NumOpError;
/// # Ok::<_, NumOpError>(())
/// ```
///
+/// If both operands of a binary op are already [`Error`], only the left-hand error is returned.
+///
/// [`Valid`]: NumOpResult::Valid
/// [`Error`]: NumOpResult::Error
/// [`unwrap`]: NumOpResult::unwrap
-/// [`Div`]: core::ops::Div
-/// [`Rem`]: core::ops::Rem
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
#[must_use]
pub enum NumOpResult<T> {
@@ -248,7 +248,8 @@ crate::internal_macros::impl_op_for_references! {
fn add(self, rhs: Self) -> Self::Output {
match (self, rhs) {
(R::Valid(lhs), R::Valid(rhs)) => lhs + rhs,
- (_, _) => R::Error(NumOpError::while_doing(MathErrorKind::Overflow { op: MathOp::Add, is_negative: false })),
+ (R::Error(e), _) => R::Error(e),
+ (_, R::Error(e)) => R::Error(e),
}
}
}
@@ -271,7 +272,8 @@ crate::internal_macros::impl_op_for_references! {
fn sub(self, rhs: Self) -> Self::Output {
match (self, rhs) {
(R::Valid(lhs), R::Valid(rhs)) => lhs - rhs,
- (_, _) => R::Error(NumOpError::while_doing(MathErrorKind::Overflow { op: MathOp::Sub, is_negative: true })),
+ (R::Error(e), _) => R::Error(e),
+ (_, R::Error(e)) => R::Error(e),
}
}
}
@@ -738,4 +740,24 @@ mod tests {
#[cfg(feature = "std")]
assert!(e.source().is_none());
}
+
+ #[test]
+ fn add_preserves_division_by_zero_error() {
+ let division_by_zero = Amount::from_sat_u32(1) / 0_u64;
+ let valid = NumOpResult::Valid(Amount::ZERO);
+
+ let combined = division_by_zero + valid;
+
+ assert!(combined.unwrap_err().operation().is_division());
+ }
+
+ #[test]
+ fn sub_preserves_division_by_zero_error() {
+ let division_by_zero = Amount::from_sat_u32(1) / 0_u64;
+ let valid = NumOpResult::Valid(Amount::ZERO);
+
+ let combined = valid - division_by_zero;
+
+ assert!(combined.unwrap_err().operation().is_division());
+ }
}Why this scored 49/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.