Avoid crashes at shutdown due to printf() in global destructors. #1909

pull gavinandresen wants to merge 1 commits into bitcoin:master from gavinandresen:crash_at_exit changing 1 files +8 −2

gavinandresen commented at 8:36 PM on October 4, 2012: contributor

Fixes #1832
Avoid crashes at shutdown due to printf() in global destructors. cac6b389d1
in src/util.cpp:None in cac6b389d1
```
 226 | +            // This routine may be called by global destructors during shutdown.
 227 | +            // Since the order of destruction of static/global objects is undefined,
 228 | +            // allocate mutexDebugLog on the heap the first time this routine
 229 | +            // is called to avoid crashes during shutdown.
 230 | +            static boost::mutex* mutexDebugLog = NULL;
 231 | +            if (mutexDebugLog == NULL) mutexDebugLog = new boost::mutex();
```
sipa commented at 9:02 PM on October 4, 2012:

What if OutputDebugStringF is being called twice simultaneously? This doesn't solve anything, unless you have a guaranteed call of this function while still in single-thread modus. That's almost certainly the case, but if it is, why not make it obvious, and have an InitLogging() function, called in init?

I'm nitpicking. ACK.

laanwj commented at 5:27 AM on October 5, 2012:

@sipa I also proposed that in #1832. I'm for explicitly constructing and destructing the global objects, such as logging and db, enforced by assertions. This makes thiings more predictable and maintainable than armoring against all possible non-determinism (which is pretty much impossible, you will forget something).

Then again, it's good to have a fix in for 0.7.1. ACK.
BitcoinPullTester commented at 12:14 PM on October 5, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/cac6b389d101999d98c3137b17812cce062f924d for binaries and test log.
jgarzik commented at 10:09 PM on October 8, 2012: contributor

Yeah, it just seems terribly odd and possibly error-prone to allocate a lock... in a racy fashion.

Just make the lock global and put it early in main, to make sure it is instantiated/initialized before anything else in the program.

As it stands now, either this your change or without, the first-use occurs very late in the program, and that seems like a root cause (or at least contributing factor).
gavinandresen commented at 11:09 PM on October 8, 2012: contributor

@jgarzik making it global and putting it early in main won't fix the problem; the order of global destructors is undefined in C++.

As long as there is a printf/OutputDebugStringF before we start creating threads (and there is, early in AppInit2()) there is no race.

Reworking logging should be done... someday... For now, I think this little change is the right thing to do.
jgarzik commented at 11:41 PM on October 8, 2012: contributor

gcc has long followed the now defined C++0x order described here: http://cpp0x.centaur.ath.cx/basic.start.term.html

"If the completion of the constructor or dynamic initialization of an object with static storage duration is sequenced before that of another, the completion of the destructor of the second is sequenced before the initiation of the destructor of the first. If an object is initialized statically, the object is destroyed in the same order as if the object was dynamically initialized"
laanwj commented at 6:01 AM on October 9, 2012: member
I already acked this for 0.7.1 (assuming we want this out of the door soon), but it does need more thinking.

If you make the pointer global (i.s.o static) and explicitly initialize it in an InitLogging() (called directly at the beginning of AppInit2) function you avoid the destructor trouble, plus you don't make the safety of initialization dependent on who-calls-first. Then add an assert() to printf that the lock is allocated, just in case. Another plus is that you can use the same lock in #ifdef WIN32 if(fPrintToDebugger), where the issue still exists.

After all, printf does have some prerequisite requirements: it needs the arguments to be parsed and DataDir to be set correctly, which is only guaranteed when entering AppInit2. What if someone accidentally adds a printf before this is done? (ie, in GUI initialization, after all a printf looks reaaallly harmless):
- It logs to the wrong datadir (the default one). Not that big of an issue for a few log messages to end up in the wrong log. However,
- It caches this datadir in cachedPath, and will return it every time GetDataDir is called. Uh oh!
An explicit InitLogging() call would solve this, and trap all accidental calls to printf before everything is in order.

Talking of GetDataDir, it has the same issue. Though we jump the shark as it will never enter the lock when the path is cached, which you expect by the time it reaches destructors :-) But I can see another subtle initialization race, I think.
gavinandresen commented at 4:10 PM on October 9, 2012: contributor

Ok. I pinky-swear promise I'll rewrite this The Right Way for 0.8.
gavinandresen merged this on Oct 9, 2012
gavinandresen closed this on Oct 9, 2012
sipa commented at 4:12 PM on October 10, 2012: member

Maybe The Right Way means using http://www.boost.org/doc/libs/1_32_0/doc/html/call_once.html ?
KolbyML referenced this in commit 48e3c91e8c on Dec 5, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#1832 Crash at shutdown (global destructor order is undefined)