common: saturate marginal_feerate() instead of overflowing - #9365
common: saturate marginal_feerate() instead of overflowing#9365morehouse wants to merge 1 commit into
marginal_feerate() instead of overflowing#9365Conversation
marginal_feerate() computed current_feerate * 1.1 as a double and
converted the result back to u32. Since current_feerate is chosen by
the peer in open_channel or update_fee, they could choose an absurdly
high value that overflows u32 after the computation. UBSan reports:
common/fee_states.c:179:10: runtime error: 4.72446e+09 is outside the range of representable values of type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior common/fee_states.c:179:10
Do the arithmetic with u64 and saturate at UINT32_MAX to avoid the
undefined behavior.
Found by fuzzing with smite.
Changelog-Fixed: JSON-RPC: `listpeerchannels` no longer derives `receivable_msat` from an overflowed fee estimate when the peer sets an absurd `feerate_per_kw`.
Andezion
left a comment
Andezion
left a comment
There was a problem hiding this comment.
The fix correctly addresses the UBSan-reported issue, the arithmetic is correct (I verified it numerically), and the added tests cover the exact boundary condition
| if (current_feerate > maxfeerate) | ||
| return current_feerate * 1.1; | ||
| if (current_feerate > maxfeerate) { | ||
| u64 marginal = ((u64)current_feerate * 11) / 10; |
There was a problem hiding this comment.
Is it intentional that the fixed version rounds down ((current_feerate * 11) / 10 truncates), while the old double version also truncated on conversion to u32? I confirmed both truncate the same way for in-range values, so behavior should be unchanged for normal feerates - just confirming this was checked, since it affects the exact fee charged to peers, not just an internal-only value :)
There was a problem hiding this comment.
I was simply trying to preserve the existing behavior. However, I don't think it really matters whether we use floor, round, or ceil here, since this function only influences the "fee spike buffer", whose size is a fuzzy concept to begin with.
2 participants
marginal_feerate()computedcurrent_feerate * 1.1as a double and converted the result back to u32. Sincecurrent_feerateis chosen by the peer viaopen_channelorupdate_fee, they could choose an absurdly high value that overflows u32 after the computation. UBSan reports:Do the arithmetic with u64 and saturate at
UINT32_MAXto avoid the undefined behavior.Found by fuzzing with smite.