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 12: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

practicalswiftpaymogluke-jrlaanwj

Review Requested
laanwj