From 67e1dc37bff195d91f2959f3f4bf5d5d10da597c Mon Sep 17 00:00:00 2001 From: LLFourn Date: Tue, 19 Nov 2024 10:47:40 +1100 Subject: [PATCH 1/2] fix(core): Fix checkpoint Drop stack overflow Fixes #1634 --- crates/core/src/checkpoint.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/crates/core/src/checkpoint.rs b/crates/core/src/checkpoint.rs index 23d5731f9..9b5b0bbd4 100644 --- a/crates/core/src/checkpoint.rs +++ b/crates/core/src/checkpoint.rs @@ -21,6 +21,34 @@ struct CPInner { prev: Option>, } +/// When a `CPInner` is dropped we need to go back down the chain and manually remove any +/// no-longer referenced checkpoints. Letting the default rust dropping mechanism handle this +/// leads to recursive logic and stack overflows +/// +/// https://github.com/bitcoindevkit/bdk/issues/1634 +impl Drop for CPInner { + fn drop(&mut self) { + // Take out `prev` so its `drop` won't be called when this drop is finished + let mut current = self.prev.take(); + while let Some(arc_node) = current { + // Get rid of the Arc around `prev` if we're the only one holding a ref + // So the `drop` on it won't be called when the `Arc` is dropped. + // + // FIXME: When MSRV > 1.70.0 this should use Arc::into_inner which actually guarantees + // that no recursive drop calls can happen even with multiple threads. + match Arc::try_unwrap(arc_node).ok() { + Some(mut node) => { + // Keep goign backwards + current = node.prev.take(); + // Don't call `drop` on `CPInner` since that risks it becoming recursive. + core::mem::forget(node); + } + None => break, + } + } + } +} + impl PartialEq for CheckPoint { fn eq(&self, other: &Self) -> bool { let self_cps = self.iter().map(|cp| cp.block_id()); From 2df5861c5e1bf308f1b215b19e00d882aee07e87 Mon Sep 17 00:00:00 2001 From: valued mammal Date: Tue, 19 Nov 2024 10:43:15 -0500 Subject: [PATCH 2/2] test(core): test `Drop` implementation of `CPInner` Check that dropping `CheckPoint` does not cause nasty behavior by creating a large linked list in memory and then destroying it. --- crates/core/src/checkpoint.rs | 2 +- crates/core/tests/test_checkpoint.rs | 23 +++++++++++++++++++++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/crates/core/src/checkpoint.rs b/crates/core/src/checkpoint.rs index 9b5b0bbd4..9bfdf2eed 100644 --- a/crates/core/src/checkpoint.rs +++ b/crates/core/src/checkpoint.rs @@ -38,7 +38,7 @@ impl Drop for CPInner { // that no recursive drop calls can happen even with multiple threads. match Arc::try_unwrap(arc_node).ok() { Some(mut node) => { - // Keep goign backwards + // Keep going backwards current = node.prev.take(); // Don't call `drop` on `CPInner` since that risks it becoming recursive. core::mem::forget(node); diff --git a/crates/core/tests/test_checkpoint.rs b/crates/core/tests/test_checkpoint.rs index 2ec96c153..a5194e5b9 100644 --- a/crates/core/tests/test_checkpoint.rs +++ b/crates/core/tests/test_checkpoint.rs @@ -1,5 +1,5 @@ -use bdk_core::CheckPoint; -use bdk_testenv::block_id; +use bdk_core::{BlockId, CheckPoint}; +use bdk_testenv::{block_id, hash}; /// Inserting a block that already exists in the checkpoint chain must always succeed. #[test] @@ -32,3 +32,22 @@ fn checkpoint_insert_existing() { } } } + +#[test] +fn checkpoint_destruction_is_sound() { + // Prior to the fix in https://github.com/bitcoindevkit/bdk/pull/1731 + // this could have caused a stack overflow due to drop recursion in Arc. + // We test that a long linked list can clean itself up without blowing + // out the stack. + let mut cp = CheckPoint::new(BlockId { + height: 0, + hash: hash!("g"), + }); + let end = 10_000; + for height in 1u32..end { + let hash = bitcoin::hashes::Hash::hash(height.to_be_bytes().as_slice()); + let block = BlockId { height, hash }; + cp = cp.push(block).unwrap(); + } + assert_eq!(cp.iter().count() as u32, end); +}