Merge bitcoin/bitcoin#24470: Disallow more unsafe string->path conversions allowed by path append operators

f64aa9c411ad78259756a28756ec1eb8069b5ab4 Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky)

Pull request description:

  Add more `fs::path` `operator/` and `operator+` overloads to prevent unsafe string->path conversions on Windows that would cause strings to be decoded according to the current Windows locale & code page instead of the correct string encoding.

  Update application code to deal with loss of implicit string->path conversions by calling `fs::u8path` or `fs::PathFromString` explicitly, or by just changing variable types from `std::string` to `fs::path` to avoid conversions altogether, or make them happen earlier.

  In all cases, there's no change in behavior either (1) because strings only contained ASCII characters and would be decoded the same regardless of what encoding was used, or (2) because of the 1:1 mapping between paths and strings using the `PathToString` and `PathFromString` functions.

  Motivation for this PR was just that I was experimenting with #24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them.

ACKs for top commit:
  hebasto:
    ACK f64aa9c411ad78259756a28756ec1eb8069b5ab4

Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f

3 files changed, 12 insertions, 13 deletions

diff --git a/src/wallet/test/db_tests.cpp b/src/wallet/test/db_tests.cpp
index fbf1e0efd3..f61808c549 100644
--- a/src/wallet/test/db_tests.cpp
+++ b/src/wallet/test/db_tests.cpp
@@ -15,22 +15,22 @@
 namespace wallet {
 BOOST_FIXTURE_TEST_SUITE(db_tests, BasicTestingSetup)
 
-static std::shared_ptr<BerkeleyEnvironment> GetWalletEnv(const fs::path& path, std::string& database_filename)
+static std::shared_ptr<BerkeleyEnvironment> GetWalletEnv(const fs::path& path, fs::path& database_filename)
 {
     fs::path data_file = BDBDataFile(path);
-    database_filename = fs::PathToString(data_file.filename());
+    database_filename = data_file.filename();
     return GetBerkeleyEnv(data_file.parent_path(), false);
 }
 
 BOOST_AUTO_TEST_CASE(getwalletenv_file)
 {
-    std::string test_name = "test_name.dat";
+    fs::path test_name = "test_name.dat";
     const fs::path datadir = m_args.GetDataDirNet();
     fs::path file_path = datadir / test_name;
     std::ofstream f{file_path};
     f.close();
 
-    std::string filename;
+    fs::path filename;
     std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);
     BOOST_CHECK_EQUAL(filename, test_name);
     BOOST_CHECK_EQUAL(env->Directory(), datadir);
@@ -38,10 +38,10 @@ BOOST_AUTO_TEST_CASE(getwalletenv_file)
 
 BOOST_AUTO_TEST_CASE(getwalletenv_directory)
 {
-    std::string expected_name = "wallet.dat";
+    fs::path expected_name = "wallet.dat";
     const fs::path datadir = m_args.GetDataDirNet();
 
-    std::string filename;
+    fs::path filename;
     std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(datadir, filename);
     BOOST_CHECK_EQUAL(filename, expected_name);
     BOOST_CHECK_EQUAL(env->Directory(), datadir);
@@ -51,7 +51,7 @@ BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_multiple)
 {
     fs::path datadir = m_args.GetDataDirNet() / "1";
     fs::path datadir_2 = m_args.GetDataDirNet() / "2";
-    std::string filename;
+    fs::path filename;
 
     std::shared_ptr<BerkeleyEnvironment> env_1 = GetWalletEnv(datadir, filename);
     std::shared_ptr<BerkeleyEnvironment> env_2 = GetWalletEnv(datadir, filename);
@@ -65,7 +65,7 @@ BOOST_AUTO_TEST_CASE(getwalletenv_g_dbenvs_free_instance)
 {
     fs::path datadir = gArgs.GetDataDirNet() / "1";
     fs::path datadir_2 = gArgs.GetDataDirNet() / "2";
-    std::string filename;
+    fs::path filename;
 
     std::shared_ptr <BerkeleyEnvironment> env_1_a = GetWalletEnv(datadir, filename);
     std::shared_ptr <BerkeleyEnvironment> env_2_a = GetWalletEnv(datadir_2, filename);
diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp
index 1c16da25bd..9089c8ff46 100644
--- a/src/wallet/test/fuzz/notifications.cpp
+++ b/src/wallet/test/fuzz/notifications.cpp
@@ -64,7 +64,7 @@ struct FuzzedWallet {
         assert(RemoveWallet(context, wallet, load_on_start, warnings));
         assert(warnings.empty());
         UnloadWallet(std::move(wallet));
-        fs::remove_all(GetWalletDir() / name);
+        fs::remove_all(GetWalletDir() / fs::PathFromString(name));
     }
     CScript GetScriptPubKey(FuzzedDataProvider& fuzzed_data_provider)
     {
diff --git a/src/wallet/test/init_test_fixture.cpp b/src/wallet/test/init_test_fixture.cpp
index 34c22a9c0d..8eb7689c94 100644
--- a/src/wallet/test/init_test_fixture.cpp
+++ b/src/wallet/test/init_test_fixture.cpp
@@ -17,8 +17,7 @@ InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainNam
 {
     m_wallet_loader = MakeWalletLoader(*m_node.chain, m_args);
 
-    std::string sep;
-    sep += fs::path::preferred_separator;
+    const auto sep = fs::path::preferred_separator;
 
     m_datadir = m_args.GetDataDirNet();
     m_cwd = fs::current_path();
@@ -27,8 +26,8 @@ InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainNam
     m_walletdir_path_cases["custom"] = m_datadir / "my_wallets";
     m_walletdir_path_cases["nonexistent"] = m_datadir / "path_does_not_exist";
     m_walletdir_path_cases["file"] = m_datadir / "not_a_directory.dat";
-    m_walletdir_path_cases["trailing"] = m_datadir / ("wallets" + sep);
-    m_walletdir_path_cases["trailing2"] = m_datadir / ("wallets" + sep + sep);
+    m_walletdir_path_cases["trailing"] = (m_datadir / "wallets") + sep;
+    m_walletdir_path_cases["trailing2"] = (m_datadir / "wallets") + sep + sep;
 
     fs::current_path(m_datadir);
     m_walletdir_path_cases["relative"] = "wallets";