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

6544ea5 refactor: Block unsafe fs::path std::string conversion calls (Russell Yanofsky)
b39a477 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 6544ea5
  laanwj:
    Code review ACK 6544ea5
  hebasto:
    re-ACK 6544ea5, only added `fsbridge_stem` test case, updated comment, and rebased since my [previous](bitcoin/bitcoin#22937 (review)) review. Verified with the following command:

Tree-SHA512: c36324740eb4ee55151146626166c00d5ccc4b6f3df777e75c112bcb4d1db436c1d9cc8c29a1e7fb96051457d317961ab42e6c380c3be2771d135771b2b49fa0

Author: laanwj

src/addrdb.cpp

@@ -58,7 +58,7 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
     if (fileout.IsNull()) {
         fileout.fclose();
         remove(pathTmp);
-        return error("%s: Failed to open file %s", __func__, pathTmp.string());
+        return error("%s: Failed to open file %s", __func__, fs::PathToString(pathTmp));
     }
 
     // Serialize
@@ -70,7 +70,7 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
     if (!FileCommit(fileout.Get())) {
         fileout.fclose();
         remove(pathTmp);
-        return error("%s: Failed to flush file %s", __func__, pathTmp.string());
+        return error("%s: Failed to flush file %s", __func__, fs::PathToString(pathTmp));
     }
     fileout.fclose();
 
@@ -122,8 +122,8 @@ void DeserializeFileDB(const fs::path& path, Data& data, int version)
 } // namespace
 
 CBanDB::CBanDB(fs::path ban_list_path)
-    : m_banlist_dat(ban_list_path.string() + ".dat"),
-      m_banlist_json(ban_list_path.string() + ".json")
+    : m_banlist_dat(ban_list_path + ".dat"),
+      m_banlist_json(ban_list_path + ".json")
 {
 }
 
@@ -143,7 +143,7 @@ bool CBanDB::Write(const banmap_t& banSet)
 bool CBanDB::Read(banmap_t& banSet)
 {
     if (fs::exists(m_banlist_dat)) {
-        LogPrintf("banlist.dat ignored because it can only be read by " PACKAGE_NAME " version 22.x. Remove %s to silence this warning.\n", m_banlist_dat);
+        LogPrintf("banlist.dat ignored because it can only be read by " PACKAGE_NAME " version 22.x. Remove %s to silence this warning.\n", fs::quoted(fs::PathToString(m_banlist_dat)));
     }
     // If the JSON banlist does not exist, then recreate it
     if (!fs::exists(m_banlist_json)) {
@@ -155,15 +155,15 @@ bool CBanDB::Read(banmap_t& banSet)
 
     if (!util::ReadSettings(m_banlist_json, settings, errors)) {
         for (const auto& err : errors) {
-            LogPrintf("Cannot load banlist %s: %s\n", m_banlist_json.string(), err);
+            LogPrintf("Cannot load banlist %s: %s\n", fs::PathToString(m_banlist_json), err);
         }
         return false;
     }
 
     try {
         BanMapFromJson(settings[JSON_KEY], banSet);
     } catch (const std::runtime_error& e) {
-        LogPrintf("Cannot parse banlist %s: %s\n", m_banlist_json.string(), e.what());
+        LogPrintf("Cannot parse banlist %s: %s\n", fs::PathToString(m_banlist_json), e.what());
         return false;
     }
 
@@ -194,12 +194,12 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
     } catch (const DbNotFoundError&) {
         // Addrman can be in an inconsistent state after failure, reset it
         addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
-        LogPrintf("Creating peers.dat because the file was not found (%s)\n", path_addr);
+        LogPrintf("Creating peers.dat because the file was not found (%s)\n", fs::quoted(fs::PathToString(path_addr)));
         DumpPeerAddresses(args, *addrman);
     } catch (const std::exception& e) {
         addrman = nullptr;
         return strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) to have a new one created on the next start."),
-                         e.what(), PACKAGE_BUGREPORT, path_addr);
+                         e.what(), PACKAGE_BUGREPORT, fs::quoted(fs::PathToString(path_addr)));
     }
     return std::nullopt;
 }
