Merge bitcoin/bitcoin#22937: refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method

6544ea5035268025207d2402db2f7d90fde947a6 refactor: Block unsafe fs::path std::string conversion calls (Russell Yanofsky)
b39a477ec69a51b2016d3a8c70c0c77670f87f2b refactor: Add fs::PathToString, fs::PathFromString, u8string, u8path functions (Russell Yanofsky)

Pull request description:

  The `fs::path` class has a `std::string` constructor which will implicitly convert from strings. Implicit conversions like this are not great in general because they can hide complexity and inefficiencies in the code, but this case is especially bad, because after the transition from `boost::filesystem` to `std::filesystem` in #20744 the behavior of this constructor on windows will be more complicated and can mangle path strings. The `fs::path` class also has a `.string()` method which is inverse of the constructor and has the same problems.

  Fix this by replacing the unsafe method calls with `PathToString` and `PathFromString` function calls, and by forbidding unsafe method calls in the future.

ACKs for top commit:
  kiminuo:
    ACK 6544ea5035268025207d2402db2f7d90fde947a6
  laanwj:
    Code review ACK 6544ea5035268025207d2402db2f7d90fde947a6
  hebasto:
    re-ACK 6544ea5035268025207d2402db2f7d90fde947a6, only added `fsbridge_stem` test case, updated comment, and rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22937#pullrequestreview-765503126) review. Verified with the following command:

Tree-SHA512: c36324740eb4ee55151146626166c00d5ccc4b6f3df777e75c112bcb4d1db436c1d9cc8c29a1e7fb96051457d317961ab42e6c380c3be2771d135771b2b49fa0

6 files changed, 42 insertions, 15 deletions

diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp
index 526a3c27be..ecb838a7dd 100644
--- a/src/test/fs_tests.cpp
+++ b/src/test/fs_tests.cpp
@@ -11,6 +11,33 @@
 
 BOOST_FIXTURE_TEST_SUITE(fs_tests, BasicTestingSetup)
 
+BOOST_AUTO_TEST_CASE(fsbridge_pathtostring)
+{
+    std::string u8_str = "fs_tests_₿_🏃";
+    BOOST_CHECK_EQUAL(fs::PathToString(fs::PathFromString(u8_str)), u8_str);
+    BOOST_CHECK_EQUAL(fs::u8path(u8_str).u8string(), u8_str);
+    BOOST_CHECK_EQUAL(fs::PathFromString(u8_str).u8string(), u8_str);
+    BOOST_CHECK_EQUAL(fs::PathToString(fs::u8path(u8_str)), u8_str);
+#ifndef WIN32
+    // On non-windows systems, verify that arbitrary byte strings containing
+    // invalid UTF-8 can be round tripped successfully with PathToString and
+    // PathFromString. On non-windows systems, paths are just byte strings so
+    // these functions do not do any encoding. On windows, paths are Unicode,
+    // and these functions do encoding and decoding, so the behavior of this
+    // test would be undefined.
+    std::string invalid_u8_str = "\xf0";
+    BOOST_CHECK_EQUAL(invalid_u8_str.size(), 1);
+    BOOST_CHECK_EQUAL(fs::PathToString(fs::PathFromString(invalid_u8_str)), invalid_u8_str);
+#endif
+}
+
+BOOST_AUTO_TEST_CASE(fsbridge_stem)
+{
+    std::string test_filename = "fs_tests_₿_🏃.dat";
+    std::string expected_stem = "fs_tests_₿_🏃";
+    BOOST_CHECK_EQUAL(fs::PathToString(fs::PathFromString(test_filename).stem()), expected_stem);
+}
+
 BOOST_AUTO_TEST_CASE(fsbridge_fstream)
 {
     fs::path tmpfolder = m_args.GetDataDirBase();
diff --git a/src/test/fuzz/banman.cpp b/src/test/fuzz/banman.cpp
index 561cc83c72..fbba25c404 100644
--- a/src/test/fuzz/banman.cpp
+++ b/src/test/fuzz/banman.cpp
@@ -48,7 +48,7 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
     const bool start_with_corrupted_banlist{fuzzed_data_provider.ConsumeBool()};
     bool force_read_and_write_to_err{false};
     if (start_with_corrupted_banlist) {
-        assert(WriteBinaryFile(banlist_file.string() + ".json",
+        assert(WriteBinaryFile(banlist_file + ".json",
                                fuzzed_data_provider.ConsumeRandomLengthString()));
     } else {
         force_read_and_write_to_err = fuzzed_data_provider.ConsumeBool();
@@ -111,5 +111,5 @@ FUZZ_TARGET_INIT(banman, initialize_banman)
             assert(banmap == banmap_read);
         }
     }
-    fs::remove(banlist_file.string() + ".json");
+    fs::remove(fs::PathToString(banlist_file + ".json"));
 }
diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp
index 340ce33d91..15cba9e3e5 100644
--- a/src/test/settings_tests.cpp
+++ b/src/test/settings_tests.cpp
@@ -80,19 +80,19 @@ BOOST_AUTO_TEST_CASE(ReadWrite)
         "dupe": "dupe"
     })");
     BOOST_CHECK(!util::ReadSettings(path, values, errors));
