← Watch feed
args: eliminate all recursive locking of cs_args
Public commit record
68/100 · Adequate
What the developer wrote
Authored by w0xlt
args: eliminate all recursive locking of cs_args
In methods that already hold cs_args, replace public method calls with
their lock-held private counterparts:
- ParseParameters: GetArgFlags() -> GetArgFlags_()
- GetBlocksDirPath: IsArgSet()/GetPathArg()/GetDataDirBase() ->
GetSetting_()/GetPathArg_()/GetDataDir()
- ReadSettingsFile: GetArgFlags() -> GetArgFlags_()
- SoftSetArg: IsArgSet()/ForceSetArg() -> GetSetting_()/direct write
- CheckMultipleCLIArgs: IsArgSet() -> GetSetting_()
- logArgsPrefix: GetArgFlags() -> GetArgFlags_()
- ReadConfigStream: GetArgFlags() -> GetArgFlags_()
- ReadConfigFiles: GetPathArg() -> GetPathArg_()
No behavior change. This eliminates all recursive lock acquisitions,
preparing for the conversion of cs_args from RecursiveMutex to Mutex.
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This is a straightforward internal code cleanup in Bitcoin Core. It replaces public method calls with private equivalents inside functions that already hold a lock, so the same lock is not acquired twice. The commit explicitly states there is no behavior change, and the diff supports that: every change is a one-for-one replacement of a locked wrapper with its already-locked internal version. There is no security bug being fixed here.
Recommended action
No security action required. Treat as normal code-quality/refactoring change.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The commit refactors ArgsManager to avoid recursive locking of cs_args by calling lock-held private helpers (GetArgFlags_, GetSetting_, GetPathArg_, GetDataDir) from functions that already hold cs_args. This is preparation for converting cs_args from RecursiveMutex to Mutex. The replacements are semantically equivalent: GetArgFlags_ returns the same flags lookup, GetSetting_ returns the same setting lookup, GetPathArg_ returns the same path resolution, and the GetDataDir call is equivalent to GetDataDirBase. No logic, validation, or trust boundary changes are introduced.
Changed components
src/common/args.cppsrc/common/config.cppInspect captured patch +13 / −12
diff --git a/src/common/args.cpp b/src/common/args.cpp
index a460c0bd..be8c5133 100644
--- a/src/common/args.cpp
+++ b/src/common/args.cpp
@@ -206,7 +206,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
if (key[0] != '-') {
if (!m_accept_any_command && m_command.empty()) {
// The first non-dash arg is a registered command
- std::optional<unsigned int> flags = GetArgFlags(key);
+ std::optional<unsigned int> flags = GetArgFlags_(key);
if (!flags || !(*flags & ArgsManager::COMMAND)) {
error = strprintf("Invalid command '%s'", argv[i]);
return false;
@@ -227,7 +227,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
// Transform -foo to foo
key.erase(0, 1);
KeyInfo keyinfo = InterpretKey(key);
- std::optional<unsigned int> flags = GetArgFlags('-' + keyinfo.name);
+ std::optional<unsigned int> flags = GetArgFlags_('-' + keyinfo.name);
// Unknown command line options and command line options with dot
// characters (which are returned from InterpretKey with nonempty
@@ -306,14 +306,14 @@ fs::path ArgsManager::GetBlocksDirPath() const
// this function
if (!path.empty()) return path;
- if (IsArgSet("-blocksdir")) {
- path = fs::absolute(GetPathArg("-blocksdir"));
+ if (!GetSetting_("-blocksdir").isNull()) {
+ path = fs::absolute(GetPathArg_("-blocksdir"));
if (!fs::is_directory(path)) {
path = "";
return path;
}
} else {
- path = GetDataDirBase();
+ path = GetDataDir(/*net_specific=*/false);
}
path /= fs::PathFromString(BaseParams().DataDir());
@@ -443,7 +443,7 @@ bool ArgsManager::ReadSettingsFile(std::vector<std::string>* errors)
}
for (const auto& setting : m_settings.rw_settings) {
KeyInfo key = InterpretKey(setting.first); // Split setting key into section and argname
- if (!GetArgFlags('-' + key.name)) {
+ if (!GetArgFlags_('-' + key.name)) {
LogWarning("Ignoring unknown rw_settings value %s", setting.first);
}
}
@@ -579,8 +579,8 @@ INSTANTIATE_INT_TYPE(uint64_t);
bool ArgsManager::SoftSetArg(const std::string& strArg, const std::string& strValue)
{
LOCK(cs_args);
- if (IsArgSet(strArg)) return false;
- ForceSetArg(strArg, strValue);
+ if (!GetSetting_(strArg).isNull()) return false;
+ m_settings.forced_settings[SettingName(strArg)] = strValue;
return true;
}
@@ -653,7 +653,7 @@ void ArgsManager::CheckMultipleCLIArgs() const
auto cmds = m_available_args.find(OptionsCategory::CLI_COMMANDS);
if (cmds != m_available_args.end()) {
for (const auto& [cmd, argspec] : cmds->second) {
- if (IsArgSet(cmd)) {
+ if (!GetSetting_(cmd).isNull()) {
found.push_back(cmd);
}
}
@@ -910,7 +910,7 @@ void ArgsManager::logArgsPrefix(
std::string section_str = section.empty() ? "" : "[" + section + "] ";
for (const auto& arg : args) {
for (const auto& value : arg.second) {
- std::optional<unsigned int> flags = GetArgFlags('-' + arg.first);
+ std::optional<unsigned int> flags = GetArgFlags_('-' + arg.first);
if (flags) {
std::string value_str = (*flags & SENSITIVE) ? "****" : value.write();
LogInfo("%s %s%s=%s\n", prefix, section_str, arg.first, value_str);
diff --git a/src/common/config.cpp b/src/common/config.cpp
index a2cf9381..85bea674 100644
--- a/src/common/config.cpp
+++ b/src/common/config.cpp
@@ -99,7 +99,7 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file
}
for (const std::pair<std::string, std::string>& option : options) {
KeyInfo key = InterpretKey(option.first);
- std::optional<unsigned int> flags = GetArgFlags('-' + key.name);
+ std::optional<unsigned int> flags = GetArgFlags_('-' + key.name);
if (!IsConfSupported(key, error)) return false;
if (flags) {
std::optional<common::SettingsValue> value = InterpretValue(key, &option.second, *flags, error);
@@ -125,7 +125,8 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
LOCK(cs_args);
m_settings.ro_config.clear();
m_config_sections.clear();
- m_config_path = AbsPathForConfigVal(*this, GetPathArg("-conf", BITCOIN_CONF_FILENAME), /*net_specific=*/false);
+ const auto conf_val = GetPathArg_("-conf", BITCOIN_CONF_FILENAME);
+ m_config_path = (conf_val.is_absolute() || conf_val.empty()) ? conf_val : fsbridge::AbsPathJoin(GetDataDir(/*net_specific=*/false), conf_val);
}
const auto conf_path{GetConfigFilePath()};
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.