From faeb58fe668662d8262c4cc7c54ad2af756dbe3b Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 22 May 2025 15:09:53 +0200 Subject: [PATCH 1/3] refactor: Set G_ABORT_ON_FAILED_ASSUME when G_FUZZING_BUILD This does not change behavior, but documents that G_ABORT_ON_FAILED_ASSUME is set when G_FUZZING_BUILD. --- src/util/check.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/check.h b/src/util/check.h index f7dea5a3c2e..198e9e078b2 100644 --- a/src/util/check.h +++ b/src/util/check.h @@ -21,7 +21,7 @@ constexpr bool G_FUZZING_BUILD{ false #endif }; -constexpr bool G_ABORT_ON_FAILED_ASSUME{ +constexpr bool G_ABORT_ON_FAILED_ASSUME{G_FUZZING_BUILD || #ifdef ABORT_ON_FAILED_ASSUME true #else @@ -75,7 +75,7 @@ void assertion_fail(std::string_view file, int line, std::string_view func, std: template constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion) { - if (IS_ASSERT || std::is_constant_evaluated() || G_FUZZING_BUILD || G_ABORT_ON_FAILED_ASSUME) { + if (IS_ASSERT || std::is_constant_evaluated() || G_ABORT_ON_FAILED_ASSUME) { if (!val) { assertion_fail(file, line, func, assertion); } From fa0dc4bdffb06b6f0c192fe1aa02b4dfdcdc6e15 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 23 May 2025 08:47:35 +0200 Subject: [PATCH 2/3] test: Allow testing of check failures This allows specific tests to mock the check behavior to consistently use exceptions instead of aborts for intentionally failing checks in all build configurations. --- src/test/CMakeLists.txt | 1 + src/test/util_check_tests.cpp | 33 +++++++++++++++++++++++++++++++++ src/util/check.cpp | 5 +++++ src/util/check.h | 6 ++++++ 4 files changed, 45 insertions(+) create mode 100644 src/test/util_check_tests.cpp diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index 93db34d5a21..fa0e8791180 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -114,6 +114,7 @@ add_executable(test_bitcoin txvalidation_tests.cpp txvalidationcache_tests.cpp uint256_tests.cpp + util_check_tests.cpp util_string_tests.cpp util_tests.cpp util_threadnames_tests.cpp diff --git a/src/test/util_check_tests.cpp b/src/test/util_check_tests.cpp new file mode 100644 index 00000000000..93ac9194604 --- /dev/null +++ b/src/test/util_check_tests.cpp @@ -0,0 +1,33 @@ +// Copyright (c) The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include + +BOOST_AUTO_TEST_SUITE(util_check_tests) + +BOOST_AUTO_TEST_CASE(check_pass) +{ + Assume(true); + Assert(true); + CHECK_NONFATAL(true); +} + +BOOST_AUTO_TEST_CASE(check_fail) +{ + // Disable aborts for easier testing here + test_only_CheckFailuresAreExceptionsNotAborts mock_checks{}; + + if constexpr (G_ABORT_ON_FAILED_ASSUME) { + BOOST_CHECK_EXCEPTION(Assume(false), NonFatalCheckError, HasReason{"Internal bug detected: false"}); + } else { + BOOST_CHECK_NO_THROW(Assume(false)); + } + BOOST_CHECK_EXCEPTION(Assert(false), NonFatalCheckError, HasReason{"Internal bug detected: false"}); + BOOST_CHECK_EXCEPTION(CHECK_NONFATAL(false), NonFatalCheckError, HasReason{"Internal bug detected: false"}); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util/check.cpp b/src/util/check.cpp index 9f07fbe4780..77783c843d7 100644 --- a/src/util/check.cpp +++ b/src/util/check.cpp @@ -27,8 +27,13 @@ NonFatalCheckError::NonFatalCheckError(std::string_view msg, std::string_view fi { } +bool g_detail_test_only_CheckFailuresAreExceptionsNotAborts{false}; + void assertion_fail(std::string_view file, int line, std::string_view func, std::string_view assertion) { + if (g_detail_test_only_CheckFailuresAreExceptionsNotAborts) { + throw NonFatalCheckError{assertion, file, line, func}; + } auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion); fwrite(str.data(), 1, str.size(), stderr); std::abort(); diff --git a/src/util/check.h b/src/util/check.h index 198e9e078b2..6b4ec11f1c8 100644 --- a/src/util/check.h +++ b/src/util/check.h @@ -46,6 +46,12 @@ inline bool EnableFuzzDeterminism() } } +extern bool g_detail_test_only_CheckFailuresAreExceptionsNotAborts; +struct test_only_CheckFailuresAreExceptionsNotAborts { + test_only_CheckFailuresAreExceptionsNotAborts() { g_detail_test_only_CheckFailuresAreExceptionsNotAborts = true; }; + ~test_only_CheckFailuresAreExceptionsNotAborts() { g_detail_test_only_CheckFailuresAreExceptionsNotAborts = false; }; +}; + std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func); class NonFatalCheckError : public std::runtime_error From fa37153288ca420420636046ef6b8c4ba7e5a478 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 22 May 2025 16:24:45 +0200 Subject: [PATCH 3/3] util: Abort on failing CHECK_NONFATAL in debug builds This requires adjusting some tests to force exceptions over aborts, or accept either exceptions or aborts. Also, remove a fuzz test in integer.cpp that is mostly redundant with the unit test added in the prior commit. --- src/test/fuzz/integer.cpp | 5 ----- src/test/fuzz/rpc.cpp | 6 ++++++ src/test/rpc_tests.cpp | 29 ++++++++++++++++------------- src/util/check.h | 9 ++++++--- test/functional/rpc_misc.py | 24 +++++++++++++++++++----- 5 files changed, 47 insertions(+), 26 deletions(-) diff --git a/src/test/fuzz/integer.cpp b/src/test/fuzz/integer.cpp index a6729155d1f..f40a2be1e85 100644 --- a/src/test/fuzz/integer.cpp +++ b/src/test/fuzz/integer.cpp @@ -255,9 +255,4 @@ FUZZ_TARGET(integer, .init = initialize_integer) } catch (const std::ios_base::failure&) { } } - - try { - CHECK_NONFATAL(b); - } catch (const NonFatalCheckError&) { - } } diff --git a/src/test/fuzz/rpc.cpp b/src/test/fuzz/rpc.cpp index 580a6338a84..c8f5e87adfa 100644 --- a/src/test/fuzz/rpc.cpp +++ b/src/test/fuzz/rpc.cpp @@ -381,6 +381,12 @@ FUZZ_TARGET(rpc, .init = initialize_rpc) arguments.push_back(ConsumeRPCArgument(fuzzed_data_provider, good_data)); } try { + std::optional maybe_mock{}; + if (rpc_command == "echo") { + // Avoid aborting fuzzing for this specific test-only RPC with an + // intentional trigger_internal_bug + maybe_mock.emplace(); + } rpc_testing_setup->CallRPC(rpc_command, arguments); } catch (const UniValue& json_rpc_error) { const std::string error_msg{json_rpc_error.find_value("message").get_str()}; diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 87303d74174..5d81b4287dd 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -534,20 +534,23 @@ BOOST_AUTO_TEST_CASE(check_dup_param_names) make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED}}); make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED_ONLY}}); - // Error if parameters names are duplicates, unless one parameter is - // positional and the other is named and .also_positional is true. - BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", POSITIONAL}}), NonFatalCheckError); - make_rpc({{"p1", POSITIONAL}, {"p1", NAMED}}); - BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", NAMED_ONLY}}), NonFatalCheckError); - make_rpc({{"p1", NAMED}, {"p1", POSITIONAL}}); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED}}), NonFatalCheckError); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED_ONLY}}), NonFatalCheckError); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", POSITIONAL}}), NonFatalCheckError); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED}}), NonFatalCheckError); - BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + { + test_only_CheckFailuresAreExceptionsNotAborts mock_checks{}; + // Error if parameter names are duplicates, unless one parameter is + // positional and the other is named and .also_positional is true. + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", POSITIONAL}}), NonFatalCheckError); + make_rpc({{"p1", POSITIONAL}, {"p1", NAMED}}); + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + make_rpc({{"p1", NAMED}, {"p1", POSITIONAL}}); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED_ONLY}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", POSITIONAL}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED}}), NonFatalCheckError); + BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED_ONLY}}), NonFatalCheckError); - // Make sure duplicate aliases are detected too. - BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p2|p1", NAMED_ONLY}}), NonFatalCheckError); + // Make sure duplicate aliases are detected too. + BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p2|p1", NAMED_ONLY}}), NonFatalCheckError); + } } BOOST_AUTO_TEST_CASE(help_example) diff --git a/src/util/check.h b/src/util/check.h index 6b4ec11f1c8..17318e6525a 100644 --- a/src/util/check.h +++ b/src/util/check.h @@ -60,11 +60,17 @@ public: NonFatalCheckError(std::string_view msg, std::string_view file, int line, std::string_view func); }; +/** Internal helper */ +void assertion_fail(std::string_view file, int line, std::string_view func, std::string_view assertion); + /** Helper for CHECK_NONFATAL() */ template T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const char* file, int line, const char* func, const char* assertion) { if (!val) { + if constexpr (G_ABORT_ON_FAILED_ASSUME) { + assertion_fail(file, line, func, assertion); + } throw NonFatalCheckError{assertion, file, line, func}; } return std::forward(val); @@ -74,9 +80,6 @@ T&& inline_check_non_fatal(LIFETIMEBOUND T&& val, const char* file, int line, co #error "Cannot compile without assertions!" #endif -/** Helper for Assert() */ -void assertion_fail(std::string_view file, int line, std::string_view func, std::string_view assertion); - /** Helper for Assert()/Assume() */ template constexpr T&& inline_assertion_check(LIFETIMEBOUND T&& val, [[maybe_unused]] const char* file, [[maybe_unused]] int line, [[maybe_unused]] const char* func, [[maybe_unused]] const char* assertion) diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py index 3c7cce2f168..fe9f20b6ae5 100755 --- a/test/functional/rpc_misc.py +++ b/test/functional/rpc_misc.py @@ -15,6 +15,9 @@ from test_framework.util import ( from test_framework.authproxy import JSONRPCException +import http +import subprocess + class RpcMiscTest(BitcoinTestFramework): def set_test_params(self): @@ -24,11 +27,22 @@ class RpcMiscTest(BitcoinTestFramework): node = self.nodes[0] self.log.info("test CHECK_NONFATAL") - assert_raises_rpc_error( - -1, - 'Internal bug detected: request.params[9].get_str() != "trigger_internal_bug"', - lambda: node.echo(arg9='trigger_internal_bug'), - ) + msg_internal_bug = 'request.params[9].get_str() != "trigger_internal_bug"' + self.restart_node(0) # Required to flush the chainstate + try: + node.echo(arg9="trigger_internal_bug") + assert False # Must hit one of the exceptions below + except ( + subprocess.CalledProcessError, + http.client.CannotSendRequest, + http.client.RemoteDisconnected, + ): + self.log.info("Restart node after crash") + assert_equal(-6, node.process.wait(timeout=10)) + self.start_node(0) + except JSONRPCException as e: + assert_equal(e.error["code"], -1) + assert f"Internal bug detected: {msg_internal_bug}" in e.error["message"] self.log.info("test getmemoryinfo") memory = node.getmemoryinfo()['locked']