Add missing fs::u8path wrap

MarcoFalke commented at 1:50 pm on March 7, 2022: member

Without the wrap, paths can flee the “fs-jail”.

For example, the following compiles on current master:

 0diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp
 1index 5875f0218f..04eef9f5c6 100644
 2--- a/src/test/fs_tests.cpp
 3+++ b/src/test/fs_tests.cpp
 4@@ -19,7 +19,7 @@ BOOST_AUTO_TEST_CASE(fsbridge_pathtostring)
 5 {
 6     std::string u8_str = "fs_tests_₿_🏃";
 7     BOOST_CHECK_EQUAL(fs::PathToString(fs::PathFromString(u8_str)), u8_str);
 8-    BOOST_CHECK_EQUAL(fs::u8path(u8_str).u8string(), u8_str);
 9+    BOOST_CHECK_EQUAL(fs::u8path(u8_str).string(), u8_str);
10     BOOST_CHECK_EQUAL(fs::PathFromString(u8_str).u8string(), u8_str);
11     BOOST_CHECK_EQUAL(fs::PathToString(fs::u8path(u8_str)), u8_str);
12 #ifndef WIN32

After this pull, it will fail, as expected:

0test/fs_tests.cpp:22:42: error: attempt to use a deleted function
1    BOOST_CHECK_EQUAL(fs::u8path(u8_str).string(), u8_str);
2                                         ^
3./fs.h:52:17: note: 'string' has been explicitly marked deleted here
4    std::string string() const = delete;
5                ^
61 error generated.
7make[2]: *** [Makefile:17833: test/test_bitcoin-fs_tests.o] Error 1

Add missing fs::u8path wrap fa7b1b61ca

MarcoFalke added the label Refactoring on Mar 7, 2022

fanquake requested review from ryanofsky on Mar 7, 2022

fanquake requested review from hebasto on Mar 7, 2022

ryanofsky commented at 5:15 pm on March 7, 2022: contributor

This seems ok, but on first impression I don’t like this. There are so many other functions and path methods that return std::filesystem::path types instead of fs::path types that it’s seems inconsistent and pointless to just override this single function.

I think people are getting lost in the sauce over these locale conversion issues. The point of introducing the fs::path class in #22937 was to fix real bugs. Not imaginary bugs, not possible bugs, but actual, real bugs and changes in behavior that would on running bitcoin core software on windows with i18n paths and code pages due to the fact that boost provided a boost::filesystem::path::imbue function, and std::filesystem::path has no equivalent.

The point of adding the fs::path class in #22937 was not to make it impossible to write crazy things like "tests_₿_🏃" or do intentionally bad locale conversions. If that is your goal, there is a better way of achieving it: stop making fs::path inherit from std::filesystem::path, and instead let it contain a private std::filesystem::path member, and whitelist all the methods our application code calls on this class instead of taking the #22937 blacklist approach.

I didn’t do this in #22937 because it would have made the PR substantially bigger. I still don’t think it worth doing now because I think the practical benefit is basically 0. But I wouldn’t object if someone wants to take that more principled approach.

I think this PR and the comments about adding u8path conversions around every ASCII string #24469#pullrequestreview-900999083 and #24469#pullrequestreview-901089587 are overzealous. I think #24469 is a good PR because it fixes a theoretically real test bug. I think #24470 is a questionable PR for the same reason this PR is questionable. But I think it it better motivated, because it is preventing implicit std::string -> path decodings and we know these can be dangerous. I think this PR is less useful because it is not directly preventing any decodings, but just trying to replace std::filesystem::path temporaries with fs::path temporaries, which was never a goal of #22937, and which this PR doesn’t actually come close to accomplishing.

MarcoFalke commented at 5:23 pm on March 7, 2022: member

Ok, closing.

This patch will also be required as part of #24169, so I will just leave it there.

MarcoFalke closed this on Mar 7, 2022

MarcoFalke deleted the branch on Mar 7, 2022

ryanofsky commented at 7:01 pm on March 7, 2022: contributor

Fair enough. I can only imagine the extra levels of bug prevention c++20 constexpr and concept features will allow. If anybody even thinks about introducing a path encoding bug, c++20 can trigger a compile error in their brain

MarcoFalke commented at 2:26 pm on March 16, 2022: member

The thing with the current “mixed wrapper approach” as opposed to a “full wrapper approach” is that changing one line of code may result in compile errors in another part of the code. Beside the “theoretical” one mentioned in this pull, there is already at least one in our codebase, which was fixed with a c-style-cast to fs::path https://github.com/bitcoin/bitcoin/pull/24201/files#diff-a86c3e4f3b8f46384cbbbc5104be50f5637a030ffeb52a83bd207abeb0e72fabR200 . It wonder how long it will take until one of those bites us into the foot.

bitcoin locked this on Aug 9, 2023

Add missing fs::u8path wrap #24493