← Watch feed
test: scope cs_args locks to avoid recursive locking
Public commit record
100/100 · Strong
What the developer wrote
Authored by w0xlt
test: scope cs_args locks to avoid recursive locking
Restructure argsman tests to use scoped LOCK(cs_args) blocks only
around direct protected member access (m_settings, m_network), keeping
public method calls outside lock scopes. This avoids recursive lock
acquisitions that would deadlock with a non-recursive Mutex.
- util_ParseParameters: scope locks around m_settings checks
- util_GetBoolArg: scope lock around m_settings size check
- util_ReadConfigStream: scope lock around m_settings checks
- util_GetArg: scope lock around m_settings writes
- util_ArgsMerge: use SelectConfigNetwork() instead of m_network
- util_ChainMerge: remove unnecessary lock
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Uses a recognizable type or scope✓ Provides detailed explanatory context✓ Explains rationale or failure mode✓ Mentions testing or verification
What changed, and why it matters
This commit only changes Bitcoin Core's internal unit tests. It restructures how tests lock a shared mutex (cs_args) so that public methods are called without already holding the lock, preventing recursive lock attempts that would deadlock if the mutex were made non-recursive in the future. There is no change to production code, no user-facing behavior change, and no security vulnerability being fixed.
Recommended action
No security action required. Treat as normal test maintenance. If reviewing the broader PR, verify the companion production commit that changes cs_args to a non-recursive mutex, but this commit alone is not security-relevant.
Security signals we found
01
No production code modified
02
Test-only refactoring
03
No vulnerability class present in diff
04
No input parsing or cryptographic changes
05
No privilege boundary crossed
Evidence from the diff
The patch modifies src/test/argsman_tests.cpp to replace direct LOCK(testArgs.cs_args) blocks around entire test bodies with scoped LockSettings() callbacks that only protect direct member access to m_settings. Public ArgsManager methods (ParseParameters, SetupArgs, IsArgSet, GetArgs, GetArg, ReadConfigStream, SelectConfigNetwork, AddArg) are now invoked outside the lock. This avoids recursive acquisition of cs_args because those public methods internally lock cs_args themselves. The change also removes the exposed m_network using-declaration and uses SelectConfigNetwork() instead. It is purely a test-code refactoring in preparation for a potential future non-recursive mutex; it does not alter the locking semantics of production code.
Changed components
src/test/argsman_tests.cppInspect captured patch +63 / −59
diff --git a/src/test/argsman_tests.cpp b/src/test/argsman_tests.cpp
index e91ae5e9..3500bc5c 100644
--- a/src/test/argsman_tests.cpp
+++ b/src/test/argsman_tests.cpp
@@ -78,7 +78,6 @@ struct TestArgsManager : public ArgsManager
using ArgsManager::GetSettingsList;
using ArgsManager::ReadConfigStream;
using ArgsManager::cs_args;
- using ArgsManager::m_network;
using ArgsManager::m_settings;
};
@@ -217,29 +216,34 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters)
const char *argv_test[] = {"-ignored", "-a", "-b", "-ccc=argument", "-ccc=multiple", "f", "-d=e"};
std::string error;
- LOCK(testArgs.cs_args);
testArgs.SetupArgs({a, b, ccc, d});
BOOST_CHECK(testArgs.ParseParameters(0, argv_test, error));
- BOOST_CHECK(testArgs.m_settings.command_line_options.empty() && testArgs.m_settings.ro_config.empty());
+ testArgs.LockSettings([&](const common::Settings& s) {
+ BOOST_CHECK(s.command_line_options.empty() && s.ro_config.empty());
+ });
BOOST_CHECK(testArgs.ParseParameters(1, argv_test, error));
- BOOST_CHECK(testArgs.m_settings.command_line_options.empty() && testArgs.m_settings.ro_config.empty());
+ testArgs.LockSettings([&](const common::Settings& s) {
+ BOOST_CHECK(s.command_line_options.empty() && s.ro_config.empty());
+ });
BOOST_CHECK(testArgs.ParseParameters(7, argv_test, error));
// expectation: -ignored is ignored (program name argument),
// -a, -b and -ccc end up in map, -d ignored because it is after
// a non-option argument (non-GNU option parsing)
- BOOST_CHECK(testArgs.m_settings.command_line_options.size() == 3 && testArgs.m_settings.ro_config.empty());
BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.IsArgSet("-ccc")
&& !testArgs.IsArgSet("f") && !testArgs.IsArgSet("-d"));
- BOOST_CHECK(testArgs.m_settings.command_line_options.contains("a") && testArgs.m_settings.command_line_options.contains("b") && testArgs.m_settings.command_line_options.contains("ccc")
- && !testArgs.m_settings.command_line_options.contains("f") && !testArgs.m_settings.command_line_options.contains("d"));
-
- BOOST_CHECK(testArgs.m_settings.command_line_options["a"].size() == 1);
- BOOST_CHECK(testArgs.m_settings.command_line_options["a"].front().get_str() == "");
- BOOST_CHECK(testArgs.m_settings.command_line_options["ccc"].size() == 2);
- BOOST_CHECK(testArgs.m_settings.command_line_options["ccc"].front().get_str() == "argument");
- BOOST_CHECK(testArgs.m_settings.command_line_options["ccc"].back().get_str() == "multiple");
+ testArgs.LockSettings([&](const common::Settings& s) {
+ BOOST_CHECK(s.command_line_options.size() == 3 && s.ro_config.empty());
+ BOOST_CHECK(s.command_line_options.contains("a") && s.command_line_options.contains("b") && s.command_line_options.contains("ccc")
+ && !s.command_line_options.contains("f") && !s.command_line_options.contains("d"));
+
+ BOOST_CHECK(s.command_line_options.at("a").size() == 1);
+ BOOST_CHECK(s.command_line_options.at("a").front().get_str() == "");
+ BOOST_CHECK(s.command_line_options.at("ccc").size() == 2);
+ BOOST_CHECK(s.command_line_options.at("ccc").front().get_str() == "argument");
+ BOOST_CHECK(s.command_line_options.at("ccc").back().get_str() == "multiple");
+ });
BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2);
}
@@ -331,7 +335,6 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg)
const char *argv_test[] = {
"ignored", "-a", "-nob", "-c=0", "-d=1", "-e=false", "-f=true"};
std::string error;
- LOCK(testArgs.cs_args);
testArgs.SetupArgs({a, b, c, d, e, f});
BOOST_CHECK(testArgs.ParseParameters(7, argv_test, error));
@@ -340,8 +343,9 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg)
BOOST_CHECK(testArgs.IsArgSet({'-', opt}) || !opt);
// Nothing else should be in the map
- BOOST_CHECK(testArgs.m_settings.command_line_options.size() == 6 &&
- testArgs.m_settings.ro_config.empty());
+ testArgs.LockSettings([&](const common::Settings& s) {
+ BOOST_CHECK(s.command_line_options.size() == 6 && s.ro_config.empty());
+ });
// The -no prefix should get stripped on the way in.
BOOST_CHECK(!testArgs.IsArgSet("-nob"));
@@ -437,7 +441,6 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
"iii=2\n";
TestArgsManager test_args;
- LOCK(test_args.cs_args);
const auto a = std::make_pair("-a", ArgsManager::ALLOW_ANY);
const auto b = std::make_pair("-b", ArgsManager::ALLOW_ANY);
const auto ccc = std::make_pair("-ccc", ArgsManager::ALLOW_ANY);
@@ -454,24 +457,26 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
// expectation: a, b, ccc, d, fff, ggg, h, i end up in map
// so do sec1.ccc, sec1.d, sec1.h, sec2.ccc, sec2.iii
- BOOST_CHECK(test_args.m_settings.command_line_options.empty());
- BOOST_CHECK(test_args.m_settings.ro_config.size() == 3);
- BOOST_CHECK(test_args.m_settings.ro_config[""].size() == 8);
- BOOST_CHECK(test_args.m_settings.ro_config["sec1"].size() == 3);
- BOOST_CHECK(test_args.m_settings.ro_config["sec2"].size() == 2);
-
- BOOST_CHECK(test_args.m_settings.ro_config[""].contains("a"));
- BOOST_CHECK(test_args.m_settings.ro_config[""].contains("b"));
- BOOST_CHECK(test_args.m_settings.ro_config[""].contains("ccc"));
- BOOST_CHECK(test_args.m_settings.ro_config[""].contains("d"));
- BOOST_CHECK(test_args.m_settings.ro_config[""].contains("fff"));
- BOOST_CHECK(test_args.m_settings.ro_config[""].contains("ggg"));
- BOOST_CHECK(test_args.m_settings.ro_config[""].contains("h"));
- BOOST_CHECK(test_args.m_settings.ro_config[""].contains("i"));
- BOOST_CHECK(test_args.m_settings.ro_config["sec1"].contains("ccc"));
- BOOST_CHECK(test_args.m_settings.ro_config["sec1"].contains("h"));
- BOOST_CHECK(test_args.m_settings.ro_config["sec2"].contains("ccc"));
- BOOST_CHECK(test_args.m_settings.ro_config["sec2"].contains("iii"));
+ test_args.LockSettings([&](const common::Settings& s) {
+ BOOST_CHECK(s.command_line_options.empty());
+ BOOST_CHECK(s.ro_config.size() == 3);
+ BOOST_CHECK(s.ro_config.at("").size() == 8);
+ BOOST_CHECK(s.ro_config.at("sec1").size() == 3);
+ BOOST_CHECK(s.ro_config.at("sec2").size() == 2);
+
+ BOOST_CHECK(s.ro_config.at("").contains("a"));
+ BOOST_CHECK(s.ro_config.at("").contains("b"));
+ BOOST_CHECK(s.ro_config.at("").contains("ccc"));
+ BOOST_CHECK(s.ro_config.at("").contains("d"));
+ BOOST_CHECK(s.ro_config.at("").contains("fff"));
+ BOOST_CHECK(s.ro_config.at("").contains("ggg"));
+ BOOST_CHECK(s.ro_config.at("").contains("h"));
+ BOOST_CHECK(s.ro_config.at("").contains("i"));
+ BOOST_CHECK(s.ro_config.at("sec1").contains("ccc"));
+ BOOST_CHECK(s.ro_config.at("sec1").contains("h"));
+ BOOST_CHECK(s.ro_config.at("sec2").contains("ccc"));
+ BOOST_CHECK(s.ro_config.at("sec2").contains("iii"));
+ });
BOOST_CHECK(test_args.IsArgSet("-a"));
BOOST_CHECK(test_args.IsArgSet("-b"));
@@ -604,25 +609,26 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
BOOST_AUTO_TEST_CASE(util_GetArg)
{
TestArgsManager testArgs;
- LOCK(testArgs.cs_args);
- testArgs.m_settings.command_line_options.clear();
- testArgs.m_settings.command_line_options["strtest1"] = {"string..."};
- // strtest2 undefined on purpose
- testArgs.m_settings.command_line_options["inttest1"] = {"12345"};
- testArgs.m_settings.command_line_options["inttest2"] = {"81985529216486895"};
- // inttest3 undefined on purpose
- testArgs.m_settings.command_line_options["booltest1"] = {""};
- // booltest2 undefined on purpose
- testArgs.m_settings.command_line_options["booltest3"] = {"0"};
- testArgs.m_settings.command_line_options["booltest4"] = {"1"};
-
- // priorities
- testArgs.m_settings.command_line_options["pritest1"] = {"a", "b"};
- testArgs.m_settings.ro_config[""]["pritest2"] = {"a", "b"};
- testArgs.m_settings.command_line_options["pritest3"] = {"a"};
- testArgs.m_settings.ro_config[""]["pritest3"] = {"b"};
- testArgs.m_settings.command_line_options["pritest4"] = {"a","b"};
- testArgs.m_settings.ro_config[""]["pritest4"] = {"c","d"};
+ testArgs.LockSettings([&](common::Settings& s) {
+ s.command_line_options.clear();
+ s.command_line_options["strtest1"] = {"string..."};
+ // strtest2 undefined on purpose
+ s.command_line_options["inttest1"] = {"12345"};
+ s.command_line_options["inttest2"] = {"81985529216486895"};
+ // inttest3 undefined on purpose
+ s.command_line_options["booltest1"] = {""};
+ // booltest2 undefined on purpose
+ s.command_line_options["booltest3"] = {"0"};
+ s.command_line_options["booltest4"] = {"1"};
+
+ // priorities
+ s.command_line_options["pritest1"] = {"a", "b"};
+ s.ro_config[""]["pritest2"] = {"a", "b"};
+ s.command_line_options["pritest3"] = {"a"};
+ s.ro_config[""]["pritest3"] = {"b"};
+ s.command_line_options["pritest4"] = {"a","b"};
+ s.ro_config[""]["pritest4"] = {"c","d"};
+ });
BOOST_CHECK_EQUAL(testArgs.GetArg("strtest1", "default"), "string...");
BOOST_CHECK_EQUAL(testArgs.GetArg("strtest2", "default"), "default");
@@ -815,11 +821,10 @@ BOOST_FIXTURE_TEST_CASE(util_ArgsMerge, ArgsMergeTestingSetup)
ForEachMergeSetup([&](const ActionList& arg_actions, const ActionList& conf_actions, bool soft_set, bool force_set,
const std::string& section, const std::string& network, bool net_specific) {
TestArgsManager parser;
- LOCK(parser.cs_args);
std::string desc = "net=";
desc += network;
- parser.m_network = network;
+ parser.SelectConfigNetwork(network);
const std::string& name = net_specific ? "wallet" : "server";
const std::string key = "-" + name;
@@ -949,7 +954,6 @@ BOOST_FIXTURE_TEST_CASE(util_ChainMerge, ChainMergeTestingSetup)
ForEachMergeSetup([&](const ActionList& arg_actions, const ActionList& conf_actions) {
TestArgsManager parser;
- LOCK(parser.cs_args);
parser.AddArg("-regtest", "regtest", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
parser.AddArg("-testnet4", "testnet4", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
@@ -1027,14 +1031,14 @@ BOOST_AUTO_TEST_CASE(util_ReadWriteSettings)
// Test writing setting.
TestArgsManager args1;
args1.ForceSetArg("-datadir", fs::PathToString(m_path_root));
- args1.LockSettings([&](common::Settings& settings) { settings.rw_settings["name"] = "value"; });
+ args1.LockSettings([&](common::Settings& s) { s.rw_settings["name"] = "value"; });
args1.WriteSettingsFile();
// Test reading setting.
TestArgsManager args2;
args2.ForceSetArg("-datadir", fs::PathToString(m_path_root));
args2.ReadSettingsFile();
- args2.LockSettings([&](common::Settings& settings) { BOOST_CHECK_EQUAL(settings.rw_settings["name"].get_str(), "value"); });
+ args2.LockSettings([&](common::Settings& s) { BOOST_CHECK_EQUAL(s.rw_settings["name"].get_str(), "value"); });
// Test error logging, and remove previously written setting.
{
Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.