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

pull practicalswift wants to merge 3 commits into bitcoin:master from practicalswift:trimming-without-borders changing 4 files +24 −7

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.
Do not depend on the global locale when trimming strings 5f1cfcc4db
tests: Add TrimString(...) tests 8c5e407631
tests: Mark boost::algorithm::trim and boost::trim as locale dependent 86d060574e
DrahtBot added the label RPC/REST/ZMQ on Dec 17, 2019
DrahtBot added the label Tests on Dec 17, 2019
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?
paymog commented at 11:38 AM on December 21, 2019: none

What is the motivation for locale independent trimming?
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."
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.
luke-jr changes_requested
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.
practicalswift requested review from laanwj on Jan 14, 2020
Empact cross-referenced this on Feb 12, 2020 from issue Replace uses of boost::trim* with locale-independent alternatives by Empact
practicalswift closed this on Feb 12, 2020
practicalswift deleted the branch on Apr 10, 2021
bitcoin locked this on Aug 16, 2022

Contributors

Review Requested
laanwj

Labels

Tests RPC/REST/ZMQ

Linked (view graph)

#18130 Replace uses of boost::trim* with locale-independent alternatives