diff --git a/src/init.cpp b/src/init.cpp --- a/src/init.cpp +++ b/src/init.cpp @@ -2151,7 +2151,7 @@ // Warn about relative -datadir path. if (args.IsArgSet("-datadir") && - !fs::PathFromString(args.GetArg("-datadir", "")).is_absolute()) { + !args.GetPathArg("-datadir").is_absolute()) { LogPrintf("Warning: relative datadir option '%s' specified, which will " "be interpreted relative to the current working directory " "'%s'. This is fragile, because if bitcoin is started in the " diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp --- a/src/test/getarg_tests.cpp +++ b/src/test/getarg_tests.cpp @@ -158,6 +158,97 @@ BOOST_CHECK_EQUAL(m_local_args.GetIntArg("-bar", 11), 0); } +BOOST_AUTO_TEST_CASE(patharg) { + const auto dir = std::make_pair("-dir", ArgsManager::ALLOW_ANY); + SetupArgs({dir}); + ResetArgs(""); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), fs::path{}); + + const fs::path root_path{"/"}; + ResetArgs("-dir=/"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path); + + ResetArgs("-dir=/."); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path); + + ResetArgs("-dir=/./"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path); + + ResetArgs("-dir=/.//"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), root_path); + +#ifdef WIN32 + const fs::path win_root_path{"C:\\"}; + ResetArgs("-dir=C:\\"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path); + + ResetArgs("-dir=C:/"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path); + + ResetArgs("-dir=C:\\\\"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path); + + ResetArgs("-dir=C:\\."); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path); + + ResetArgs("-dir=C:\\.\\"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path); + + ResetArgs("-dir=C:\\.\\\\"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), win_root_path); +#endif + + const fs::path absolute_path{"/home/user/.bitcoin"}; + ResetArgs("-dir=/home/user/.bitcoin"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path); + + ResetArgs("-dir=/root/../home/user/.bitcoin"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path); + + ResetArgs("-dir=/home/./user/.bitcoin"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path); + + ResetArgs("-dir=/home/user/.bitcoin/"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path); + + ResetArgs("-dir=/home/user/.bitcoin//"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path); + + ResetArgs("-dir=/home/user/.bitcoin/."); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path); + + ResetArgs("-dir=/home/user/.bitcoin/./"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path); + + ResetArgs("-dir=/home/user/.bitcoin/.//"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), absolute_path); + + const fs::path relative_path{"user/.bitcoin"}; + ResetArgs("-dir=user/.bitcoin"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path); + + ResetArgs("-dir=somewhere/../user/.bitcoin"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path); + + ResetArgs("-dir=user/./.bitcoin"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path); + + ResetArgs("-dir=user/.bitcoin/"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path); + + ResetArgs("-dir=user/.bitcoin//"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path); + + ResetArgs("-dir=user/.bitcoin/."); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path); + + ResetArgs("-dir=user/.bitcoin/./"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path); + + ResetArgs("-dir=user/.bitcoin/.//"); + BOOST_CHECK_EQUAL(m_local_args.GetPathArg("-dir"), relative_path); +} + BOOST_AUTO_TEST_CASE(doubledash) { const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY); const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY); diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -75,12 +75,9 @@ args.ClearPathCache(); BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase()); -#ifndef WIN32 - // Windows does not consider "datadir/.//" to be a valid directory path. args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/.//"); args.ClearPathCache(); BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase()); -#endif } BOOST_AUTO_TEST_CASE(util_check) { diff --git a/src/util/system.h b/src/util/system.h --- a/src/util/system.h +++ b/src/util/system.h @@ -233,6 +233,17 @@ */ const std::list GetUnrecognizedSections() const; + /** + * Get a normalized path from a specified pathlike argument + * + * It is guaranteed that the returned path has no trailing slashes. + * + * @param pathlike_arg Pathlike argument to get a path from (e.g., + * "-datadir", "-blocksdir" or "-walletdir") + * @return Normalized path which is get from a specified pathlike argument + */ + fs::path GetPathArg(std::string pathlike_arg) const; + /** * Get blocks directory path * diff --git a/src/util/system.cpp b/src/util/system.cpp --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -244,19 +244,6 @@ return true; } -namespace { -fs::path StripRedundantLastElementsOfPath(const fs::path &path) { - auto result = path; - while (result.filename().empty() || - fs::PathToString(result.filename()) == ".") { - result = result.parent_path(); - } - - assert(fs::equivalent(result, path)); - return result; -} -} // namespace - // Define default constructor and destructor that are not inline, so code // instantiating this class doesn't need to #include class definitions for all // members. For example, m_settings has an internal dependency on univalue. @@ -405,6 +392,13 @@ return std::nullopt; } +fs::path ArgsManager::GetPathArg(std::string pathlike_arg) const { + auto result = + fs::PathFromString(GetArg(pathlike_arg, "")).lexically_normal(); + // Remove trailing slash, if present. + return result.has_filename() ? result : result.parent_path(); +} + const fs::path &ArgsManager::GetBlocksDirPath() const { LOCK(cs_args); fs::path &path = m_cached_blocks_path; @@ -416,7 +410,7 @@ } if (IsArgSet("-blocksdir")) { - path = fs::absolute(fs::PathFromString(GetArg("-blocksdir", ""))); + path = fs::absolute(GetPathArg("-blocksdir")); if (!fs::is_directory(path)) { path = ""; return path; @@ -441,10 +435,10 @@ if (!path.empty()) { return path; } - std::string datadir = GetArg("-datadir", ""); + + const fs::path datadir{GetPathArg("-datadir")}; if (!datadir.empty()) { - path = fs::absolute( - StripRedundantLastElementsOfPath(fs::PathFromString(datadir))); + path = fs::absolute(datadir); if (!fs::is_directory(path)) { path = ""; return path; @@ -464,7 +458,6 @@ } } - path = StripRedundantLastElementsOfPath(path); return path; } @@ -842,9 +835,8 @@ } bool CheckDataDirOption() { - std::string datadir = gArgs.GetArg("-datadir", ""); - return datadir.empty() || - fs::is_directory(fs::absolute(fs::PathFromString(datadir))); + const fs::path datadir{gArgs.GetPathArg("-datadir")}; + return datadir.empty() || fs::is_directory(fs::absolute(datadir)); } fs::path GetConfigFile(const std::string &confPath) { diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp --- a/src/wallet/load.cpp +++ b/src/wallet/load.cpp @@ -21,18 +21,17 @@ bool VerifyWallets(interfaces::Chain &chain) { if (gArgs.IsArgSet("-walletdir")) { - fs::path wallet_dir = - fs::PathFromString(gArgs.GetArg("-walletdir", "")); + const fs::path wallet_dir{gArgs.GetPathArg("-walletdir")}; std::error_code error; // The canonical path cleans the path, preventing >1 Berkeley // environment instances for the same directory fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error); - if (error || !fs::exists(wallet_dir)) { + if (error || !fs::exists(canonical_wallet_dir)) { chain.initError( strprintf(_("Specified -walletdir \"%s\" does not exist"), fs::PathToString(wallet_dir))); return false; - } else if (!fs::is_directory(wallet_dir)) { + } else if (!fs::is_directory(canonical_wallet_dir)) { chain.initError( strprintf(_("Specified -walletdir \"%s\" is not a directory"), fs::PathToString(wallet_dir))); diff --git a/src/wallet/test/init_tests.cpp b/src/wallet/test/init_tests.cpp --- a/src/wallet/test/init_tests.cpp +++ b/src/wallet/test/init_tests.cpp @@ -17,7 +17,7 @@ SetWalletDir(m_walletdir_path_cases["default"]); bool result = m_wallet_client->verify(); BOOST_CHECK(result == true); - fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", "")); + fs::path walletdir = gArgs.GetPathArg("-walletdir"); fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]); BOOST_CHECK(walletdir == expected_path); } @@ -26,7 +26,7 @@ SetWalletDir(m_walletdir_path_cases["custom"]); bool result = m_wallet_client->verify(); BOOST_CHECK(result == true); - fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", "")); + fs::path walletdir = gArgs.GetPathArg("-walletdir"); fs::path expected_path = fs::canonical(m_walletdir_path_cases["custom"]); BOOST_CHECK(walletdir == expected_path); } @@ -58,14 +58,11 @@ } } -#ifndef WIN32 -// Windows does not consider "datadir/wallets//" to be a valid directory path. -// FIXME: enable these tests again ASAP (D13730 & core#24251) BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing) { SetWalletDir(m_walletdir_path_cases["trailing"]); bool result = m_wallet_client->verify(); BOOST_CHECK(result == true); - fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", "")); + fs::path walletdir = gArgs.GetPathArg("-walletdir"); fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]); BOOST_CHECK(walletdir == expected_path); } @@ -74,10 +71,9 @@ SetWalletDir(m_walletdir_path_cases["trailing2"]); bool result = m_wallet_client->verify(); BOOST_CHECK(result == true); - fs::path walletdir = fs::PathFromString(gArgs.GetArg("-walletdir", "")); + fs::path walletdir = gArgs.GetPathArg("-walletdir"); fs::path expected_path = fs::canonical(m_walletdir_path_cases["default"]); BOOST_CHECK(walletdir == expected_path); } -#endif BOOST_AUTO_TEST_SUITE_END() diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp --- a/src/wallet/walletutil.cpp +++ b/src/wallet/walletutil.cpp @@ -14,7 +14,7 @@ fs::path path; if (gArgs.IsArgSet("-walletdir")) { - path = fs::PathFromString(gArgs.GetArg("-walletdir", "")); + path = gArgs.GetPathArg("-walletdir"); if (!fs::is_directory(path)) { // If the path specified doesn't exist, we return the deliberately // invalid empty string.