@@ -215,7 +215,7 @@ std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
     std::vector<CAddress> anchors;
     try {
         DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT);
-        LogPrintf("Loaded %i addresses from %s\n", anchors.size(), anchors_db_path.filename());
+        LogPrintf("Loaded %i addresses from %s\n", anchors.size(), fs::quoted(fs::PathToString(anchors_db_path.filename())));
     } catch (const std::exception&) {
         anchors.clear();
     }

src/bitcoin-cli.cpp

@@ -786,7 +786,7 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
         if (failedToGetAuthCookie) {
             throw std::runtime_error(strprintf(
                 "Could not locate RPC credentials. No authentication cookie could be found, and RPC password is not set.  See -rpcpassword and -stdinrpcpass.  Configuration file: (%s)",
-                GetConfigFile(gArgs.GetArg("-conf", BITCOIN_CONF_FILENAME)).string()));
+                fs::PathToString(GetConfigFile(gArgs.GetArg("-conf", BITCOIN_CONF_FILENAME)))));
         } else {
             throw std::runtime_error("Authorization failed: Incorrect rpcuser or rpcpassword");
         }

src/dbwrapper.cpp

@@ -115,7 +115,7 @@ static leveldb::Options GetOptions(size_t nCacheSize)
 }
 
 CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bool fWipe, bool obfuscate)
-    : m_name{path.stem().string()}
+    : m_name{fs::PathToString(path.stem())}
 {
     penv = nullptr;
     readoptions.verify_checksums = true;
@@ -129,21 +129,21 @@ CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bo
         options.env = penv;
     } else {
         if (fWipe) {
-            LogPrintf("Wiping LevelDB in %s\n", path.string());
-            leveldb::Status result = leveldb::DestroyDB(path.string(), options);
+            LogPrintf("Wiping LevelDB in %s\n", fs::PathToString(path));
+            leveldb::Status result = leveldb::DestroyDB(fs::PathToString(path), options);
             dbwrapper_private::HandleError(result);
         }
         TryCreateDirectories(path);
-        LogPrintf("Opening LevelDB in %s\n", path.string());
+        LogPrintf("Opening LevelDB in %s\n", fs::PathToString(path));
     }
-    leveldb::Status status = leveldb::DB::Open(options, path.string(), &pdb);
+    leveldb::Status status = leveldb::DB::Open(options, fs::PathToString(path), &pdb);
     dbwrapper_private::HandleError(status);
     LogPrintf("Opened LevelDB successfully\n");
 
     if (gArgs.GetBoolArg("-forcecompactdb", false)) {
-        LogPrintf("Starting database compaction of %s\n", path.string());
+        LogPrintf("Starting database compaction of %s\n", fs::PathToString(path));
         pdb->CompactRange(nullptr, nullptr);
-        LogPrintf("Finished database compaction of %s\n", path.string());
+        LogPrintf("Finished database compaction of %s\n", fs::PathToString(path));
     }
 
     // The base-case obfuscation key, which is a noop.
@@ -160,10 +160,10 @@ CDBWrapper::CDBWrapper(const fs::path& path, size_t nCacheSize, bool fMemory, bo
         Write(OBFUSCATE_KEY_KEY, new_key);
         obfuscate_key = new_key;
 
-        LogPrintf("Wrote new obfuscate key for %s: %s\n", path.string(), HexStr(obfuscate_key));
+        LogPrintf("Wrote new obfuscate key for %s: %s\n", fs::PathToString(path), HexStr(obfuscate_key));
     }
 
-    LogPrintf("Using obfuscation key for %s: %s\n", path.string(), HexStr(obfuscate_key));
+    LogPrintf("Using obfuscation key for %s: %s\n", fs::PathToString(path), HexStr(obfuscate_key));
 }
 
 CDBWrapper::~CDBWrapper()

src/flatfile.cpp

@@ -41,11 +41,11 @@ FILE* FlatFileSeq::Open(const FlatFilePos& pos, bool read_only)
     if (!file && !read_only)
         file = fsbridge::fopen(path, "wb+");
     if (!file) {
-        LogPrintf("Unable to open file %s\n", path.string());
+        LogPrintf("Unable to open file %s\n", fs::PathToString(path));
         return nullptr;
     }
     if (pos.nPos && fseek(file, pos.nPos, SEEK_SET)) {
-        LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, path.string());
+        LogPrintf("Unable to seek to position %u of %s\n", pos.nPos, fs::PathToString(path));
         fclose(file);
         return nullptr;
     }

