From fa37153288ca420420636046ef6b8c4ba7e5a478 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 22 May 2025 16:24:45 +0200 Subject: [PATCH] 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']