Disallow more unsafe string->path conversions allowed by path append operators #24470

1. 
   ryanofsky commented at 8:07 pm on March 3, 2022: member

   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.

2. DrahtBot added the label GUI on Mar 3, 2022
3. DrahtBot added the label P2P on Mar 3, 2022
4. DrahtBot added the label Utils/log/libs on Mar 3, 2022
5. DrahtBot added the label UTXO Db and Indexes on Mar 3, 2022
6. DrahtBot added the label Validation on Mar 3, 2022
7. DrahtBot added the label Wallet on Mar 3, 2022
8. 
   DrahtBot commented at 10:34 pm on March 3, 2022: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #18904 (Don’t call lsn_reset in periodic flush by bvbfan)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

9. 
   hebasto commented at 7:24 pm on March 5, 2022: member

   Concept ACK.
10. Trinacristella approved
11. DrahtBot added the label Needs rebase on Mar 24, 2022
12. 
    laanwj commented at 2:24 pm on April 7, 2022: member

    Concept ACK
13. laanwj removed the label GUI on Apr 7, 2022
14. laanwj removed the label Wallet on Apr 7, 2022
15. laanwj removed the label UTXO Db and Indexes on Apr 7, 2022
16. laanwj removed the label P2P on Apr 7, 2022
17. laanwj removed the label Validation on Apr 7, 2022
18. laanwj removed the label Utils/log/libs on Apr 7, 2022
19. laanwj added the label Refactoring on Apr 7, 2022
20. Disallow more unsafe string->path conversions allowed by path append operators

    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 altoghther, 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.
    
    Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

    f64aa9c411
21. ryanofsky force-pushed on Apr 22, 2022
22. 
    ryanofsky commented at 2:17 am on April 22, 2022: member

    Rebased 67ca71e5b7ec8ca3946a993c538dc03b94233b89 -> cb9f6b11865dd671e0e51755b5d7225b68fbcb26 (pr/patha.1 -> pr/patha.2, compare) due to conflict with #23732, and adding fix for windows build error https://cirrus-ci.com/task/4639862308470784
23. DrahtBot removed the label Needs rebase on Apr 22, 2022
24. 
    hebasto commented at 7:45 am on April 22, 2022: member

    Maybe add

     0diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp
     1index 1e67b1a0d..706cf2f7d 100644
     2--- a/src/qt/guiutil.cpp
     3+++ b/src/qt/guiutil.cpp
     4@@ -508,7 +508,7 @@ fs::path static StartupShortcutPath()
     5         return GetSpecialFolderPath(CSIDL_STARTUP) / "Bitcoin.lnk";
     6     if (chain == CBaseChainParams::TESTNET) // Remove this special case when CBaseChainParams::TESTNET = "testnet4"
     7         return GetSpecialFolderPath(CSIDL_STARTUP) / "Bitcoin (testnet).lnk";
     8-    return GetSpecialFolderPath(CSIDL_STARTUP) / strprintf("Bitcoin (%s).lnk", chain);
     9+    return GetSpecialFolderPath(CSIDL_STARTUP) / fs::u8path(strprintf("Bitcoin (%s).lnk", chain));
    10 }
    11 
    12 bool GetStartOnSystemStartup()
    13diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
    14index 82693eae8..dbc297f45 100644
    15--- a/src/wallet/bdb.cpp
    16+++ b/src/wallet/bdb.cpp
    17@@ -302,7 +302,7 @@ BerkeleyDatabase::~BerkeleyDatabase()
    18         assert(!m_db);
    19         size_t erased = env->m_databases.erase(m_filename);
    20         assert(erased == 1);
    21-        env->m_fileids.erase(m_filename);
    22+        env->m_fileids.erase(fs::PathToString(m_filename));
    23     }
    24 }
    25 
    

    ?

    It makes CI greener – https://cirrus-ci.com/build/6528392144093184 :)

25. in src/fs.h:100 in cb9f6b1186 outdated

     97 {
     98-    p1 += std::move(p2);
     99+    p1 /= std::move(p2);
    100     return p1;
    101 }
    102+static inline path operator/(path p1, std::filesystem::path p2)
    

    ---

    ---

    

    hebasto commented at 9:03 am on April 22, 2022:

    Why std::filesystem::path for second parameter? Isn’t it safer to limit the usage of std::filesystem::path only within the fs::path wrapper?

    ---

    

    ryanofsky commented at 4:54 pm on April 25, 2022:

    re: #24470 (review)

    Why std::filesystem::path for second parameter? Isn’t it safer to limit the usage of std::filesystem::path only within the fs::path wrapper?

    Good point, fs::path is safer and this doesn’t seem to be necessary, now dropped

26. in src/fs.h:114 in cb9f6b1186 outdated

    116+}
    117+static inline path operator+(path p1, path::value_type p2)
    118+{
    119+    p1 += p2;
    120+    return p1;
    121+}
    

    ---

    ---

    

    dongcarl commented at 9:34 pm on April 22, 2022:

    Wondering why there’s a difference between the allowed right-hand operands for / and +

    ---

    

    ryanofsky commented at 4:54 pm on April 25, 2022:

    re: #24470 (review)

    Wondering why there’s a difference between the allowed right-hand operands for / and +

    The cases that are listed are just cases that seemed safe and are in used by existing code. (I think it’d be fine to add cases that are safe and aren’t in use by existing code, too, but I didn’t go looking for these.)

    On the specific cases, combining two path objects with / is safe and useful. Combining two path objects with + is safe probably not useful. Appending a literal string is safe to a path is safe (assuming the literal string is ASCII) both with + and /. Adding a native character to a path is safe and semi-useful (used by some tests at least)

27. ryanofsky force-pushed on Apr 25, 2022
28. 
    ryanofsky commented at 5:56 pm on April 25, 2022: member

    Updated cb9f6b11865dd671e0e51755b5d7225b68fbcb26 -> f64aa9c411ad78259756a28756ec1eb8069b5ab4 (pr/patha.2 -> pr/patha.3, compare) adding hebasto’s fixes for windows build errors https://cirrus-ci.com/build/6528392144093184
29. hebasto approved
30. 
    hebasto commented at 6:33 pm on April 25, 2022: member

    ACK f64aa9c411ad78259756a28756ec1eb8069b5ab4
31. MarcoFalke merged this on May 3, 2022
32. MarcoFalke closed this on May 3, 2022

    ---

33. sidhujag referenced this in commit ad37f1d3bc on May 4, 2022
34. DrahtBot locked this on May 3, 2023

Contributors
ryanofsky DrahtBot hebasto Trinacristella laanwj dongcarl

Labels
Refactoring