util: Don't depend on locale when trimming strings. Add tests. #17760
Conversation
See the developer notes: "Avoid using locale dependent functions if possible. You can use the provided Rationale: Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix." |
| @@ -794,7 +793,7 @@ static int CommandLineRawTx(int argc, char* argv[]) | |||
| // param: hex-encoded bitcoin transaction | |||
| std::string strHexTx(argv[1]); | |||
| if (strHexTx == "-") // "-" implies standard input | |||
| strHexTx = readStdin(); | |||
| strHexTx = TrimString(readStdin()); | |||
There was a problem hiding this comment.
This one actually should be locale-dependent.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
| @@ -766,8 +767,6 @@ static std::string readStdin() | |||
| if (ferror(stdin)) | |||
| throw std::runtime_error("error reading stdin"); | |||
|
|
|||
| boost::algorithm::trim_right(ret); | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Wait, how are newline characters locale dependent?
Don't depend on locale when trimming strings.
Add
TrimString(…)tests.Add
boost::algorithm::trimandboost::trimto list of locale dependent functions we want to avoid.