Bitcoin-Qt: fix crash on Windows caused by CDBEnv::EnvShutdown() #2024

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:fix_qt_crash changing 2 files +6 −2

Diapolo commented at 11:07 AM on November 18, 2012: none
- can be triggerd by just adding -proxy=crashme with 0.7.1
- crash occured, when AppInit2() was left with return false; after the first call to bitdb.open() (Step 6 in init)
- this is caused by GetDataDir() or .string() in CDBEnv::EnvShutdown() called via the bitdb global destructor
More details in #2025, as I'm not sure this fixes the underlying issue or just that single appearance of the bug.
laanwj commented at 11:17 AM on November 18, 2012: member

I don't understand how this can fix anything. The code should be equivalent. I suspect this is a memory corruption issue, and that by adding a variable you're moving some memory out of the way, probably moving the corruption issue to some place else.
laanwj commented at 11:23 AM on November 18, 2012: member

~~I think I understand the problem: fDbEnvInit seems to be never initialized to false when CDBEnv is created. It should be initialized to false in the constructor.~~

~~Otherwise, if the CDbEnv is created and destroyed without calling Open, something bad may happen in the destructor.~~

Edit: Hm no the CDBEnv object is created as global static object, so it should be zero-initialized. So that's not the cause either. Still, initializing it to false seems a good precaution to prevent problems in future refactorings.
laanwj commented at 11:37 AM on November 18, 2012: member

Another possibility would be a static destructor order issue. We've had those before with GetDataDir(). In which case this patch can actually solve the problem, by not calling GetDataDir()! But I thought that problem inside GetDataDir() was solved.

ACK anyway, this change can never hurt, storing the path inside the object is better encapsulation too.

Edit: But please also add the fDbEnvInit initialization.
Diapolo commented at 1:05 PM on November 18, 2012: none

The last time my debugger was working correctly, I saw that inside the Boost .string() code it was crashing. So this patch solves that by preventing the use of Boost filesystem in the destructor.

Bitcoin-Qt: fix crash on Windows caused by CDBEnv::EnvShutdown()

- can be triggerd by just adding -proxy=crashme with 0.7.1
- crash occured, when AppInit2() was left with return false; after the
  first call to bitdb.open() (Step 6 in init)
- this is caused by GetDataDir() or .string() in CDBEnv::EnvShutdown()
  called via the bitdb global destructor
- init fDbEnvInit and fMockDb to false in CDBEnv::CDBEnv()

be8e1f8479

Diapolo commented at 1:15 PM on November 18, 2012: none

@laanwj Could this be related https://svn.boost.org/trac/boost/ticket/6320?

Edit: Last rebase adds init of the private flags to false.

Edit 2: It seems the crash is coming from path.hpp (Boost filesystem).
sipa commented at 6:53 PM on November 18, 2012: member

Haven't tested, but I don't expect to see any difference on Linux. ACK
luke-jr commented at 2:06 PM on November 21, 2012: member

Looked over the implications visually. This actually fixes a bug on all platforms in theory, though in practice has no effect (besides destructor safety) since we only use CDBEnv for the same path. ACK
BitcoinPullTester commented at 9:31 AM on November 22, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/be8e1f8479ff46c99f97ac3e396736d36e5458c6 for binaries and test log.
laanwj referenced this in commit b0e228a4b0 on Nov 22, 2012
laanwj merged this on Nov 22, 2012
laanwj closed this on Nov 22, 2012
laudney referenced this in commit 5630792c29 on Mar 19, 2014
KolbyML referenced this in commit a307a26eb6 on Dec 5, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#2025 bitcoin-qt.exe crash with 0.7.1 (reproducible)