test: Avoid accessing free'd memory in validation_chainstatemanager

MarcoFalke commented at 12:15 AM on April 13, 2020: member

No description provided.

MarcoFalke force-pushed on Apr 13, 2020

fanquake added the label Tests on Apr 13, 2020

MarcoFalke force-pushed on Apr 13, 2020

practicalswift commented at 5:17 PM on April 13, 2020: contributor

Concept ACK

Did any of the sanitizers find this issue?

MarcoFalke commented at 5:50 PM on April 13, 2020: member

Yes, this should be hit in sanitizers when additionally the validationinterface debug log category is enabled.

jamesob commented at 5:14 PM on April 14, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/18615/commits/fac4a9757b3bfd131a78570866c349b590f492ca

Thanks for fixing this.

in src/test/validation_chainstatemanager_tests.cpp:105 in fac4a9757b outdated

  97 | @@ -97,7 +98,13 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
  98 |      exp_tip = c1.m_chain.Tip();
  99 |      BOOST_CHECK_EQUAL(validated_tip, exp_tip);
 100 |  
 101 | -    // Avoid triggering the address sanitizer.
 102 | +    // Drop any events from the scheduler to avoid accessing memory that is going to be unloaded
 103 | +    m_node.scheduler->stop();
 104 | +    threadGroup.interrupt_all();
 105 | +    threadGroup.join_all();
 106 | +    GetMainSignals().FlushBackgroundCallbacks();

ryanofsky commented at 5:22 PM on April 14, 2020:

Could be wrong, but I'd think FlushBackgroundCallbacks should be called before scheduler->stop because flushing uses the scheduler

ryanofsky commented at 5:24 PM on April 14, 2020:

Never mind, I confused FlushBackgroundCallbacks with SyncWithValidationInterfaceQueue

in src/test/validation_chainstatemanager_tests.cpp:106 in fac4a9757b outdated

 102 | +    // Drop any events from the scheduler to avoid accessing memory that is going to be unloaded
 103 | +    m_node.scheduler->stop();
 104 | +    threadGroup.interrupt_all();
 105 | +    threadGroup.join_all();
 106 | +    GetMainSignals().FlushBackgroundCallbacks();
 107 | +    GetMainSignals().UnregisterBackgroundSignalScheduler();

ryanofsky commented at 6:33 PM on April 14, 2020:

Would just calling SyncWithValidationInterfaceQueue here instead of dropping notifications and cleaning everything up work?

If I'm understanding the bug correctly, the problem is a race condition where scheduled notifications from code above are racing to finish before manager.Unload() below. If this is the case, calling Sync should be a better fix because it would remove nondeterminism from the test (notifications non deterministically getting executing or dropped). Also it would avoid having cleanup code in the test that duplicates fixture cleanup code and runs twice

MarcoFalke commented at 3:06 PM on April 15, 2020:

Thanks. Done

ryanofsky approved

ryanofsky commented at 6:34 PM on April 14, 2020: member

Code review ACK fac4a9757b3bfd131a78570866c349b590f492ca, but see question below. I think this may not be the ideal fix

MarcoFalke force-pushed on Apr 15, 2020

in src/test/validation_chainstatemanager_tests.cpp:101 in fa4cb83303 outdated

  97 | @@ -97,7 +98,9 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
  98 |      exp_tip = c1.m_chain.Tip();
  99 |      BOOST_CHECK_EQUAL(validated_tip, exp_tip);
 100 |  
 101 | -    // Avoid triggering the address sanitizer.
 102 | +    // Drop any events from the scheduler to avoid accessing memory that is going to be unloaded

ryanofsky commented at 3:32 PM on April 15, 2020:

Would say "Let scheduler events finish running to avoid..." instead of "Drop any events from the scheduler to avoid..."

MarcoFalke commented at 3:46 PM on April 15, 2020:

Thanks, done

ryanofsky approved

ryanofsky commented at 3:33 PM on April 15, 2020: member

Code review ACK fa4cb833037a8a59dc511a9773aa5d64a416066e

test: Avoid accessing free'd memory in validation_chainstatemanager_tests fa176e253f

MarcoFalke force-pushed on Apr 15, 2020

ryanofsky approved

ryanofsky commented at 4:48 PM on April 15, 2020: member

Code review ACK fa176e253fb473767c61d4d8cd2d93e87d71a015, though if you have to update this again, would suggest separating txindex test cleanup and the chainstatemanager test fix in separate commits, or identifying which part of the change is the bugfix fix in the commit description. Also to clean up the txindex test it might make sense to call SyncWithValidationInterfaceQueue in the test destructor to prevent nondeterminism in other tests

MarcoFalke commented at 4:52 PM on April 15, 2020: member

Also to clean up the txindex test it might make sense to call SyncWithValidationInterfaceQueue in the test destructor to prevent nondeterminism in other tests

It needs to happen before the TestingSetup is destructed, because txindex is scoped inside the test case.

ryanofsky commented at 5:26 PM on April 15, 2020: member

It needs to happen before the TestingSetup is destructed, because txindex is scoped inside the test case.

Missed that, thanks.

MarcoFalke merged this on Apr 15, 2020

MarcoFalke closed this on Apr 15, 2020

MarcoFalke deleted the branch on Apr 15, 2020

Fabcien referenced this in commit 3d4316d11f on Dec 9, 2020

vijaydasmp referenced this in commit 1ec217b843 on Sep 5, 2021

DrahtBot locked this on Feb 15, 2022

test: Avoid accessing free'd memory in validation_chainstatemanager_tests #18615