util: Don’t depend on locale when trimming strings. Add tests. #17760

1. 
   practicalswift commented at 9:11 am on December 17, 2019: contributor

   Don’t depend on locale when trimming strings.

   Add TrimString(…) tests.

   Add boost::algorithm::trim and boost::trim to list of locale dependent functions we want to avoid.

2. Do not depend on the global locale when trimming strings 5f1cfcc4db
3. tests: Add TrimString(...) tests 8c5e407631
4. tests: Mark boost::algorithm::trim and boost::trim as locale dependent 86d060574e
5. DrahtBot added the label RPC/REST/ZMQ on Dec 17, 2019
6. DrahtBot added the label Tests on Dec 17, 2019
7. in src/bitcoin-tx.cpp:769 in 86d060574e

   766@@ -766,8 +767,6 @@ static std::string readStdin()
   767     if (ferror(stdin))
   768         throw std::runtime_error("error reading stdin");
   769 
   770-    boost::algorithm::trim_right(ret);
   

   ---

   ---

   

   paymog commented at 11:38 am on December 21, 2019:

   why does this not have a replacement?

   ---

   

   practicalswift commented at 7:32 pm on December 22, 2019:

   It has: see L796 where it makes more sense :)

   ---

   

   luke-jr commented at 3:49 pm on December 24, 2019:

   Even though it does make sense to trim all whitespace from a hex-encoded transaction, it still also makes sense to trim newline characters here (which should be locale-dependent).

   ---

   

   practicalswift commented at 3:50 pm on December 29, 2019:

   @luke-jr I’m not sure I follow: could you explain why? Please include an example if possible where the new code would give bad results.

   ---

   

   laanwj commented at 3:48 pm on February 12, 2020:

   Wait, how are newline characters locale dependent?
8. 
   paymog commented at 11:38 am on December 21, 2019: none

   What is the motivation for locale independent trimming?
9. 
   practicalswift commented at 9:29 pm on December 22, 2019: contributor

   What is the motivation for locale independent trimming?

   See the developer notes:

   “Avoid using locale dependent functions if possible. You can use the provided lint-locale-dependence.sh to check for accidental use of locale dependent functions.

   Rationale: Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix.”

10. in src/bitcoin-tx.cpp:796 in 86d060574e

    792@@ -794,7 +793,7 @@ static int CommandLineRawTx(int argc, char* argv[])
    793             // param: hex-encoded bitcoin transaction
    794             std::string strHexTx(argv[1]);
    795             if (strHexTx == "-")                 // "-" implies standard input
    796-                strHexTx = readStdin();
    797+                strHexTx = TrimString(readStdin());
    

    ---

    ---

    

    luke-jr commented at 3:46 pm on December 24, 2019:

    This one actually should be locale-dependent.

    ---

    

    practicalswift commented at 0:31 am on December 25, 2019:

    @luke-jr I’m not sure I follow: could you explain why? Please include an example if possible where the new code would give bad results.

    ---

    

    laanwj commented at 3:50 pm on February 12, 2020:

    I’m not sure how important this specific is, but generally we do not use locale-dependent input handling in bitcoind nor utilities. We’d like to avoid differences in locale resulting in hard-to-reproduce bugs and unexpected behavior. Note that this tool is mainly for scripting use where deterministic behavior is important.
11. luke-jr changes_requested
12. 
    luke-jr commented at 3:50 pm on December 24, 2019: member

    Developer notes are ultimately just notes, not a reason to do things the wrong way. Standard input from the user should be using locale-dependent functions.
13. practicalswift requested review from laanwj on Jan 14, 2020
14. practicalswift closed this on Feb 12, 2020

    ---

15. practicalswift deleted the branch on Apr 10, 2021
16. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift paymog luke-jr laanwj

Review Requested
laanwj

Labels
Tests RPC/REST/ZMQ