src/fs.cpp

@@ -24,7 +24,7 @@ namespace fsbridge {
 FILE *fopen(const fs::path& p, const char *mode)
 {
 #ifndef WIN32
-    return ::fopen(p.string().c_str(), mode);
+    return ::fopen(p.c_str(), mode);
 #else
     std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>,wchar_t> utf8_cvt;
     return ::_wfopen(p.wstring().c_str(), utf8_cvt.from_bytes(mode).c_str());
@@ -46,7 +46,7 @@ static std::string GetErrorReason()
 
 FileLock::FileLock(const fs::path& file)
 {
-    fd = open(file.string().c_str(), O_RDWR);
+    fd = open(file.c_str(), O_RDWR);
     if (fd == -1) {
         reason = GetErrorReason();
     }
@@ -249,9 +249,9 @@ void ofstream::close()
 #else // __GLIBCXX__
 
 #if BOOST_VERSION >= 107700
-static_assert(sizeof(*BOOST_FILESYSTEM_C_STR(fs::path())) == sizeof(wchar_t),
+static_assert(sizeof(*BOOST_FILESYSTEM_C_STR(boost::filesystem::path())) == sizeof(wchar_t),
 #else
-static_assert(sizeof(*fs::path().BOOST_FILESYSTEM_C_STR) == sizeof(wchar_t),
+static_assert(sizeof(*boost::filesystem::path().BOOST_FILESYSTEM_C_STR) == sizeof(wchar_t),
 #endif // BOOST_VERSION >= 107700
     "Warning: This build is using boost::filesystem ofstream and ifstream "
     "implementations which will fail to open paths containing multibyte "

src/fs.h

@@ -13,9 +13,132 @@
 
 #include <boost/filesystem.hpp>
 #include <boost/filesystem/fstream.hpp>
+#include <tinyformat.h>
 
 /** Filesystem operations and types */
-namespace fs = boost::filesystem;
+namespace fs {
+
+using namespace boost::filesystem;
+
+/**
+ * Path class wrapper to prepare application code for transition from
+ * boost::filesystem library to std::filesystem implementation. The main
+ * purpose of the class is to define fs::path::u8string() and fs::u8path()
+ * functions not present in boost. It also blocks calls to the
+ * fs::path(std::string) implicit constructor and the fs::path::string()
+ * method, which worked well in the boost::filesystem implementation, but have
+ * unsafe and unpredictable behavior on Windows in the std::filesystem
+ * implementation (see implementation note in \ref PathToString for details).
+ */
+class path : public boost::filesystem::path
+{
+public:
+    using boost::filesystem::path::path;
+
+    // Allow path objects arguments for compatibility.
+    path(boost::filesystem::path path) : boost::filesystem::path::path(std::move(path)) {}
+    path& operator=(boost::filesystem::path path) { boost::filesystem::path::operator=(std::move(path)); return *this; }
+    path& operator/=(boost::filesystem::path path) { boost::filesystem::path::operator/=(std::move(path)); return *this; }
+
+    // Allow literal string arguments, which are safe as long as the literals are ASCII.
+    path(const char* c) : boost::filesystem::path(c) {}
+    path& operator=(const char* c) { boost::filesystem::path::operator=(c); return *this; }
+    path& operator/=(const char* c) { boost::filesystem::path::operator/=(c); return *this; }
+    path& append(const char* c) { boost::filesystem::path::append(c); return *this; }
+
+    // Disallow std::string arguments to avoid locale-dependent decoding on windows.
+    path(std::string) = delete;
+    path& operator=(std::string) = delete;
+    path& operator/=(std::string) = delete;
+    path& append(std::string) = delete;
+
+    // Disallow std::string conversion method to avoid locale-dependent encoding on windows.
+    std::string string() const = delete;
+
+    // Define UTF-8 string conversion method not present in boost::filesystem but present in std::filesystem.
+    std::string u8string() const { return boost::filesystem::path::string(); }
+};
+
+// Define UTF-8 string conversion function not present in boost::filesystem but present in std::filesystem.
+static inline path u8path(const std::string& string)
+{
+    return boost::filesystem::path(string);
+}
+
+// Disallow implicit std::string conversion for system_complete to avoid
+// locale-dependent encoding on windows.
+static inline path system_complete(const path& p)
+{
+    return boost::filesystem::system_complete(p);
+}
+
+// Disallow implicit std::string conversion for exists to avoid
+// locale-dependent encoding on windows.
+static inline bool exists(const path& p)
+{
+    return boost::filesystem::exists(p);
+}
+
+// Allow explicit quoted stream I/O.
+static inline auto quoted(const std::string& s)
+{
+    return boost::io::quoted(s, '&');
+}
+
+// Allow safe path append operations.
+static inline path operator+(path p1, path p2)
+{
+    p1 += std::move(p2);
+    return p1;
+}
+
+/**
+ * Convert path object to byte string. On POSIX, paths natively are byte
+ * strings so this is trivial. On Windows, paths natively are Unicode, so an
+ * encoding step is necessary.
+ *
+ * The inverse of \ref PathToString is \ref PathFromString. The strings
+ * returned and parsed by these functions can be used to call POSIX APIs, and
+ * for roundtrip conversion, logging, and debugging. But they are not
+ * guaranteed to be valid UTF-8, and are generally meant to be used internally,
+ * not externally. When communicating with external programs and libraries that
+ * require UTF-8, fs::path::u8string() and fs::u8path() methods can be used.
+ * For other applications, if support for non UTF-8 paths is required, or if
+ * higher-level JSON or XML or URI or C-style escapes are preferred, it may be
+ * also be appropriate to use different path encoding functions.
+ *
+ * Implementation note: On Windows, the std::filesystem::path(string)
+ * constructor and std::filesystem::path::string() method are not safe to use
+ * here, because these methods encode the path using C++'s narrow multibyte
+ * encoding, which on Windows corresponds to the current "code page", which is
+ * unpredictable and typically not able to represent all valid paths. So
+ * std::filesystem::path::u8string() and std::filesystem::u8path() functions
+ * are used instead on Windows. On POSIX, u8string/u8path functions are not
+ * safe to use because paths are not always valid UTF-8, so plain string
+ * methods which do not transform the path there are used.
+ */
+static inline std::string PathToString(const path& path)
+{
+#ifdef WIN32
+    return path.u8string();
+#else
+    static_assert(std::is_same<path::string_type, std::string>::value, "PathToString not implemented on this platform");
+    return path.boost::filesystem::path::string();
+#endif
+}
+
+/**
+ * Convert byte string to path object. Inverse of \ref PathToString.
+ */
+static inline path PathFromString(const std::string& string)
+{
+#ifdef WIN32
+    return u8path(string);
+#else
+    return boost::filesystem::path(string);
+#endif
+}
+} // namespace fs
 
 /** Bridge operations to C stdio */
 namespace fsbridge {
@@ -103,4 +226,11 @@ namespace fsbridge {
 #endif // WIN32 && __GLIBCXX__
 };
 
+// Disallow path operator<< formatting in tinyformat to avoid locale-dependent
+// encoding on windows.
+namespace tinyformat {
+template<> inline void formatValue(std::ostream&, const char*, const char*, int, const boost::filesystem::path&) = delete;
+template<> inline void formatValue(std::ostream&, const char*, const char*, int, const fs::path&) = delete;
+} // namespace tinyformat
+
 #endif // BITCOIN_FS_H

src/i2p.cpp

@@ -328,7 +328,7 @@ void Session::GenerateAndSavePrivateKey(const Sock& sock)
     if (!WriteBinaryFile(m_private_key_file,
                          std::string(m_private_key.begin(), m_private_key.end()))) {
         throw std::runtime_error(
-            strprintf("Cannot save I2P private key to %s", m_private_key_file));
+            strprintf("Cannot save I2P private key to %s", fs::quoted(fs::PathToString(m_private_key_file))));
     }
 }