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

1. 
   MarcoFalke commented at 0:15 am on April 13, 2020: member
2. MarcoFalke force-pushed on Apr 13, 2020
3. fanquake added the label Tests on Apr 13, 2020
4. MarcoFalke force-pushed on Apr 13, 2020
5. MarcoFalke force-pushed on Apr 13, 2020
6. MarcoFalke force-pushed on Apr 13, 2020
7. 
   practicalswift commented at 5:17 pm on April 13, 2020: contributor

   Concept ACK

   Did any of the sanitizers find this issue?

8. 
   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.
9. 
   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.

10. 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
11. 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
12. ryanofsky approved
13. 
    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
14. MarcoFalke force-pushed on Apr 15, 2020
15. 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
16. ryanofsky approved
17. 
    ryanofsky commented at 3:33 pm on April 15, 2020: member

    Code review ACK fa4cb833037a8a59dc511a9773aa5d64a416066e
18. test: Avoid accessing free'd memory in validation_chainstatemanager_tests fa176e253f
19. MarcoFalke force-pushed on Apr 15, 2020
20. ryanofsky approved
21. 
    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
22. 
    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.

23. 
    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.

24. MarcoFalke merged this on Apr 15, 2020
25. MarcoFalke closed this on Apr 15, 2020

    ---

26. MarcoFalke deleted the branch on Apr 15, 2020
27. Fabcien referenced this in commit 3d4316d11f on Dec 9, 2020
28. vijaydasmp referenced this in commit 1ec217b843 on Sep 5, 2021
29. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke practicalswift jamesob ryanofsky

Labels
Tests