test: Fix CLI_MAX_ARG_SIZE issues #33243

maflcko · 2025-08-23T09:39:43Z

CLI_MAX_ARG_SIZE has many edge case issues:

It seems to be lower on some systems, but it is unknown how to reproduce locally: ci: limit max stack size to 512 KiB #33079 (comment)
MAX_ARG_STRLEN is a limit per arg, but we probably want "The maximum length of [all of] the arguments": See https://www.man7.org/linux/man-pages/man3/sysconf.3.html, section ARG_MAX - _SC_ARG_MAX.

It doesn't account for the additional args added by the bitcoin command later on:

Lines 85 to 92 in 73220fc

    
           args.emplace_back("bitcoin-cli"); 
        
           // Since "bitcoin rpc" is a new interface that doesn't need to be 
        
           // backward compatible, enable -named by default so it is convenient 
        
           // for callers to use a mix of named and unnamed parameters. Callers 
        
           // can override this by specifying -nonamed, but should not need to 
        
           // unless they are passing string values containing '=' characters 
        
           // as unnamed parameters. 
        
           args.emplace_back("-named");

It doesn't account for unicode encoding a string to bytes before taking its length.

The issues are mostly harmless edge cases, but it would be good to fix them. So do that here, by:

Replacing max() by sum(), to correctly take into account all args, not just the largest one.
Reduce CLI_MAX_ARG_SIZE, to account for the bitcoin command additional args.

Also, there is a test. The test can be called with ulimit to hopefully limit the max args size to the hard-coded value in the test framework. For reference:

$ ( ulimit -s 512 && python3 -c 'import os; print(os.sysconf("SC_ARG_MAX") )' ) 
131072

On top of this pull it should pass, ...

bash -c 'ulimit -s 512 && BITCOIN_CMD="bitcoin -M" ./bld-cmake/test/functional/rpc_misc.py --usecli -l DEBUG'

... and with the test_framework changes reverted, it should fail:

OSError: [Errno 7] Argument list too long: 'bitcoin'

Also, there is a commit to enable CI_LIMIT_STACK_SIZE=1 in the i686 task, because it should now be possible and no longer hit the hard-to-reproduce issue mentioned above.

DrahtBot · 2025-08-23T09:39:47Z

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/33243.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	cedwies, enirox001
Concept ACK	mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

cedwies · 2025-08-25T15:57:52Z

test/functional/rpc_misc.py

@@ -30,6 +30,11 @@ def run_test(self):
            lambda: node.echo(arg9='trigger_internal_bug'),
        )

+        self.log.info("test max arg size")


This test also passes on master (MacOS environment with ARG_MAX = 1048576). It does not fail with the test_framework changes reverted.
However, the following test only passes on this PR, not on my master:

self.log.info("test many small args (sum-size trigger)") tiny = "a" * 120000 args = [tiny] * 10 assert_equal(args, node.echo(*args))

This test also passes on master (MacOS environment with ARG_MAX = 1048576).

Thanks for adding a macos data point for reference. This further supports that the hard-coded value may be different on different platforms.

However, the following test only passes on this PR, not on my master:

Nice, thanks for adapting the test and checking the bugfix on macos.

cedwies · 2025-08-25T16:18:53Z

ACK fabe214

This PR fixes how the functional test framework estimates command-line argument size. Previously it checked only the longest argument, but the OS limit is on the total argv/env size, so it now sums all args. It also adds a functional test using very large echo args, which exercises the -stdin fallback. The test passes for me.

I appreciate lowering CLI_MAX_ARG_SIZE to 1024. It’s a very safe, conservative threshold that avoids platform-dependent ARG_MAX issues, and since larger payloads automatically go through -stdin anyway, I see no real downside. A nice fail-safe change IMO.

enirox001

Concept ACK fa18c82 — The fix is a welcome improvement: summing arg sizes better reflects kernel ARG_MAX behavior. Good use of >= to match inclusive boundary semantics. Left a few non-blocking clarifier nits inline.

test/functional/test_framework/test_node.py

test/functional/rpc_misc.py

cedwies · 2025-08-26T13:09:14Z

ACK fa96a4a

enirox001

ACK fa96a4a — thanks for addressing the nits and clarifying the test; LGTM.

mzumsande

It claims to be a "limit per arg", but it is a "The maximum length of the arguments": See https://www.man7.org/linux/man-pages/man3/sysconf.3.html, section ARG_MAX - _SC_ARG_MAX.

the relevant bottleneck for me (tested on Ubuntu and Debian) was not ARG_MAX, but MAX_ARG_STRLEN, which accounts for the maximum per arg, not the sum.
e.g. the folllowing passes

