util: Don't depend on locale when trimming strings. Add tests.#17760
util: Don't depend on locale when trimming strings. Add tests.#17760practicalswift wants to merge 3 commits intobitcoin:masterfrom
Conversation
paymog
left a comment
paymog
left a comment
There was a problem hiding this comment.
What is the motivation for locale independent trimming?
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." |
luke-jr
left a comment
luke-jr
left a comment
There was a problem hiding this comment.
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.
| 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.
| 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?
5 participants
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.