Low level functions ScriptToAsmStr (core_io), i64tostr (strencodings) and itostr (strencodings) are locale dependent #18281

1. 
   practicalswift commented at 3:20 pm on March 6, 2020: contributor

   Low level functions ScriptToAsmStr (core_io), i64tostr (strencodings) and itostr (strencodings) are locale dependent:

    0const std::vector<uint8_t> b{0x6a, 0x4, 0xff, 0xff, 0xff, 0xff};
    1const CScript script{b.begin(), b.end()};
    2for (const std::string& locale_string : {"C", "de_DE"}) {
    3    std::locale::global(std::locale(locale_string));
    4    std::cout << "[" << locale_string << "] ScriptToAsmStr(script, false) == " 
    5        << ScriptToAsmStr(script, false) << "\n";
    6    std::cout << "[" << locale_string << "] i64tostr(12345678) == " 
    7        << i64tostr(12345678) << "\n";
    8    std::cout << "[" << locale_string << "] itostr(12345678) == " 
    9        << itostr(12345678) << "\n";
   10}
   

   0[C] ScriptToAsmStr(script, false) == OP_RETURN -2147483647
   1[C] i64tostr(12345678) == 12345678
   2[C] itostr(12345678) == 12345678
   3[de_DE] ScriptToAsmStr(script, false) == OP_RETURN -2.147.483.647
   4[de_DE] i64tostr(12345678) == 12.345.678
   5[de_DE] itostr(12345678) == 12.345.678
   

2. practicalswift added the label Bug on Mar 6, 2020
3. 
   MarcoFalke commented at 7:40 pm on March 6, 2020: member

   I can’t reproduce with bitcoind or bitcoin-qt.

   

4. 
   practicalswift commented at 11:25 pm on March 6, 2020: contributor

   @MarcoFalke That’s because the C++ locale is currently not reflecting LC_* which appears to be by pure luck and not thanks to a conscious decision: [see the comments in PR #18124 for context](https://github.com/bitcoin/bitcoin/pull/18124). Please read the entire discussion to get all the nuances and gotchas (there are quite a few of them!) :)

   The comments in the related PR is #18147 further highlights the confusion regarding what we assume to hold true with regards to locales in the project.

   Note that in bitcoin-qt the C locale (setlocale) is reflecting LC_*, but the C++ locale (std::locale::global) is not.

   If we consider ScriptToAsmStr, i64tostr and itostr to be safe as written now that implies that we assume that the C++ locale never being set to anything but C (the classic locale).

   If we assume the C++ locale to always be C then we should state that explicitly and also assert it as runtime as suggested in #18124.

   The current non-principled “whack-a-mole” approach to locale handling seems fragile at best :)

5. 
   practicalswift commented at 11:56 am on May 19, 2020: contributor

   This one could be marked “good first issue”: it should be trivial to fix.
6. 
   MarcoFalke commented at 12:18 pm on May 19, 2020: member

   @practicalswift “good first issue” is only for issues where one (and only one) uncontroversial solution exists.
7. 
   MarcoFalke commented at 12:20 pm on May 19, 2020: member

   Also i64tostr doesn’t exist in this repo. I think the issue has already been solved?
8. 
   practicalswift commented at 12:30 pm on May 19, 2020: contributor

   @MarcoFalke Yes, i64tostr and itostr are gone, but ScriptToAsmStr is still doing strprintf("%d", CScriptNum(vch, false).getint()); which is locale dependent:

   0const std::vector<uint8_t> b{0x6a, 0x4, 0xff, 0xff, 0xff, 0xff};
   1const CScript script{b.begin(), b.end()};
   2for (const std::string& locale_string : {"C", "de_DE"}) {
   3    std::locale::global(std::locale(locale_string));
   4    std::cout << "[" << locale_string << "] ScriptToAsmStr(script, false) == " 
   5        << ScriptToAsmStr(script, false) << "\n";
   6}
   

   0[C] ScriptToAsmStr(script, false) == OP_RETURN -2147483647
   1[de_DE] ScriptToAsmStr(script, false) == OP_RETURN -2.147.483.647
   

   :)

9. 
   MarcoFalke commented at 1:18 pm on May 19, 2020: member

   yeah, and that is fixed by #18450
10. practicalswift closed this on May 30, 2020

    ---

11. DrahtBot locked this on Feb 15, 2022

Contributors
practicalswift MarcoFalke

Labels
Bug