#36366 net: limit relative ban times to uint32
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36366 net: limit relative ban times to uint32 | Ready | 0 (+1) | Strong | 6 + 25 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Caps relative ban durations to UINT32_MAX in both the setban RPC and the -bantime configuration argument. The setban RPC rejects relative ban durations exceeding UINT32_MAX with an invalid parameter error, while -bantime clamps values greater than UINT32_MAX using std::min. Absolute timestamps remain unaffected.
Problem: BanMan calculates ban expiration as GetTime() + bantime using signed 64-bit integers. If a relative ban duration near INT64_MAX is provided, the addition overflows, causing a signed integer overflow runtime error under UBSan; without UBSan, it wraps negative, causing the node to treat the ban as already expired and silently drop it while setban still returns success.
Evidence of importance:
- PR description quotes a UBSan runtime error: src/banman.cpp:140:73: runtime error: signed integer overflow: 1790412558 + 9223372036854775807 cannot be represented in type 'int64_t'
Risk of a mistake: medium (rpc_api). A flaw in duration validation could reject valid bans or cause bans to expire at unintended times for node operators.
Changed since the previous assessment: Switched from parsing -bantime as uint32_t to keeping GetIntArg and clamping with std::min so oversized values in settings.json do not abort node startup, and added a test for settings.json.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: willcl-ark to review fix for settings.json bantime parsing 2026-10-02
Agreement
Strong
substantive support, no open objection (maflcko)
Thread notes: willcl-ark pointed out an issue where values > uint32 in settings.json throw on startup; the author pushed a fix parsing as int64 and capping at UINT32_MAX.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| maflcko (member) | objection, approach | Supporting durations unsupported by C++ chrono using SaturatingAdd does not work for chrono types; excessively large bantime arguments should be rejected instead | accepted · fix pushed · author switched to rejecting large bantimes and reviewer accepted with an ACK | changed | yes | 2026-09-28: 'I am not sure about starting/trying to support durations that C++ itself doesn't even support... Also, `SaturatingAdd` doesn't work for chrono types ...' Status from: 2026-09-29: maflcko: 'Seems fine to reject or clamp values larger than any meaningful use-case.' |
| maflcko (member) | suggestion, approach | Reject relative ban times above UINT32_MAX to avoid a time dependency | accepted · fix pushed · author implemented uint32 limit in setban and reviewer ACKed | answered | yes | 2026-09-29: 'This should be more than enough and also avoid the weird time dependency.' Status from: 2026-09-29: maflcko: 'lgtm ACK d49c1c9ab3e139158d76141b71943ff79903e87a' |
| maflcko (member) | suggestion, approach | Parse bantime as u32 | accepted · fix pushed · author parsed -bantime as uint32_t and reviewer ACKed | answered | yes | 2026-09-29: 'What about parsing it as u32?' Status from: 2026-09-29: maflcko: 'lgtm ACK d49c1c9ab3e139158d76141b71943ff79903e87a' |
| willcl-ark (member) | objection, correctness | A bantime value greater than uint32 in settings.json throws during conversion on startup | fixed · fix pushed · author reverted -bantime parsing to int64, capped at UINT32_MAX, and added a test case | changed | yes | 2026-10-02: 'settings.json can also have a value > uint32 which will throw during conversion' Status from: 2026-10-02: Bruce039: '`-bantime` is now read as int64 again and capped at UINT32_MAX, so it no longer matters where the value comes from.' |
Support:
- maflcko (member) (ACK, 2026-09-29) [read]: Seems fine to reject or clamp values larger than any meaningful use-case.
Participants:
- maflcko (member, support, 3 statements 2026-09-28 to 2026-09-29): ACKed after author addressed objection to SaturatingAdd and adopted suggestions to limit bantime to uint32
- willcl-ark (member, objection, 1 statements 2026-10-02 to 2026-10-02): pointed out bantime > uint32 in settings.json throws on startup
Files
25 lines under test/bench/ci.
- test/functional/rpc_setban.py +25/-0
- src/rpc/net.cpp +5/-0
- src/init.cpp +1/-1
Card
The short text the ranking pass compares across PRs.
Caps relative ban durations to UINT32_MAX across the setban RPC and the -bantime startup option. Relative ban durations near INT64_MAX currently cause signed 64-bit integer overflow in BanMan, triggering UBSan errors and wrapping to negative timestamps that silently discard bans while reporting success. With this change, setban rejects relative durations exceeding UINT32_MAX, and -bantime clamps larger values without failing startup.
Data
thread read through 2026-10-02 (7 statements), head 514ff32e2f
labels: RPC/REST/ZMQ
Categories
RPC / REST / ZMQ
P3 · bug fix (ranking pass: Moved up 15: setban silently dropping bans while reporting success is a false-success bug like the others grouped here.)
Prevents signed integer overflow and silent ban drops when callers provide relative ban times near INT64_MAX. As noted in the description, 'GetTime() + bantime... wraps negative, causing the node to treat the ban as already expired and silently drop it while setban still returns success.'
Membership: Matches RPC/REST/ZMQ label and modifies `src/rpc/net.cpp` parameter validation for the `setban` RPC method.
Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 0