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

6544ea5035 refactor: Block unsafe fs::path std::string conversion calls (Russell Yanofsky)
b39a477ec6 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 6544ea5035
  laanwj:
    Code review ACK 6544ea5035
  hebasto:
    re-ACK 6544ea5035, 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

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();

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"));
 }

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());
 }
 

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.
     //

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);

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"); });