Page MenuHomePhabricator

fix ASAN error relating to nSigChecksBlockLimiter
ClosedPublic

Authored by markblundeberg on Feb 15 2020, 10:03.

Details

Summary

See sanitizer log here, we have a use after scope:
https://build.bitcoinabc.org/viewLog.html?buildId=29430&buildTypeId=BitcoinABC_Master_BitcoinAbcMasterAsan&tab=artifacts

The order of destructor cleanup is reverse of declaration order.
Thus we need nSigChecksBlockLimiter to be declared before control which is the RAII on all script check worker threads having finished their work.

(D5179)

Test Plan

ninja check-all ; run ASAN tests

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fixup
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9511
Build 16934: Default Diff Build & Tests
Build 16933: arc lint + arc unit

Event Timeline

markblundeberg retitled this revision from fix ASAN error to fix ASAN error relating to nSigChecksBlockLimiter.Feb 15 2020, 10:06
markblundeberg edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Feb 15 2020, 10:23

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.