test: Fix shutdown vptr race in BlockFilterIndexSync bench
What the developer wrote
Authored by MarcoFalke
test: Fix shutdown vptr race in BlockFilterIndexSync bench
What changed, and why it matters
This commit fixes a test-only race condition during shutdown of benchmark and unit tests that use Bitcoin Core's index objects. It adds a missing Stop() call in one benchmark and removes unnecessary synchronization comments/calls in several tests. The issue is not exploitable by users or network attackers; it only affects internal test code and could cause flaky or incorrect test results, including a potential virtual-pointer (vptr) race during benchmark teardown.
No production action needed. For developers, ensure all index tests and benchmarks follow the documented shutdown sequence (Stop() before destruction) and verify CI stability after removing the SyncWithValidationInterfaceQueue() calls.
Security signals we found
race condition in object shutdown/teardown
vptr corruption risk during concurrent destruction
missing Stop() in benchmark shutdown path
test-only code path, not production network code
Evidence from the diff
The patch addresses a shutdown race in BlockFilterIndexSync benchmark by adding filter_index.Stop() before the benchmark lambda exits, matching the normal Shutdown() sequence in init.cpp. It also removes SyncWithValidationInterfaceQueue() calls and associated comments from coinstatsindex_tests, txindex_tests, and adds a Stop() comment in txospenderindex_tests. The vptr race refers to the possibility of the index destructor running while background notification processing is still active, which can corrupt the object’s virtual table during teardown in test environments.
Changed components
src/bench/index_blockfilter.cppsrc/test/coinstatsindex_tests.cppsrc/test/txindex_tests.cppsrc/test/txospenderindex_tests.cppInspect captured patch +4 / −16
diff --git a/src/bench/index_blockfilter.cpp b/src/bench/index_blockfilter.cpp
index ea40b132..805ad050 100644
--- a/src/bench/index_blockfilter.cpp
+++ b/src/bench/index_blockfilter.cpp
@@ -53,6 +53,9 @@ static void BlockFilterIndexSync(benchmark::Bench& bench)
IndexSummary summary = filter_index.GetSummary();
assert(summary.synced);
assert(summary.best_block_hash == WITH_LOCK(::cs_main, return test_setup->m_node.chainman->ActiveTip()->GetBlockHash()));
+
+ // Shutdown sequence (c.f. Shutdown() in init.cpp)
+ filter_index.Stop();
});
}
diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp
index 74e5f454..9b32aabb 100644
--- a/src/test/coinstatsindex_tests.cpp
+++ b/src/test/coinstatsindex_tests.cpp
@@ -64,14 +64,6 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
BOOST_CHECK(block_index != new_block_index);
- // It is not safe to stop and destroy the index until it finishes handling
- // the last BlockConnected notification. The BlockUntilSyncedToCurrentChain()
- // call above is sufficient to ensure this, but the
- // SyncWithValidationInterfaceQueue() call below is also needed to ensure
- // TSAN always sees the test thread waiting for the notification thread, and
- // avoid potential false positive reports.
- m_node.validation_signals->SyncWithValidationInterfaceQueue();
-
// Shutdown sequence (c.f. Shutdown() in init.cpp)
coin_stats_index.Stop();
}
diff --git a/src/test/txindex_tests.cpp b/src/test/txindex_tests.cpp
index 82a60039..35947996 100644
--- a/src/test/txindex_tests.cpp
+++ b/src/test/txindex_tests.cpp
@@ -61,14 +61,6 @@ BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup)
}
}
- // It is not safe to stop and destroy the index until it finishes handling
- // the last BlockConnected notification. The BlockUntilSyncedToCurrentChain()
- // call above is sufficient to ensure this, but the
- // SyncWithValidationInterfaceQueue() call below is also needed to ensure
- // TSAN always sees the test thread waiting for the notification thread, and
- // avoid potential false positive reports.
- m_node.validation_signals->SyncWithValidationInterfaceQueue();
-
// shutdown sequence (c.f. Shutdown() in init.cpp)
txindex.Stop();
}
diff --git a/src/test/txospenderindex_tests.cpp b/src/test/txospenderindex_tests.cpp
index f890692d..44d2bf0e 100644
--- a/src/test/txospenderindex_tests.cpp
+++ b/src/test/txospenderindex_tests.cpp
@@ -70,6 +70,7 @@ BOOST_FIXTURE_TEST_CASE(txospenderindex_initial_sync, TestChain100Setup)
BOOST_CHECK_EQUAL((*tx_spender)->block_hash, tip_hash);
}
+ // Shutdown sequence (c.f. Shutdown() in init.cpp)
txospenderindex.Stop();
}
Why this scored 18/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.