Differential D5297
fix ASAN error relating to nSigChecksBlockLimiter Authored by markblundeberg on Feb 15 2020, 10:03. Tags None Subscribers None
Details
See sanitizer log here, we have a use after scope: The order of destructor cleanup is reverse of declaration order. (D5179) ninja check-all ; run ASAN tests
Diff Detail
Event TimelineComment Actions Longer explanation: The script check threads (CScriptChecks) make use of resources that might not exist after ConnectBlock returns, notably the pointer to the transactions in the block. It's important that when ConnectBlock exits, all the script checking threads have stopped, and thus they will not try to access potentially dead variables. For this reason the CCheckQueueControl is declared, which means that any early return on failure from ConnectBlock will automatically cancel the checks and then wait for them to stop their work, before actually exiting the function. nSigChecksBlockLimiter is another such resource, and indeed even though it only exists in ConnectBlock, the CCheckQueueControl also ensures that all threads have stopped before the function exits, so one might think that nSigChecksBlockLimiter will not be written to after ConnectBlock exits. Totally true... however there's a catch: When return is called, C++ calls destructors in the reverse of the order of the variables' declarations. Thus, nSigChecksBlockLimiter destructor was being called before the CCheckQueueControl destructor was called, and thus the threads were still running even when nSigChecksBlockLimiter ceased to exist as a proper variable, even though the ConnectBlock stack frame was still alive. I knew about this stuff when designing the sigchecks mechanism, but by the time I got to finally writing D5179, I had forgotten about this important detail. :-( Due to how stack frames are generally implemented this wouldn't actually cause an exploit, but it's pretty bad thing anyway. Yet, ASAN helpfully picks up on things like this. So, yay for ASAN. |