Skip to content

Commit b814453

Browse files
committed
Merge bitcoin/bitcoin#33243: test: Fix CLI_MAX_ARG_SIZE issues
fa96a4a ci: Enable CI_LIMIT_STACK_SIZE=1 in i686_no_ipc task (MarcoFalke) facfde2 test: Fix CLI_MAX_ARG_SIZE issues (MarcoFalke) Pull request description: `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: bitcoin/bitcoin#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: https://github.com/bitcoin/bitcoin/blob/73220fc0f958f9b65f66cf0cf042af220b312fc6/src/bitcoin.cpp#L85-L92 * 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. ACKs for top commit: cedwies: ACK fa96a4a achow101: ACK fa96a4a enirox001: ACK fa96a4a — thanks for addressing the nits and clarifying the test; LGTM. mzumsande: Code Review ACK fa96a4a Tree-SHA512: d12211bd097d692d560c3615970ec0e911707d8c6cbbb145591abc548beed55f487a80b08f0a8c89d4eef4d76a9fbd6a33edc0b42b5860a93dd7b954355bc887
2 parents f757da8 + fa96a4a commit b814453

File tree

3 files changed

+21
-4
lines changed

3 files changed

+21
-4
lines changed

ci/test/00_setup_env_i686_no_ipc.sh

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ export CI_IMAGE_PLATFORM="linux/amd64"
1313
export PACKAGES="llvm clang g++-multilib"
1414
export DEP_OPTS="DEBUG=1 NO_IPC=1"
1515
export GOAL="install"
16+
export CI_LIMIT_STACK_SIZE=1
1617
export TEST_RUNNER_EXTRA="--v2transport --usecli"
1718
export BITCOIN_CONFIG="\
1819
-DCMAKE_BUILD_TYPE=Debug \

test/functional/rpc_misc.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,13 @@ def run_test(self):
3030
lambda: node.echo(arg9='trigger_internal_bug'),
3131
)
3232

33+
self.log.info("test max arg size")
34+
ARG_SZ_COMMON = 131071 # Common limit, used previously in the test framework, serves as a regression test
35+
ARG_SZ_LARGE = 8 * 1024 * 1024 # A large size, which should be rare to hit in practice
36+
for arg_sz in [0, 1, 100, ARG_SZ_COMMON, ARG_SZ_LARGE]:
37+
arg_string = 'a' * arg_sz
38+
assert_equal([arg_string, arg_string], node.echo(arg_string, arg_string))
39+
3340
self.log.info("test getmemoryinfo")
3441
memory = node.getmemoryinfo()['locked']
3542
assert_greater_than(memory['used'], 0)

test/functional/test_framework/test_node.py

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,12 @@
4848
# The size of the blocks xor key
4949
# from InitBlocksdirXorKey::xor_key.size()
5050
NUM_XOR_BYTES = 8
51-
CLI_MAX_ARG_SIZE = 131071 # many systems have a 128kb limit per arg (MAX_ARG_STRLEN)
51+
# Many systems have a 128kB limit for a command size. Depending on the
52+
# platform, this limit may be larger or smaller. Moreover, when using the
53+
# 'bitcoin' command, it may internally insert more args, which must be
54+
# accounted for. There is no need to pick the largest possible value here
55+
# anyway and it should be fine to set it to 1kB in tests.
56+
TEST_CLI_MAX_ARG_SIZE = 1024
5257

5358
# The null blocks key (all 0s)
5459
NULL_BLK_XOR_KEY = bytes([0] * NUM_XOR_BYTES)
@@ -951,10 +956,14 @@ def send_cli(self, clicommand=None, *args, **kwargs):
951956
if clicommand is not None:
952957
p_args += [clicommand]
953958
p_args += pos_args + named_args
954-
max_arg_size = max(len(arg) for arg in p_args)
959+
960+
# TEST_CLI_MAX_ARG_SIZE is set low enough that checking the string
961+
# length is enough and encoding to bytes is not needed before
962+
# calculating the sum.
963+
sum_arg_size = sum(len(arg) for arg in p_args)
955964
stdin_data = self.input
956-
if max_arg_size > CLI_MAX_ARG_SIZE:
957-
self.log.debug(f"Cli: Command size {max_arg_size} too large, using stdin")
965+
if sum_arg_size >= TEST_CLI_MAX_ARG_SIZE:
966+
self.log.debug(f"Cli: Command size {sum_arg_size} too large, using stdin")
958967
rpc_args = "\n".join([arg for arg in p_args[base_arg_pos:]])
959968
if stdin_data is not None:
960969
stdin_data += "\n" + rpc_args

0 commit comments

Comments
 (0)