From 7d8c327650827f455411362790497249839ce5f8 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Mon, 9 Mar 2026 15:57:49 +0700 Subject: [PATCH 1/4] fix: eliminate Owner poisoning in own_result by requiring value recovery on error The Owner::own_result method previously consumed the inner value and left the Owner in a poisoned state (inner = None) when the closure returned Err. While analysis confirmed this was not exploitable in the current codebase -- all error paths correctly drop the Walker without accessing the poisoned Owner -- the API was inherently fragile and could easily lead to panics from future code changes. This changes the own_result closure signature from FnOnce(T) -> Result to FnOnce(T) -> Result, requiring closures to return the value alongside the error. The Owner now restores the value on error, making poisoning impossible. Corresponding changes to TreeNode::attach and TreeNode::put_value variants ensure they return Self in the error tuple, enabling the recovery pattern throughout the Walker's tree operations. Co-Authored-By: Claude Opus 4.6 --- merk/src/owner.rs | 60 ++++++++++++++++++++-------- merk/src/tree/mod.rs | 95 ++++++++++++++++++++++---------------------- 2 files changed, 90 insertions(+), 65 deletions(-) diff --git a/merk/src/owner.rs b/merk/src/owner.rs index fdec527dc..6a64003e6 100644 --- a/merk/src/owner.rs +++ b/merk/src/owner.rs @@ -64,18 +64,29 @@ impl Owner { } /// Takes temporary ownership of the contained value by passing it to `f`. - /// The function must return a result of the same type (the same value, or a - /// new value to take its place). + /// The closure must return `Ok(T)` on success or `Err((T, E))` on failure. /// - /// # Warning + /// In both cases the `Owner` retains a valid value afterward -- on error, + /// the value bundled in the `Err` variant is restored, so the `Owner` is + /// **never** left in a poisoned state. /// - /// If `f` returns `Err`, the contained value has been consumed and the - /// `Owner` is left in a poisoned state (`inner = None`). Any subsequent - /// access (deref, `own`, `own_return`, etc.) will panic. Callers **must** - /// not use the `Owner` after `own_result` returns an error. + /// # Example + /// ``` + /// # use grovedb_merk::owner::Owner; + /// let mut owner = Owner::new(42); + /// let result = owner.own_result(|v| { + /// if v > 100 { + /// Ok(v + 1) + /// } else { + /// Err((v, "too small")) // value is returned alongside the error + /// } + /// }); + /// assert!(result.is_err()); + /// assert_eq!(*owner, 42); // Owner still holds the original value + /// ``` pub fn own_result(&mut self, f: F) -> Result<(), E> where - F: FnOnce(T) -> Result, + F: FnOnce(T) -> Result, { let old_value = unwrap(self.inner.take()); match f(old_value) { @@ -83,7 +94,10 @@ impl Owner { self.inner = Some(new_value); Ok(()) } - Err(e) => Err(e), + Err((restored_value, e)) => { + self.inner = Some(restored_value); + Err(e) + } } } @@ -112,8 +126,8 @@ fn unwrap(option: Option) -> T { Some(value) => value, None => panic!( "Owner is in a poisoned state (inner value is None). \ - This can happen if `own_result` was called and the closure returned Err, \ - consuming the value. The Owner must not be used after such an error." + This should never happen since `own_result` now requires \ + closures to return the value on error." ), } } @@ -125,17 +139,29 @@ mod tests { #[test] fn test_own_result_success_preserves_value() { let mut owner = Owner::new(42); - let result = owner.own_result(|v| Ok::<_, ()>(v + 1)); + let result = owner.own_result(|v| Ok::<_, (i32, ())>(v + 1)); assert!(result.is_ok()); assert_eq!(*owner, 43); } #[test] - #[should_panic(expected = "Owner is in a poisoned state")] - fn test_own_result_error_poisons_owner() { + fn test_own_result_error_restores_value() { + let mut owner = Owner::new(42); + let result = owner.own_result(|v| Err::((v, "fail"))); + assert!(result.is_err()); + // Owner still holds the original value -- no poisoning + assert_eq!(*owner, 42); + } + + #[test] + fn test_own_result_error_restores_modified_value() { let mut owner = Owner::new(42); - let _ = owner.own_result(|_v| Err::("fail")); - // Accessing the poisoned Owner should panic with a clear message - let _ = *owner; + let result = owner.own_result(|v| { + let modified = v + 10; + Err::((modified, "fail")) + }); + assert!(result.is_err()); + // Owner holds the value returned in the Err variant + assert_eq!(*owner, 52); } } diff --git a/merk/src/tree/mod.rs b/merk/src/tree/mod.rs index 624b463a1..bdc826f6f 100644 --- a/merk/src/tree/mod.rs +++ b/merk/src/tree/mod.rs @@ -755,7 +755,7 @@ impl TreeNode { /// Returns an error if there is already a child on the given side, /// indicating a corrupted tree state. #[inline] - pub fn attach(mut self, left: bool, maybe_child: Option) -> Result { + pub fn attach(mut self, left: bool, maybe_child: Option) -> Result { debug_assert_ne!( Some(self.key()), maybe_child.as_ref().map(|c| c.key()), @@ -765,11 +765,14 @@ impl TreeNode { let slot = self.slot_mut(left); if slot.is_some() { - return Err(Error::CorruptedState(if left { - "Tried to attach to left tree slot, but it is already Some" - } else { - "Tried to attach to right tree slot, but it is already Some" - })); + return Err(( + self, + Error::CorruptedState(if left { + "Tried to attach to left tree slot, but it is already Some" + } else { + "Tried to attach to right tree slot, but it is already Some" + }), + )); } *slot = Link::maybe_from_modified_tree(maybe_child); @@ -833,7 +836,7 @@ impl TreeNode { F: FnOnce(Option) -> Option, { let (tree, maybe_child) = self.detach(left); - tree.attach(left, f(maybe_child)) + tree.attach(left, f(maybe_child)).map_err(|(_tree, e)| e) } /// Like `walk`, but returns an error if there is no child on the given @@ -844,7 +847,7 @@ impl TreeNode { F: FnOnce(Self) -> Option, { let (tree, child) = self.detach_expect(left)?; - tree.attach(left, f(child)) + tree.attach(left, f(child)).map_err(|(_tree, e)| e) } /// Returns a mutable reference to the child slot for the given side. @@ -885,7 +888,7 @@ impl TreeNode { (StorageRemovedBytes, StorageRemovedBytes), Error, >, - ) -> CostResult { + ) -> CostContext> { let mut cost = OperationCost::default(); self.inner.kv = self.inner.kv.put_value_no_update_of_hashes(value); @@ -895,15 +898,14 @@ impl TreeNode { // we are replacing a value // in this case there is a possibility that the client would want to update the // element flags based on the change of values - cost_return_on_error_no_add!( - cost, - self.just_in_time_tree_node_value_update( - old_specialized_cost, - get_temp_new_value_with_old_flags, - update_tree_value_based_on_costs, - section_removal_bytes - ) - ); + if let Err(e) = self.just_in_time_tree_node_value_update( + old_specialized_cost, + get_temp_new_value_with_old_flags, + update_tree_value_based_on_costs, + section_removal_bytes, + ) { + return Err((self, e)).wrap_with_cost(cost); + } } self.inner.kv = self.inner.kv.update_hashes().unwrap_add_cost(&mut cost); @@ -939,7 +941,7 @@ impl TreeNode { (StorageRemovedBytes, StorageRemovedBytes), Error, >, - ) -> CostResult { + ) -> CostContext> { let mut cost = OperationCost::default(); self.inner.kv = self.inner.kv.put_value_with_fixed_cost_no_update_of_hashes( value, @@ -951,15 +953,14 @@ impl TreeNode { // we are replacing a value // in this case there is a possibility that the client would want to update the // element flags based on the change of values - cost_return_on_error_no_add!( - cost, - self.just_in_time_tree_node_value_update( - old_specialized_cost, - get_temp_new_value_with_old_flags, - update_tree_value_based_on_costs, - section_removal_bytes - ) - ); + if let Err(e) = self.just_in_time_tree_node_value_update( + old_specialized_cost, + get_temp_new_value_with_old_flags, + update_tree_value_based_on_costs, + section_removal_bytes, + ) { + return Err((self, e)).wrap_with_cost(cost); + } } self.inner.kv = self.inner.kv.update_hashes().unwrap_add_cost(&mut cost); @@ -995,7 +996,7 @@ impl TreeNode { (StorageRemovedBytes, StorageRemovedBytes), Error, >, - ) -> CostResult { + ) -> CostContext> { let mut cost = OperationCost::default(); self.inner.kv = self.inner.kv.put_value_no_update_of_hashes(value); @@ -1005,15 +1006,14 @@ impl TreeNode { // we are replacing a value // in this case there is a possibility that the client would want to update the // element flags based on the change of values - cost_return_on_error_no_add!( - cost, - self.just_in_time_tree_node_value_update( - old_specialized_cost, - get_temp_new_value_with_old_flags, - update_tree_value_based_on_costs, - section_removal_bytes - ) - ); + if let Err(e) = self.just_in_time_tree_node_value_update( + old_specialized_cost, + get_temp_new_value_with_old_flags, + update_tree_value_based_on_costs, + section_removal_bytes, + ) { + return Err((self, e)).wrap_with_cost(cost); + } } self.inner.kv = self @@ -1054,7 +1054,7 @@ impl TreeNode { (StorageRemovedBytes, StorageRemovedBytes), Error, >, - ) -> CostResult { + ) -> CostContext> { let mut cost = OperationCost::default(); self.inner.kv = self.inner.kv.put_value_with_fixed_cost_no_update_of_hashes( @@ -1067,15 +1067,14 @@ impl TreeNode { // we are replacing a value // in this case there is a possibility that the client would want to update the // element flags based on the change of values - cost_return_on_error_no_add!( - cost, - self.just_in_time_tree_node_value_update( - old_specialized_cost, - get_temp_new_value_with_old_flags, - update_tree_value_based_on_costs, - section_removal_bytes - ) - ); + if let Err(e) = self.just_in_time_tree_node_value_update( + old_specialized_cost, + get_temp_new_value_with_old_flags, + update_tree_value_based_on_costs, + section_removal_bytes, + ) { + return Err((self, e)).wrap_with_cost(cost); + } } self.inner.kv = self From 9ce668db790f9264b0cee698a9dfccc6b17e08fa Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Mon, 9 Mar 2026 16:12:07 +0700 Subject: [PATCH 2/4] fix: suppress clippy result_large_err for ownership-recovery error pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The (Self, Error) return pattern is intentional — it enables callers to recover the TreeNode on error. Add #[allow(clippy::result_large_err)] to the five functions using this pattern. Co-Authored-By: Claude Opus 4.6 --- merk/src/tree/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/merk/src/tree/mod.rs b/merk/src/tree/mod.rs index bdc826f6f..1529a9a43 100644 --- a/merk/src/tree/mod.rs +++ b/merk/src/tree/mod.rs @@ -755,6 +755,7 @@ impl TreeNode { /// Returns an error if there is already a child on the given side, /// indicating a corrupted tree state. #[inline] + #[allow(clippy::result_large_err)] pub fn attach(mut self, left: bool, maybe_child: Option) -> Result { debug_assert_ne!( Some(self.key()), @@ -863,6 +864,7 @@ impl TreeNode { /// Replaces the root node's value with the given value and returns the /// modified `Tree`. #[inline] + #[allow(clippy::result_large_err)] pub fn put_value( mut self, value: Vec, @@ -915,6 +917,7 @@ impl TreeNode { /// Replaces the root node's value with the given value and returns the /// modified `Tree`. #[inline] + #[allow(clippy::result_large_err)] pub fn put_value_with_fixed_cost( mut self, value: Vec, @@ -970,6 +973,7 @@ impl TreeNode { /// Replaces the root node's value with the given value and value hash /// and returns the modified `Tree`. #[inline] + #[allow(clippy::result_large_err)] pub fn put_value_and_reference_value_hash( mut self, value: Vec, @@ -1027,6 +1031,7 @@ impl TreeNode { /// Replaces the root node's value with the given value and value hash /// and returns the modified `Tree`. #[inline] + #[allow(clippy::result_large_err)] pub fn put_value_with_reference_value_hash_and_value_cost( mut self, value: Vec, From c1f6bfaa074855cd11eb2a3cd4c3dc7aad052839 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Mon, 9 Mar 2026 16:24:52 +0700 Subject: [PATCH 3/4] fix: suppress result_large_err on Walker methods using own_result closures The ownership-recovery pattern returns (Self, Error) tuples in closures passed to own_result, triggering clippy::result_large_err. This is by design for safe value recovery on error. Co-Authored-By: Claude Opus 4.6 --- merk/src/tree/walk/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/merk/src/tree/walk/mod.rs b/merk/src/tree/walk/mod.rs index b7f2788b4..6cf7f65b3 100644 --- a/merk/src/tree/walk/mod.rs +++ b/merk/src/tree/walk/mod.rs @@ -130,6 +130,7 @@ where /// Similar to `Tree#walk`, but yields a `Walker` which fetches from the /// same source as `self`. + #[allow(clippy::result_large_err)] pub fn walk( self, left: bool, @@ -158,6 +159,7 @@ where /// Similar to `Tree#walk_expect` but yields a `Walker` which fetches from /// the same source as `self`. + #[allow(clippy::result_large_err)] pub fn walk_expect( self, left: bool, @@ -210,6 +212,7 @@ where /// /// Returns an error if the slot is already occupied, indicating a /// corrupted tree state. + #[allow(clippy::result_large_err)] pub fn attach(mut self, left: bool, maybe_child: Option) -> Result where T: Into, @@ -220,6 +223,7 @@ where } /// Similar to `Tree#put_value`. + #[allow(clippy::result_large_err)] pub fn put_value( mut self, value: Vec, @@ -264,6 +268,7 @@ where } /// Similar to `Tree#put_value_with_fixed_cost`. + #[allow(clippy::result_large_err)] pub fn put_value_with_fixed_cost( mut self, value: Vec, @@ -310,6 +315,7 @@ where } /// Similar to `Tree#put_value_and_reference_value_hash`. + #[allow(clippy::result_large_err)] pub fn put_value_and_reference_value_hash( mut self, value: Vec, @@ -356,6 +362,7 @@ where } /// Similar to `Tree#put_value_with_reference_value_hash_and_value_cost`. + #[allow(clippy::result_large_err)] pub fn put_value_with_reference_value_hash_and_value_cost( mut self, value: Vec, From 0989fe03ae8149c0ca5ff83e8d276666f37f0b39 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Mon, 9 Mar 2026 16:40:11 +0700 Subject: [PATCH 4/4] docs: clarify own_result is error-safe but not unwind-safe Address CodeRabbit review: a panic inside the closure still leaves Owner in a poisoned state since the value is moved out before the call. Update docs to distinguish error-safety (Ok/Err paths) from unwind-safety. Co-Authored-By: Claude Opus 4.6 --- merk/src/owner.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/merk/src/owner.rs b/merk/src/owner.rs index 6a64003e6..26d1fb025 100644 --- a/merk/src/owner.rs +++ b/merk/src/owner.rs @@ -66,9 +66,13 @@ impl Owner { /// Takes temporary ownership of the contained value by passing it to `f`. /// The closure must return `Ok(T)` on success or `Err((T, E))` on failure. /// - /// In both cases the `Owner` retains a valid value afterward -- on error, - /// the value bundled in the `Err` variant is restored, so the `Owner` is - /// **never** left in a poisoned state. + /// **Error-safe:** In both `Ok` and `Err` paths the `Owner` retains a valid + /// value afterward -- on error, the value bundled in the `Err` variant is + /// restored. + /// + /// **Not unwind-safe:** If the closure panics, `Owner` will be left in a + /// poisoned state (inner = None) because the value was moved out before + /// the call and restoration happens only on normal return. /// /// # Example /// ``` @@ -126,8 +130,8 @@ fn unwrap(option: Option) -> T { Some(value) => value, None => panic!( "Owner is in a poisoned state (inner value is None). \ - This should never happen since `own_result` now requires \ - closures to return the value on error." + This can happen if a closure passed to `own_result` panicked \ + before returning Ok/Err." ), } }