Fix locale fallback and guard tests against invalid locale settings

dexX7 commented at 1:41 am on March 27, 2015: contributor

Unfortually a fallback to “C” locale via std::locale::global does not cover all scenarios with messed up environment locale settings and on Ubuntu 14.01 (with LANG=en_US.UTF-8, LANGUAGE=en_US, LC_* empty) setting LANG=invalid triggers a crash right at the start of bitcoind and bitcoin-qt.

This also affects test_bitcoin and test_bitcoin-qt, which were not guarded at all.

The PR expands the scope of the locale fallback and prevents crashes due to invalid locale settings of bitcoind, bitcoin-qt, test_bitcoin and test_bitcoin-qt. I didn’t want to create a seperate PR for the 0.10 branch, but a conflict free version is available: https://github.com/bitcoin/bitcoin/compare/0.10...dexX7:0.10-init-locale-fallback-tests-fix

Initialization: set fallback locale as environment variable

The scope of `std::locale::global` appears to be smaller than `setenv("LC_ALL", ...)` and insufficient to fix messed up locale settings for the whole application.

ba0fa0d9bc

laanwj commented at 7:05 am on March 27, 2015: member

This appears to revert part of #5877.

Can you add a concrete test case for this? This more and more looks like just some occult incantation to ward off the evil spirits of bad locales :)

laanwj added the label Improvement on Mar 27, 2015

Initialization: setup environment before starting tests

The environment is prepared by the main thread to guard against invalid locale settings and to prevent deinitialization issues of Boost path, which can result in app crashes.

fc3979ac69

Initialization: setup environment before starting QT tests

The environment is prepared by the main thread to guard against invalid locale settings.

3a3ecc02e5

in src/test/test_bitcoin.cpp: in c2ce0632db outdated

36@@ -37,6 +37,7 @@ BasicTestingSetup::~BasicTestingSetup()
37 
38 TestingSetup::TestingSetup()
39 {
40+        SetupEnvironment();

Diapolo commented at 12:28 pm on March 27, 2015:

Nit: Wrong indentation, also true for bitdb.MakeMock(); below.

dexX7 force-pushed on Mar 29, 2015

dexX7 commented at 9:05 am on March 29, 2015: contributor

@laanwj: Yeah, I’m truely sorry.. :/ I focused on addressing the deinitialization issue, which caused the crashes on Windows, probably with two steps forward, where only one was appropriate, as it seems.

I further moved TestingSetup() from TestingSetup() to BasicTestingSetup(), after running a proper test.

The RPC test is available here: https://gist.github.com/dexX7/ab3f7411a9040e96d55c

Travis build with the test, based on unfixed master (with delayed error assertion): https://travis-ci.org/dexX7/bitcoin/builds/56274510

Travis build with the test, based on the 0.10 branch: https://travis-ci.org/dexX7/bitcoin/builds/56281938

Travis build with the test, based on this pull request: https://travis-ci.org/dexX7/bitcoin/builds/56281999

May I add the test to qa/rpc-tests as part of this PR, but without adding it to qa/pull-tester/run-rpc-tests.sh, as it is somewhat time consuming? @Diapolo: I’d be happy to format src/test/test_bitcoin.cpp and update the copyright header in src/qt/test/test_main.cpp, if that would be considered as appropriate for the scope of this PR?

laanwj commented at 10:20 am on April 1, 2015: member

May I add the test to qa/rpc-tests as part of this PR, but without adding it to qa/pull-tester/run-rpc-tests.sh, as it is somewhat time consuming?

Sure! Although I’d prefer making the test consume less time, and adding it to the pull tester. What is so time consuming? The environment setup is one of the first things it does, after all, so at least in theory it could be a matter of quickly invoking the executables.

In any case you’ve convinced me enough to merge this, adding the test could be a further PR.

laanwj merged this on Apr 1, 2015

laanwj closed this on Apr 1, 2015

laanwj referenced this in commit 41113e33ad on Apr 1, 2015

dexX7 commented at 10:32 am on April 1, 2015: contributor

Although I’d prefer making the test consume less time, and adding it to the pull tester. What is so time consuming?

The test is probably far away from being optimal. :) While bitcoind is stopped as soon as possible, right now test_bitcoin and test_bitcoin-qt are executed four times each (with different locale settings), only to check, if the process terminates with exit status 0.

Edit: test_bitcoin does not terminate immediately without the environment setup/locale guard, but instead a few tests fail.

gavinandresen commented at 2:27 pm on April 1, 2015: contributor

RE: making the test faster: you can run just one bitcoind test suites or test cases using test_bitcoin –run_test=FOO (run all tests in FOO test suite) or –run_test=FOO/BAR (run the BAR test in the FOO test suite).

I believe multiple –run_test’s can be given if you want to run more than one.

dexX7 commented at 1:57 pm on April 2, 2015: contributor

@gavinandresen: thanks for the hint, this is very useful!

But just to clarify: the errors were not visible right after starting the Boost tests, and not all tests fail, but only a few, which use boost::path in one way or another. Further, the actual failures are sort of opaque, and the underlying issue is expressed in more than one way (see: bitcoin/jobs/56274515#L6165-L6228).

dexX7 commented at 8:04 pm on April 17, 2015: contributor

@laanwj: would you like a similar fix for 0.10, too? I sort of have a bad feeling, because I initially changed setenv("LC_ALL", "C", 1) to std::locale::global(std::locale("C")), which then turned out to be less reliable, and 0.10 currently still fails the test.

laanwj commented at 10:58 am on April 18, 2015: member

Yes, may be good idea

MarcoFalke locked this on Sep 8, 2021

Fix locale fallback and guard tests against invalid locale settings #5950