wallet: avoid int overflow in listtransactions #36339

kriss39 commented at 9:38 AM on September 26, 2026: contributor

listtransactions adds count and skip as int. Each one is checked to be non-negative, but their sum can still go past INT_MAX:

bitcoin-cli -regtest listtransactions "*" 2147483647 1

On master this crashes the node. The overflowed sum is negative, so the loop stops after the first entry. The clamp after the loop overflows the same way and doesn't fix nCount, so push_backV gets an iterator range about 2^31 elements before rend(). UBSan reports signed integer overflow: 1 + 2147483647 at transactions.cpp:584, and ASan then reports a BUS error in UniValue::push_backV.

This parses count and skip as uint32_t through self.Arg (which also handles the defaults and rejects negative values, so the manual checks go away) and adds them up in 64 bits, where the sum can't overflow. The functional test calls listtransactions with count + skip above INT_MAX and up to UINT32_MAX and checks the results match a normal-sized request. It times out on master (the node crashes) and passes with the fix. I also ran it on an ASan/UBSan build.

listrawtransactions counts differently and isn't affected.

DrahtBot added the label Wallet on Sep 26, 2026

DrahtBot commented at 9:38 AM on September 26, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36339.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	maflcko

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36264 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36264.svg"> (wallet rpc: fix stale argument metadata and help text by MrHodlX)
#34872 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/34872.svg"> (wallet: fix mixed-input transaction accounting in history RPCs by w0xlt)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

kriss39 force-pushed on Sep 26, 2026

DrahtBot added the label CI failed on Sep 26, 2026

DrahtBot removed the label CI failed on Sep 26, 2026

in src/wallet/rpc/transactions.cpp:572 in 6aca05e7b9

maflcko commented at 8:05 AM on September 28, 2026:

This is unrelated, but pretty much every line here is problematic:

The default values are hard-coded a second time manually
The values are parsed as a singed int, but negative values are rejected manually

I'd say all of this can be avoided by using self.Arg<uint32_t>("count"). etc (or u64)

Possibly there could be an early check for AdditionOverflow<u64>(count,from) and early exit, to avoid having to manually cast later on?

kriss39 commented at 8:27 AM on September 28, 2026:

Thanks, done. count and skip now go through self.Arg<uint64_t>, so the duplicated defaults and the manual negative checks are gone.

For the sum I used SaturatingAdd rather than an early exit: something like count=UINT64_MAX, skip=1 is a valid "everything but the newest" request, so returning early would give the wrong answer there. With the saturated limit the rest is plain unsigned std::min, no casts.

Negative values now fail with "JSON integer out of range" (-1) instead of "Negative count"/"Negative from" (-8), same as other Arg<uint*> params. Test updated for that, plus a UINT64_MAX case. I left listrawtransactions alone to keep this small.

kriss39 force-pushed on Sep 28, 2026

DrahtBot added the label CI failed on Sep 28, 2026

DrahtBot commented at 9:30 AM on September 28, 2026: contributor

🚧 At least one of the CI tasks failed. Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/36397480883/job/108847201668 LLM reason (✨ experimental): CI failed due to a C++ build error in src/wallet/rpc/transactions.cpp (-Werror=narrowing: narrowing conversion from uint64_t to size_t).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

kriss39 force-pushed on Sep 28, 2026

wallet: avoid int overflow in listtransactions

count and skip were parsed as int and checked to be non-negative, but
count + skip could still overflow, crashing the node for e.g.
`listtransactions "*" 2147483647 1`.

Parse both as uint32_t via self.Arg, which also takes care of the
defaults and rejects negative values, and add them up in 64 bits,
where the sum can't overflow.

9cff82fd5b

in src/wallet/rpc/transactions.cpp:562 in cd41566bf6

 568 | -
 569 | -    if (nCount < 0)
 570 | -        throw JSONRPCError(RPC_INVALID_PARAMETER, "Negative count");
 571 | -    if (nFrom < 0)
 572 | -        throw JSONRPCError(RPC_INVALID_PARAMETER, "Negative from");
 573 | +    const auto count{self.Arg<uint64_t>("count")};

maflcko commented at 2:11 PM on September 28, 2026:

The max was i32::max previously, it seems odd to raise it to u64, when u32 should be enough and also avoid the cast further down below?

kriss39 commented at 4:33 PM on September 28, 2026:

Right, switched both to uint32_t. The sum is done in 64 bits so it can't overflow, and the casts are gone. Test updated to go up to UINT32_MAX and check that UINT32_MAX + 1 is rejected.

kriss39 force-pushed on Sep 28, 2026

DrahtBot removed the label CI failed on Sep 28, 2026

maflcko commented at 7:10 AM on September 29, 2026: member

lgtm ACK 9cff82fd5b15bc2aa2c7778b61212876433a8219

Looks like a nice low-prio style cleanup to remove some confusing and incorrect c-style int casts and instead avoid overflow by capping to u32::max.