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

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2004-testNoAccessFreeMem changing 2 files +8 −9

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
MarcoFalke force-pushed 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: contributor

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: contributor

Code review ACK fa4cb833037a8a59dc511a9773aa5d64a416066e
test: Avoid accessing free'd memory in validation_chainstatemanager_tests fa176e253f
MarcoFalke force-pushed on Apr 15, 2020
jamesob cross-referenced this on Apr 15, 2020 from issue coins: allow cache resize after init by jamesob
ryanofsky approved
ryanofsky commented at 4:48 PM on April 15, 2020: contributor

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: contributor

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 cross-referenced this on Apr 15, 2020 from issue fuzz: Disable debug log file by MarcoFalke
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
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18571 fuzz: Disable debug log file #18637 coins: allow cache resize after init