-    std::vector<std::string> dup_keys = {strprintf("Found duplicate key dupe in settings file %s", path.string())};
+    std::vector<std::string> dup_keys = {strprintf("Found duplicate key dupe in settings file %s", fs::PathToString(path))};
     BOOST_CHECK_EQUAL_COLLECTIONS(errors.begin(), errors.end(), dup_keys.begin(), dup_keys.end());
 
     // Check non-kv json files not allowed
     WriteText(path, R"("non-kv")");
     BOOST_CHECK(!util::ReadSettings(path, values, errors));
-    std::vector<std::string> non_kv = {strprintf("Found non-object value \"non-kv\" in settings file %s", path.string())};
+    std::vector<std::string> non_kv = {strprintf("Found non-object value \"non-kv\" in settings file %s", fs::PathToString(path))};
     BOOST_CHECK_EQUAL_COLLECTIONS(errors.begin(), errors.end(), non_kv.begin(), non_kv.end());
 
     // Check invalid json not allowed
     WriteText(path, R"(invalid json)");
     BOOST_CHECK(!util::ReadSettings(path, values, errors));
-    std::vector<std::string> fail_parse = {strprintf("Unable to parse settings file %s", path.string())};
+    std::vector<std::string> fail_parse = {strprintf("Unable to parse settings file %s", fs::PathToString(path))};
     BOOST_CHECK_EQUAL_COLLECTIONS(errors.begin(), errors.end(), fail_parse.begin(), fail_parse.end());
 }
 
diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h
index 81ea4c38f5..e95573022c 100644
--- a/src/test/util/chainstate.h
+++ b/src/test/util/chainstate.h
@@ -36,7 +36,7 @@ CreateAndActivateUTXOSnapshot(NodeContext& node, const fs::path root, F malleati
 
     UniValue result = CreateUTXOSnapshot(node, node.chainman->ActiveChainstate(), auto_outfile);
     BOOST_TEST_MESSAGE(
-        "Wrote UTXO snapshot to " << snapshot_path.make_preferred().string() << ": " << result.write());
+        "Wrote UTXO snapshot to " << fs::PathToString(snapshot_path.make_preferred()) << ": " << result.write());
 
     // Read the written snapshot in and then activate it.
     //
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index ebefa9974e..a3c7564d76 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -91,8 +91,8 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
         extra_args);
     util::ThreadRename("test");
     fs::create_directories(m_path_root);
-    m_args.ForceSetArg("-datadir", m_path_root.string());
-    gArgs.ForceSetArg("-datadir", m_path_root.string());
+    m_args.ForceSetArg("-datadir", fs::PathToString(m_path_root));
+    gArgs.ForceSetArg("-datadir", fs::PathToString(m_path_root));
     gArgs.ClearPathCache();
     {
         SetupServerArgs(*m_node.args);
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index b5088d3c33..b1300d06ba 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -51,23 +51,23 @@ BOOST_AUTO_TEST_CASE(util_datadir)
 {
     // Use local args variable instead of m_args to avoid making assumptions about test setup
     ArgsManager args;
-    args.ForceSetArg("-datadir", m_path_root.string());
+    args.ForceSetArg("-datadir", fs::PathToString(m_path_root));
 
     const fs::path dd_norm = args.GetDataDirBase();
 
-    args.ForceSetArg("-datadir", dd_norm.string() + "/");
+    args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/");
     args.ClearPathCache();
     BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
 
-    args.ForceSetArg("-datadir", dd_norm.string() + "/.");
+    args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/.");
     args.ClearPathCache();
     BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
 
-    args.ForceSetArg("-datadir", dd_norm.string() + "/./");
+    args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/./");
     args.ClearPathCache();
     BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
 
-    args.ForceSetArg("-datadir", dd_norm.string() + "/.//");
+    args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/.//");
     args.ClearPathCache();
     BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
 }
@@ -1181,13 +1181,13 @@ BOOST_AUTO_TEST_CASE(util_ReadWriteSettings)
 {
     // Test writing setting.
     TestArgsManager args1;
-    args1.ForceSetArg("-datadir", m_path_root.string());
+    args1.ForceSetArg("-datadir", fs::PathToString(m_path_root));
     args1.LockSettings([&](util::Settings& settings) { settings.rw_settings["name"] = "value"; });
     args1.WriteSettingsFile();
 
     // Test reading setting.
     TestArgsManager args2;
-    args2.ForceSetArg("-datadir", m_path_root.string());
+    args2.ForceSetArg("-datadir", fs::PathToString(m_path_root));
     args2.ReadSettingsFile();
     args2.LockSettings([&](util::Settings& settings) { BOOST_CHECK_EQUAL(settings.rw_settings["name"].get_str(), "value"); });