Merge bitcoin/bitcoin#33243: test: Fix CLI_MAX_ARG_SIZE issues
fa96a4afeaci: Enable CI_LIMIT_STACK_SIZE=1 in i686_no_ipc task (MarcoFalke)facfde2cdctest: 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: https://github.com/bitcoin/bitcoin/pull/33079#issuecomment-3139957274 * `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: ACKfa96a4aachow101: ACKfa96a4afeaenirox001: ACKfa96a4a— thanks for addressing the nits and clarifying the test; LGTM. mzumsande: Code Review ACKfa96a4afeaTree-SHA512: d12211bd097d692d560c3615970ec0e911707d8c6cbbb145591abc548beed55f487a80b08f0a8c89d4eef4d76a9fbd6a33edc0b42b5860a93dd7b954355bc887
This commit is contained in:
3 files changed
+21
-4
No files matched your search
@@ -13,6 +13,7 @@ export CI_IMAGE_PLATFORM="linux/amd64"
|
||||
export PACKAGES="llvm clang g++-multilib"
|
||||
export DEP_OPTS="DEBUG=1 NO_IPC=1"
|
||||
export GOAL="install"
|
||||
export CI_LIMIT_STACK_SIZE=1
|
||||
export TEST_RUNNER_EXTRA="--v2transport --usecli"
|
||||
export BITCOIN_CONFIG="\
|
||||
-DCMAKE_BUILD_TYPE=Debug \
|
||||
|
||||
Reference in new issue
Block a user