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

18 files changed, 83 insertions, 59 deletions

diff --git a/src/addrdb.cpp b/src/addrdb.cpp
index 0a76f83150..b9eae292d7 100644
--- a/src/addrdb.cpp
+++ b/src/addrdb.cpp
@@ -54,7 +54,7 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
     std::string tmpfn = strprintf("%s.%04x", prefix, randv);
 
     // open temp output file, and associate with CAutoFile
-    fs::path pathTmp = gArgs.GetDataDirNet() / tmpfn;
+    fs::path pathTmp = gArgs.GetDataDirNet() / fs::u8path(tmpfn);
     FILE *file = fsbridge::fopen(pathTmp, "wb");
     CAutoFile fileout(file, SER_DISK, version);
     if (fileout.IsNull()) {
diff --git a/src/flatfile.cpp b/src/flatfile.cpp
index d6cada0c46..0fecf4f504 100644
--- a/src/flatfile.cpp
+++ b/src/flatfile.cpp
@@ -27,7 +27,7 @@ std::string FlatFilePos::ToString() const
 
 fs::path FlatFileSeq::FileName(const FlatFilePos& pos) const
 {
-    return m_dir / strprintf("%s%05u.dat", m_prefix, pos.nFile);
+    return m_dir / fs::u8path(strprintf("%s%05u.dat", m_prefix, pos.nFile));
 }
 
 FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only)
diff --git a/src/fs.h b/src/fs.h
index 8e145cbb8e..cc55793b95 100644
--- a/src/fs.h
+++ b/src/fs.h
@@ -92,11 +92,30 @@ static inline auto quoted(const std::string& s)
 }
 
 // Allow safe path append operations.
-static inline path operator+(path p1, path p2)
+static inline path operator/(path p1, path p2)
 {
-    p1 += std::move(p2);
+    p1 /= std::move(p2);
     return p1;
 }
+static inline path operator/(path p1, const char* p2)
+{
+    p1 /= p2;
+    return p1;
+}
+static inline path operator+(path p1, const char* p2)
+{
+    p1 += p2;
+    return p1;
+}
+static inline path operator+(path p1, path::value_type p2)
+{
+    p1 += p2;
+    return p1;
+}
+
+// Disallow unsafe path append operations.
+template<typename T> static inline path operator/(path p1, T p2) = delete;
+template<typename T> static inline path operator+(path p1, T p2) = delete;
 
 // Disallow implicit std::string conversion for copy_file
 // to avoid locale-dependent encoding on Windows.
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
index 4f99eddfd7..c92b8c7e19 100644
--- a/src/index/blockfilterindex.cpp
+++ b/src/index/blockfilterindex.cpp
@@ -100,7 +100,7 @@ BlockFilterIndex::BlockFilterIndex(BlockFilterType filter_type,
     const std::string& filter_name = BlockFilterTypeName(filter_type);
     if (filter_name.empty()) throw std::invalid_argument("unknown filter_type");
 
-    fs::path path = gArgs.GetDataDirNet() / "indexes" / "blockfilter" / filter_name;
+    fs::path path = gArgs.GetDataDirNet() / "indexes" / "blockfilter" / fs::u8path(filter_name);
     fs::create_directories(path);
 
     m_name = filter_name + " block filter index";
diff --git a/src/net.cpp b/src/net.cpp
index aa70bed226..90fe237418 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -3153,7 +3153,7 @@ void CaptureMessageToFile(const CAddress& addr,
     std::string clean_addr = addr.ToString();
     std::replace(clean_addr.begin(), clean_addr.end(), ':', '_');
 
-    fs::path base_path = gArgs.GetDataDirNet() / "message_capture" / clean_addr;
+    fs::path base_path = gArgs.GetDataDirNet() / "message_capture" / fs::u8path(clean_addr);
     fs::create_directories(base_path);
 
     fs::path path = base_path / (is_incoming ? "msgs_recv.dat" : "msgs_sent.dat");
diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp
index 6fb5fce5b3..706cf2f7db 100644
--- a/src/qt/guiutil.cpp
+++ b/src/qt/guiutil.cpp
@@ -508,7 +508,7 @@ fs::path static StartupShortcutPath()
         return GetSpecialFolderPath(CSIDL_STARTUP) / "Bitcoin.lnk";
     if (chain == CBaseChainParams::TESTNET) // Remove this special case when CBaseChainParams::TESTNET = "testnet4"
         return GetSpecialFolderPath(CSIDL_STARTUP) / "Bitcoin (testnet).lnk";
-    return GetSpecialFolderPath(CSIDL_STARTUP) / strprintf("Bitcoin (%s).lnk", chain);
+    return GetSpecialFolderPath(CSIDL_STARTUP) / fs::u8path(strprintf("Bitcoin (%s).lnk", chain));
 }
 
 bool GetStartOnSystemStartup()
@@ -589,7 +589,7 @@ fs::path static GetAutostartFilePath()
     std::string chain = gArgs.GetChainName();
     if (chain == CBaseChainParams::MAIN)
         return GetAutostartDir() / "bitcoin.desktop";
-    return GetAutostartDir() / strprintf("bitcoin-%s.desktop", chain);
+    return GetAutostartDir() / fs::u8path(strprintf("bitcoin-%s.desktop", chain));
 }
 
 bool GetStartOnSystemStartup()
diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h
index 09f96a033c..5ac504c24f 100644
--- a/src/test/util/chainstate.h
+++ b/src/test/util/chainstate.h
@@ -30,7 +30,7 @@ CreateAndActivateUTXOSnapshot(node::NodeContext& node, const fs::path root, F ma
     //
     int height;
     WITH_LOCK(::cs_main, height = node.chainman->ActiveHeight());
-    fs::path snapshot_path = root / tfm::format("test_snapshot.%d.dat", height);
+    fs::path snapshot_path = root / fs::u8path(tfm::format("test_snapshot.%d.dat", height));
     FILE* outfile{fsbridge::fopen(snapshot_path, "wb")};
     CAutoFile auto_outfile{outfile, SER_DISK, CLIENT_VERSION};
 
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 1ca20fd848..b880a7a9fd 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -2049,7 +2049,7 @@ BOOST_AUTO_TEST_CASE(test_ParseFixedPoint)
     BOOST_CHECK(!ParseFixedPoint("31.999999999999999999999", 3, &amount));
 }
 
-static void TestOtherThread(fs::path dirname, std::string lockname, bool *result)
+static void TestOtherThread(fs::path dirname, fs::path lockname, bool *result)
 {
     *result = LockDirectory(dirname, lockname);
 }
@@ -2059,7 +2059,7 @@ static constexpr char LockCommand = 'L';
 static constexpr char UnlockCommand = 'U';
 static constexpr char ExitCommand = 'X';
 
-[[noreturn]] static void TestOtherProcess(fs::path dirname, std::string lockname, int fd)
+[[noreturn]] static void TestOtherProcess(fs::path dirname, fs::path lockname, int fd)
 {
     char ch;
     while (true) {
@@ -2090,7 +2090,7 @@ static constexpr char ExitCommand = 'X';
 BOOST_AUTO_TEST_CASE(test_LockDirectory)
 {
     fs::path dirname = m_args.GetDataDirBase() / "lock_dir";
-    const std::string lockname = ".lock";
+    const fs::path lockname = ".lock";
 #ifndef WIN32
     // Revert SIGCHLD to default, otherwise boost.test will catch and fail on
     // it: there is BOOST_TEST_IGNORE_SIGCHLD but that only works when defined
diff --git a/src/util/getuniquepath.cpp b/src/util/getuniquepath.cpp
index 6776e7785b..1d8e511c83 100644
--- a/src/util/getuniquepath.cpp
+++ b/src/util/getuniquepath.cpp
@@ -9,6 +9,6 @@
 fs::path GetUniquePath(const fs::path& base)
 {
     FastRandomContext rnd;
-    fs::path tmpFile = base / HexStr(rnd.randbytes(8));
+    fs::path tmpFile = base / fs::u8path(HexStr(rnd.randbytes(8)));
     return tmpFile;
 }
\ No newline at end of file
diff --git a/src/util/system.cpp b/src/util/system.cpp
index f9a9ad3e20..0dee8f2a6d 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -104,7 +104,7 @@ static Mutex cs_dir_locks;
  */
 static std::map<std::string, std::unique_ptr<fsbridge::FileLock>> dir_locks GUARDED_BY(cs_dir_locks);
 
-bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only)
+bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only)
 {
     LOCK(cs_dir_locks);
     fs::path pathLockFile = directory / lockfile_name;
@@ -128,7 +128,7 @@ bool LockDirectory(const fs::path& directory, const std::string lockfile_name, b
     return true;
 }
 
-void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name)
+void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name)
 {
     LOCK(cs_dir_locks);
     dir_locks.erase(fs::PathToString(directory / lockfile_name));
diff --git a/src/util/system.h b/src/util/system.h
index a66b597d41..a7f4d16911 100644
--- a/src/util/system.h
+++ b/src/util/system.h
@@ -76,8 +76,8 @@ void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length);
  */
 [[nodiscard]] bool RenameOver(fs::path src, fs::path dest);
 