arg1=$(printf "%0*d" $((131072 - 1)) 0)
arg2=$(printf "%0*d" $((131072 - 1)) 0)
$(which printf) "%s %s" "$arg1" "$arg2" >/dev/null

whereas

arg3=$(printf "%0*d" $((131072)) 0)
$(which printf) "%s" "$arg3" >/dev/null

fails with bash: /usr/bin/printf: Argument list too long

But seeing how much this apparently differs between systems, Concept ACK to using a smaller limit.

maflcko · 2025-08-27T05:48:09Z

the relevant bottleneck for me (tested on Ubuntu and Debian) was not ARG_MAX, but MAX_ARG_STRLEN, which accounts for the maximum per arg, not the sum.

Thanks for clarifying that this was intentionally picked and that MAX_ARG_STRLEN is a limit per arg, separate from the command size limit SC_ARG_MAX. SC_ARG_MAX is larger on many modern systems, but not all. Also, it reduces to MAX_ARG_STRLEN, or even lower on some systems. I've edited the pull description, to better reflect this.

maflcko · 2025-09-04T10:12:09Z

(+GHA ci)

maflcko changed the title ~~2508 test fix cli cmd size~~ test: Fix CLI_MAX_ARG_SIZE issues Aug 23, 2025

DrahtBot changed the title ~~test: Fix CLI_MAX_ARG_SIZE issues~~ test: Fix CLI_MAX_ARG_SIZE issues Aug 23, 2025

DrahtBot added the Tests label Aug 23, 2025

maflcko mentioned this pull request Aug 23, 2025

ci: limit max stack size to 512 KiB #33079

Merged

cedwies reviewed Aug 25, 2025

View reviewed changes

enirox001 reviewed Aug 25, 2025

View reviewed changes

test/functional/test_framework/test_node.py Show resolved Hide resolved

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved

test/functional/rpc_misc.py Outdated Show resolved Hide resolved

maflcko force-pushed the 2508-test-fix-cli-cmd-size branch 2 times, most recently from fa25f97 to fac47f6 Compare August 26, 2025 11:13

DrahtBot added the CI failed label Aug 26, 2025

MarcoFalke added 2 commits August 26, 2025 13:20

test: Fix CLI_MAX_ARG_SIZE issues

facfde2

ci: Enable CI_LIMIT_STACK_SIZE=1 in i686_no_ipc task

fa96a4a

maflcko force-pushed the 2508-test-fix-cli-cmd-size branch from fac47f6 to fa96a4a Compare August 26, 2025 11:20

DrahtBot removed the CI failed label Aug 26, 2025

DrahtBot requested a review from enirox001 August 26, 2025 13:09

enirox001 reviewed Aug 26, 2025

View reviewed changes

mzumsande reviewed Aug 26, 2025

View reviewed changes

maflcko closed this Sep 4, 2025

maflcko reopened this Sep 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: Fix CLI_MAX_ARG_SIZE issues #33243

test: Fix CLI_MAX_ARG_SIZE issues #33243

maflcko commented Aug 23, 2025 •

edited

Loading

Uh oh!

DrahtBot commented Aug 23, 2025 •

edited

Loading

Uh oh!

cedwies Aug 25, 2025

Uh oh!

maflcko Aug 26, 2025

Uh oh!

cedwies commented Aug 25, 2025

Uh oh!

enirox001 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cedwies commented Aug 26, 2025

Uh oh!

enirox001 left a comment

Uh oh!

mzumsande left a comment •

edited

Loading

Uh oh!

maflcko commented Aug 27, 2025

Uh oh!

maflcko commented Sep 4, 2025

Uh oh!

Uh oh!

	args.emplace_back("bitcoin-cli");
	// Since "bitcoin rpc" is a new interface that doesn't need to be
	// backward compatible, enable -named by default so it is convenient
	// for callers to use a mix of named and unnamed parameters. Callers
	// can override this by specifying -nonamed, but should not need to
	// unless they are passing string values containing '=' characters
	// as unnamed parameters.
	args.emplace_back("-named");

test: Fix CLI_MAX_ARG_SIZE issues #33243

Are you sure you want to change the base?

test: Fix CLI_MAX_ARG_SIZE issues #33243

Conversation

maflcko commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Uh oh!

cedwies Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

cedwies commented Aug 25, 2025

Uh oh!

enirox001 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cedwies commented Aug 26, 2025

Uh oh!

enirox001 left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 27, 2025

Uh oh!

maflcko commented Sep 4, 2025

Uh oh!

Uh oh!

maflcko commented Aug 23, 2025 •

edited

Loading

DrahtBot commented Aug 23, 2025 •

edited

Loading

mzumsande left a comment •

edited

Loading