fix: resolve signed integer overflow UB in CoinJoin priority and timeout#7236
fix: resolve signed integer overflow UB in CoinJoin priority and timeout#7236thepastaclaw wants to merge 3 commits intodashpay:developfrom
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe pull request contains two safety and optimization improvements to the coinjoin module. The first change simplifies timeout validation in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/coinjoin/coinjoin.cpp`:
- Around line 57-61: Run clang-format on the indicated block (or the whole file)
to fix the whitespace/line-wrapping so the ternary expression and return
statement match project style; reformat the block containing current_time, nTime
and COINJOIN_QUEUE_TIMEOUT (the diff calculation and return) using your
project's clang-format config and commit the changes so the Clang Diff Format
Check passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 82b1eff3-1f1e-4510-a19f-8c06e26a762b
📒 Files selected for processing (2)
src/coinjoin/coinjoin.cppsrc/coinjoin/common.h
thepastaclaw
force-pushed
the
fix-coinjoin-ub
branch
from
4248612 to
dbfbbe6
Compare
CalculateAmountPriority in common.h could overflow when assigning a negated int64_t division result to an int return type with extreme CAmount values. Clamp the result to INT_MIN/INT_MAX before returning. IsTimeOutOfBounds in coinjoin.cpp could overflow on signed subtraction when current_time and nTime differ by more than INT64_MAX. Use unsigned arithmetic to compute the absolute difference safely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix-coinjoin-ub
branch
from
cb601cd to
ab4bea6
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, correct two-line fix for signed integer overflow UB in CoinJoin timeout and priority. Both arithmetic changes are mathematically sound and preserve original semantics. The only gap is the absence of regression tests exercising the extreme-value inputs that motivated these fixes.
Reviewed commit: ab4bea6
🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
💬 nitpick: Pre-existing: float-to-int implicit narrowing on denomination priority return
src/coinjoin/common.h (line 125)
Line 125 returns (float)COIN / *optDenom * 10000, which undergoes implicit float-to-int conversion. With current denominations the maximum result is ~10,000,000 (for the smallest denomination COIN/1000+1), safely within INT_MAX. But this is the same class of narrowing issue this PR fixes for the nondenom path. Not blocking since current values are safe, but worth noting for consistency.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/coinjoin.cpp`:
- [SUGGESTION] lines 60-62: No regression test for extreme timestamp values in IsTimeOutOfBounds
The fix is correct: unsigned subtraction after a signed comparison produces the exact mathematical absolute difference for all int64_t pairs, including mixed-sign and extreme values. However, existing tests in coinjoin_queue_tests.cpp (deltas of ±15, ±60) and coinjoin_inouts_tests.cpp (delta of COINJOIN_QUEUE_TIMEOUT+1) only cover small time deltas around realistic timestamps. No test exercises the scenario that triggered the UBSan finding — e.g., current_time = INT64_MAX and nTime = INT64_MIN, or vice versa. A targeted test would prevent regressions and document the fix motivation.
In `src/coinjoin/common.h`:
- [SUGGESTION] lines 132-133: No regression test for CalculateAmountPriority clamp boundary
The clamp is correct. Note that with valid money amounts, the old code would not have overflowed int: -(MAX_MONEY / COIN) = -21000000, well within INT_MIN..INT_MAX. The UB requires nInputAmount > INT_MAX * COIN (~2.1×10¹⁷), which exceeds MAX_MONEY but is representable in int64_t. Since CalculateAmountPriority accepts CAmount (int64_t) without a MoneyRange guard, fuzz testing can reach these values. There are no tests for this function at all — the function is constexpr, so a static_assert or compile-time test could verify the clamping directly.
| const uint64_t diff = (current_time > nTime) ? static_cast<uint64_t>(current_time) - static_cast<uint64_t>(nTime) | ||
| : static_cast<uint64_t>(nTime) - static_cast<uint64_t>(current_time); | ||
| return diff > static_cast<uint64_t>(COINJOIN_QUEUE_TIMEOUT); |
There was a problem hiding this comment.
🟡 Suggestion: No regression test for extreme timestamp values in IsTimeOutOfBounds
The fix is correct: unsigned subtraction after a signed comparison produces the exact mathematical absolute difference for all int64_t pairs, including mixed-sign and extreme values. However, existing tests in coinjoin_queue_tests.cpp (deltas of ±15, ±60) and coinjoin_inouts_tests.cpp (delta of COINJOIN_QUEUE_TIMEOUT+1) only cover small time deltas around realistic timestamps. No test exercises the scenario that triggered the UBSan finding — e.g., current_time = INT64_MAX and nTime = INT64_MIN, or vice versa. A targeted test would prevent regressions and document the fix motivation.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/coinjoin.cpp`:
- [SUGGESTION] lines 60-62: No regression test for extreme timestamp values in IsTimeOutOfBounds
The fix is correct: unsigned subtraction after a signed comparison produces the exact mathematical absolute difference for all int64_t pairs, including mixed-sign and extreme values. However, existing tests in coinjoin_queue_tests.cpp (deltas of ±15, ±60) and coinjoin_inouts_tests.cpp (delta of COINJOIN_QUEUE_TIMEOUT+1) only cover small time deltas around realistic timestamps. No test exercises the scenario that triggered the UBSan finding — e.g., current_time = INT64_MAX and nTime = INT64_MIN, or vice versa. A targeted test would prevent regressions and document the fix motivation.
| const int64_t val = -(nInputAmount / COIN); | ||
| return int(std::clamp<int64_t>(val, std::numeric_limits<int>::min(), std::numeric_limits<int>::max())); |
There was a problem hiding this comment.
🟡 Suggestion: No regression test for CalculateAmountPriority clamp boundary
The clamp is correct. Note that with valid money amounts, the old code would not have overflowed int: -(MAX_MONEY / COIN) = -21000000, well within INT_MIN..INT_MAX. The UB requires nInputAmount > INT_MAX * COIN (~2.1×10¹⁷), which exceeds MAX_MONEY but is representable in int64_t. Since CalculateAmountPriority accepts CAmount (int64_t) without a MoneyRange guard, fuzz testing can reach these values. There are no tests for this function at all — the function is constexpr, so a static_assert or compile-time test could verify the clamping directly.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/common.h`:
- [SUGGESTION] lines 132-133: No regression test for CalculateAmountPriority clamp boundary
The clamp is correct. Note that with valid money amounts, the old code would not have overflowed int: -(MAX_MONEY / COIN) = -21000000, well within INT_MIN..INT_MAX. The UB requires nInputAmount > INT_MAX * COIN (~2.1×10¹⁷), which exceeds MAX_MONEY but is representable in int64_t. Since CalculateAmountPriority accepts CAmount (int64_t) without a MoneyRange guard, fuzz testing can reach these values. There are no tests for this function at all — the function is constexpr, so a static_assert or compile-time test could verify the clamping directly.
There was a problem hiding this comment.
The value for this function is coming as effective_value:
/** The value of the UTXOs after deducting the cost of spending them at the effective feerate. */
CAmount effective_value{0};
which is never bigger than MAX_MONEY.
This fix is unexpected complication of code IMO
There was a problem hiding this comment.
You're right that in practice effective_value is always bounded by MAX_MONEY, so -(MAX_MONEY / COIN) = -21000000 fits comfortably in int. The narrowing is safe for all realistic inputs.
The clamp was added because UBSan flagged this during fuzz testing — the fuzz harness can feed arbitrary CAmount values to CalculateAmountPriority without going through the wallet code path that enforces the money range constraint. The implicit int64_t → int narrowing is technically undefined behavior per the standard when the value doesn't fit, even if it's unreachable in production.
That said, I can see the argument that defending against inputs that can't occur in practice adds unnecessary complexity. A few options:
- Keep the clamp (current) — zero-cost for realistic values, makes the function correct for all inputs
- Replace with
Assume()— e.g.Assume(std::abs(val) <= INT_MAX)to document the precondition without the clamp - Remove the clamp entirely — just add a comment that narrowing is safe because callers always pass values ≤ MAX_MONEY
Happy to go with whichever you prefer. The IsTimeOutOfBounds fix (unsigned arithmetic) is the more critical one — it can trigger on any signed subtraction with large deltas.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Test-only follow-up commit adding comprehensive regression tests for both UB fixes. All prior review suggestions are now addressed: queue_timestamp_extreme_values covers INT64_MIN/INT64_MAX edge cases for IsTimeOutOfBounds, and calculate_amount_priority_clamp (plus two static_asserts) verifies the clamping boundary for CalculateAmountPriority. Tests are well-structured and exercise the exact scenarios that motivated the fixes.
Reviewed commit: 1053f37
thepastaclaw
force-pushed
the
fix-coinjoin-ub
branch
from
1053f37 to
4ef1992
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix-coinjoin-ub
branch
from
4ef1992 to
4cc88ea
Compare
|
Updated per knst's feedback: replaced magic numbers in tests with symbolic expressions ( |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Incremental Review (push 4cc88eaf vs prior reviewed 1053f373)
Delta: 1 file changed — src/test/coinjoin_queue_tests.cpp (4 insertions, 5 deletions)
Test-only commit replacing hardcoded magic numbers with symbolic constants: -21000000 → -(MAX_MONEY / COIN), -2147483647 → -INT_MAX. Pure readability improvement, no behavioral change.
Prior review stands — no new findings. ✅
| } | ||
|
|
||
| static_assert(CoinJoin::CalculateAmountPriority(MAX_MONEY) == -(MAX_MONEY / COIN)); | ||
| static_assert(CoinJoin::CalculateAmountPriority(static_cast<CAmount>(INT64_MAX)) == std::numeric_limits<int>::min()); |
There was a problem hiding this comment.
this unit test doesn't make any sense, because CAmount for CJ never could be more than MAX_MONEY, isn't it?
There was a problem hiding this comment.
Agreed — the calculate_amount_priority_clamp test and static_assert are only meaningful if we keep the clamp. If we drop the clamp per the discussion above, these tests should go too.
The queue_timestamp_extreme_values tests (for IsTimeOutOfBounds) are still important though — those exercise the actual UB that existed in the signed subtraction path.
|
We could simply bail out on invalid values instead of trying to handle them i.e. |
3 participants
Summary
Fix two signed integer overflow UB issues in CoinJoin code, found during fuzz testing.
CalculateAmountPriority(common.h)The return type is
intbut the computation-(nInputAmount / COIN)operates onint64_tvalues. WhennInputAmountis extremely large (e.g. nearMAX_MONEY),the result exceeds
INT_MAXand the implicit narrowing tointis undefinedbehavior under UBSan.
Fix: Clamp the
int64_tresult to[INT_MIN, INT_MAX]before returning.This preserves the existing sort ordering for all realistic inputs while making
extreme values well-defined.
IsTimeOutOfBounds(coinjoin.cpp)The expression
current_time - nTimeoverflows when the twoint64_tvaluesdiffer by more than
INT64_MAX(e.g. one large positive, one large negative).Fix: Compute the absolute difference using unsigned arithmetic, which is
well-defined for all inputs.
Validation
and the priority function only affects local sort order
ci/fuzz-regressionbranch