-bool LockDirectory(const fs::path& directory, const std::string lockfile_name, bool probe_only=false);
-void UnlockDirectory(const fs::path& directory, const std::string& lockfile_name);
+bool LockDirectory(const fs::path& directory, const fs::path& lockfile_name, bool probe_only=false);
+void UnlockDirectory(const fs::path& directory, const fs::path& lockfile_name);
 bool DirIsWritable(const fs::path& directory);
 bool CheckDiskSpace(const fs::path& dir, uint64_t additional_bytes = 0);
 
diff --git a/src/validation.cpp b/src/validation.cpp
index e2eb6a1f7a..5a2bee2418 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1491,7 +1491,7 @@ CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams)
 }
 
 CoinsViews::CoinsViews(
-    std::string ldb_name,
+    fs::path ldb_name,
     size_t cache_size_bytes,
     bool in_memory,
     bool should_wipe) : m_dbview(
@@ -1519,7 +1519,7 @@ void CChainState::InitCoinsDB(
     size_t cache_size_bytes,
     bool in_memory,
     bool should_wipe,
-    std::string leveldb_name)
+    fs::path leveldb_name)
 {
     if (m_from_snapshot_blockhash) {
         leveldb_name += "_" + m_from_snapshot_blockhash->ToString();
diff --git a/src/validation.h b/src/validation.h
index afc53bb445..42e41502f9 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -424,7 +424,7 @@ public:
     //! state to disk, which should not be done until the health of the database is verified.
     //!
     //! All arguments forwarded onto CCoinsViewDB.
-    CoinsViews(std::string ldb_name, size_t cache_size_bytes, bool in_memory, bool should_wipe);
+    CoinsViews(fs::path ldb_name, size_t cache_size_bytes, bool in_memory, bool should_wipe);
 
     //! Initialize the CCoinsViewCache member.
     void InitCache() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
@@ -518,7 +518,7 @@ public:
         size_t cache_size_bytes,
         bool in_memory,
         bool should_wipe,
-        std::string leveldb_name = "chainstate");
+        fs::path leveldb_name = "chainstate");
 
     //! Initialize the in-memory coins cache (to be done after the health of the on-disk database
     //! is verified).
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
index 7f709ffa3e..1aa0339445 100644
--- a/src/wallet/bdb.cpp
+++ b/src/wallet/bdb.cpp
@@ -261,7 +261,7 @@ BerkeleyBatch::SafeDbt::operator Dbt*()
 bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
 {
     fs::path walletDir = env->Directory();
-    fs::path file_path = walletDir / strFile;
+    fs::path file_path = walletDir / m_filename;
 
     LogPrintf("Using BerkeleyDB version %s\n", BerkeleyDatabaseVersion());
     LogPrintf("Using wallet %s\n", fs::PathToString(file_path));
@@ -275,6 +275,7 @@ bool BerkeleyDatabase::Verify(bilingual_str& errorStr)
         assert(m_refcount == 0);
 
         Db db(env->dbenv.get(), 0);
+        const std::string strFile = fs::PathToString(m_filename);
         int result = db.verify(strFile.c_str(), nullptr, nullptr, 0);
         if (result != 0) {
             errorStr = strprintf(_("%s corrupt. Try using the wallet tool bitcoin-wallet to salvage or restoring a backup."), fs::quoted(fs::PathToString(file_path)));
@@ -297,11 +298,11 @@ BerkeleyDatabase::~BerkeleyDatabase()
 {
     if (env) {
         LOCK(cs_db);
-        env->CloseDb(strFile);
+        env->CloseDb(m_filename);
         assert(!m_db);
-        size_t erased = env->m_databases.erase(strFile);
+        size_t erased = env->m_databases.erase(m_filename);
         assert(erased == 1);
-        env->m_fileids.erase(strFile);
+        env->m_fileids.erase(fs::PathToString(m_filename));
     }
 }
 
@@ -313,7 +314,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, b
     fFlushOnClose = fFlushOnCloseIn;
     env = database.env.get();
     pdb = database.m_db.get();
-    strFile = database.strFile;
+    strFile = fs::PathToString(database.m_filename);
     if (!Exists(std::string("version"))) {
         bool fTmp = fReadOnly;
         fReadOnly = false;
@@ -335,6 +336,7 @@ void BerkeleyDatabase::Open()
         if (m_db == nullptr) {
             int ret;
             std::unique_ptr<Db> pdb_temp = std::make_unique<Db>(env->dbenv.get(), 0);
+            const std::string strFile = fs::PathToString(m_filename);
 
             bool fMockDb = env->IsMock();
             if (fMockDb) {
@@ -407,11 +409,11 @@ void BerkeleyBatch::Close()
         Flush();
 }
 
-void BerkeleyEnvironment::CloseDb(const std::string& strFile)
+void BerkeleyEnvironment::CloseDb(const fs::path& filename)
 {
     {
         LOCK(cs_db);
-        auto it = m_databases.find(strFile);
+        auto it = m_databases.find(filename);
         assert(it != m_databases.end());
         BerkeleyDatabase& database = it->second.get();
         if (database.m_db) {
@@ -434,12 +436,12 @@ void BerkeleyEnvironment::ReloadDbEnv()
         return true;
     });
 
-    std::vector<std::string> filenames;
+    std::vector<fs::path> filenames;
     for (auto it : m_databases) {
         filenames.push_back(it.first);
     }
     // Close the individual Db's
-    for (const std::string& filename : filenames) {
+    for (const fs::path& filename : filenames) {
         CloseDb(filename);
     }
     // Reset the environment
@@ -454,9 +456,10 @@ bool BerkeleyDatabase::Rewrite(const char* pszSkip)
     while (true) {
         {
             LOCK(cs_db);
+            const std::string strFile = fs::PathToString(m_filename);
             if (m_refcount <= 0) {
                 // Flush log data to the dat file
-                env->CloseDb(strFile);
+                env->CloseDb(m_filename);
                 env->CheckpointLSN(strFile);
                 m_refcount = -1;
 
@@ -508,7 +511,7 @@ bool BerkeleyDatabase::Rewrite(const char* pszSkip)
                     }
                     if (fSuccess) {
                         db.Close();
-                        env->CloseDb(strFile);
+                        env->CloseDb(m_filename);
                         if (pdbCopy->close(0))
                             fSuccess = false;
                     } else {
@@ -544,13 +547,14 @@ void BerkeleyEnvironment::Flush(bool fShutdown)
         LOCK(cs_db);
         bool no_dbs_accessed = true;
         for (auto& db_it : m_databases) {
-            std::string strFile = db_it.first;
+            const fs::path& filename = db_it.first;
             int nRefCount = db_it.second.get().m_refcount;
             if (nRefCount < 0) continue;
+            const std::string strFile = fs::PathToString(filename);
             LogPrint(BCLog::WALLETDB, "BerkeleyEnvironment::Flush: Flushing %s (refcount = %d)...\n", strFile, nRefCount);
             if (nRefCount == 0) {
                 // Move log data to the dat file
-                CloseDb(strFile);
+                CloseDb(filename);
                 LogPrint(BCLog::WALLETDB, "BerkeleyEnvironment::Flush: %s checkpoint\n", strFile);
                 dbenv->txn_checkpoint(0, 0, 0);
                 LogPrint(BCLog::WALLETDB, "BerkeleyEnvironment::Flush: %s detach\n", strFile);
@@ -590,11 +594,12 @@ bool BerkeleyDatabase::PeriodicFlush()
     // Don't flush if there haven't been any batch writes for this database.
     if (m_refcount < 0) return false;
 
+    const std::string strFile = fs::PathToString(m_filename);
     LogPrint(BCLog::WALLETDB, "Flushing %s\n", strFile);
     int64_t nStart = GetTimeMillis();
 
     // Flush wallet file so it's self contained
-    env->CloseDb(strFile);
+    env->CloseDb(m_filename);
     env->CheckpointLSN(strFile);
     m_refcount = -1;
 
@@ -605,6 +610,7 @@ bool BerkeleyDatabase::PeriodicFlush()
 
 bool BerkeleyDatabase::Backup(const std::string& strDest) const
 {
+    const std::string strFile = fs::PathToString(m_filename);
     while (true)
     {
         {
@@ -612,14 +618,14 @@ bool BerkeleyDatabase::Backup(const std::string& strDest) const
             if (m_refcount <= 0)
             {
                 // Flush log data to the dat file
-                env->CloseDb(strFile);
+                env->CloseDb(m_filename);
                 env->CheckpointLSN(strFile);
 
                 // Copy wallet file
-                fs::path pathSrc = env->Directory() / strFile;
+                fs::path pathSrc = env->Directory() / m_filename;
                 fs::path pathDest(fs::PathFromString(strDest));
                 if (fs::is_directory(pathDest))
-                    pathDest /= fs::PathFromString(strFile);
+                    pathDest /= m_filename;
 
                 try {
                     if (fs::exists(pathDest) && fs::equivalent(pathSrc, pathDest)) {
@@ -831,7 +837,7 @@ std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, con
     std::unique_ptr<BerkeleyDatabase> db;
     {
         LOCK(cs_db); // Lock env.m_databases until insert in BerkeleyDatabase constructor
-        std::string data_filename = fs::PathToString(data_file.filename());
+        fs::path data_filename = data_file.filename();
         std::shared_ptr<BerkeleyEnvironment> env = GetBerkeleyEnv(data_file.parent_path(), options.use_shared_memory);
         if (env->m_databases.count(data_filename)) {
             error = Untranslated(strprintf("Refusing to load database. Data file '%s' is already loaded.", fs::PathToString(env->Directory() / data_filename)));
diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h
index fd6c76183e..1c99e1f9af 100644
--- a/src/wallet/bdb.h
+++ b/src/wallet/bdb.h
@@ -51,7 +51,7 @@ private:
 
 public:
     std::unique_ptr<DbEnv> dbenv;
-    std::map<std::string, std::reference_wrapper<BerkeleyDatabase>> m_databases;
+    std::map<fs::path, std::reference_wrapper<BerkeleyDatabase>> m_databases;
     std::unordered_map<std::string, WalletDatabaseFileId> m_fileids;
     std::condition_variable_any m_db_in_use;
     bool m_use_shared_memory;
@@ -70,7 +70,7 @@ public:
     void Flush(bool fShutdown);
     void CheckpointLSN(const std::string& strFile);
 
-    void CloseDb(const std::string& strFile);
+    void CloseDb(const fs::path& filename);
     void ReloadDbEnv();
 
     DbTxn* TxnBegin(int flags = DB_TXN_WRITE_NOSYNC)
@@ -97,10 +97,10 @@ public:
     BerkeleyDatabase() = delete;
 
     /** Create DB handle to real database */
-    BerkeleyDatabase(std::shared_ptr<BerkeleyEnvironment> env, std::string filename, const DatabaseOptions& options) :
-        WalletDatabase(), env(std::move(env)), strFile(std::move(filename)), m_max_log_mb(options.max_log_mb)
+    BerkeleyDatabase(std::shared_ptr<BerkeleyEnvironment> env, fs::path filename, const DatabaseOptions& options) :
+        WalletDatabase(), env(std::move(env)), m_filename(std::move(filename)), m_max_log_mb(options.max_log_mb)
     {
-        auto inserted = this->env->m_databases.emplace(strFile, std::ref(*this));
+        auto inserted = this->env->m_databases.emplace(m_filename, std::ref(*this));
         assert(inserted.second);
     }
 
@@ -141,7 +141,7 @@ public:
     bool Verify(bilingual_str& error);
 
     /** Return path to main database filename */
-    std::string Filename() override { return fs::PathToString(env->Directory() / strFile); }
+    std::string Filename() override { return fs::PathToString(env->Directory() / m_filename); }
 
     std::string Format() override { return "bdb"; }
     /**
@@ -158,7 +158,7 @@ public:
     /** Database pointer. This is initialized lazily and reset during flushes, so it can be null. */
     std::unique_ptr<Db> m_db;
 
-    std::string strFile;
+    fs::path m_filename;
     int64_t m_max_log_mb;
 
     /** Make a BerkeleyBatch connected to this database */
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";