From e276ccdaff5b04d8fcaf528a93444a45e84dd479 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 2 Sep 2026 04:38:03 +0000 Subject: [PATCH] Stop a read zipper from walking out of its own root `to_next_sibling_byte` guarded with `prefix_buf.len() == 0`, which asks "am I at the *map* root". The question is "am I at *this zipper's* root", which `at_root()` answers. Rooted anywhere else, the sibling step rewrote the last byte of `prefix_buf`, and at the zipper's root that byte belongs to the root path, so the zipper's own root moved: `origin_path` changed while `root_key_start` and `focus_node` went on describing the old one. `to_sibling`, which `to_prev_sibling_byte` routes through, had no guard at all. `ZipperMoving` documents the answer here as "did not move", and `ZipperHead` depends on it: it hands out zippers on the promise that each stays inside its subtrie. It is worse than a containment violation. Once `origin_path` has moved, `reset()` truncates to it and pops the ancestor stack, leaving the zipper deregularized; `descend_to` asserts on that in a debug build and loops forever in a release one, and `move_to_path` is `reset` then `descend_to`. The differential fuzzer hit this as two hangs per 2000 inputs. A second, smaller fix in the same method: the re-descent after a sibling step tested `key_bytes.len() == 1`, which expresses "landed at the end of the path within the node" only when the node key is a single byte. `node_key.len()` is `fixed_len + 1`, so that is the test. Regression tests: `read_zipper_sibling_step_never_leaves_its_root` covers both entry points, for roots that exist and roots that do not, and fails before this change. `read_zipper_sibling_steps_when_rooted_inside_a_node` exercises sibling steps and subsequent movement from zippers rooted at every depth inside a shared prefix; it did not reach the second condition on the shapes tried and is a guard rather than a reproduction of that half. lean/FINDINGS.md finding 3 on the lean-fuzzer-restage branch. Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BZmoASqM5FUuzvJeJaYQjR --- src/zipper.rs | 114 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 111 insertions(+), 3 deletions(-) diff --git a/src/zipper.rs b/src/zipper.rs index 99d2ac1..9aadd94 100644 --- a/src/zipper.rs +++ b/src/zipper.rs @@ -2257,7 +2257,13 @@ pub(crate) mod read_zipper_core { fn to_next_sibling_byte(&mut self) -> Option { timed_span!(ToNextSiblingByte, COUNTERS); self.prepare_buffers(); - if self.prefix_buf.len() == 0 { + //A sibling step rewrites the last byte of `prefix_buf`. At this zipper's *root* that + // byte belongs to the root path, so rewriting it moves the zipper's own root: + // `origin_path` changes while `root_key_start` and `focus_node` still describe the + // old one. `ZipperMoving` documents the answer here as "did not move", and + // `ZipperHead` depends on it -- a zipper must never leave the subtrie it was granted. + // This used to test `prefix_buf.len() == 0`, which asks "at the *map* root". + if self.at_root() { return None } debug_assert!(self.is_regularized()); @@ -2292,8 +2298,16 @@ pub(crate) mod read_zipper_core { self.focus_iter_token = new_tok; //If this operation landed us at the end of the path within the node, then we - // should re-regularize the zipper before returning - if key_bytes.len() == 1 { + // should re-regularize the zipper before returning. + // + //`node_key.len()` is `fixed_len + 1`, so this is "the sibling's key ends + // exactly where the node key does". It used to test `key_bytes.len() == 1`, + // which says the same thing only when the node key is a single byte -- i.e. + // when the zipper's root happens to sit on a node boundary. Rooted inside a + // node, `node_key` is longer, the descent was skipped, and the zipper was + // returned deregularized. `descend_to` asserts on that in a debug build and + // loops forever in a release one. + if key_bytes.len() == fixed_len + 1 { match child_node { None => {}, Some(rec) => { @@ -2954,6 +2968,12 @@ pub(crate) mod read_zipper_core { #[inline] fn to_sibling(&mut self, next: bool) -> Option { self.prepare_buffers(); + //Same contract as `to_next_sibling_byte`, and for the same reason: every arm below + // rewrites the last byte of `prefix_buf`, and at this zipper's root that byte belongs + // to the root path, so rewriting it moves the zipper's own root. + if self.at_root() { + return None + } debug_assert!(self.is_regularized()); if self.node_key().len() != 0 { match self.focus_node.get_sibling_of_child(self.node_key(), next) { @@ -5588,4 +5608,92 @@ mod tests { assert_eq!(rz.path(), b"rubicundus"); assert_eq!(&observed[..], rz.path()); } + + /// A sibling step at the zipper's own root must not move. `to_next_sibling_byte` + /// used to guard on the *map* root (`prefix_buf.len() == 0`) and `to_sibling` had no + /// guard at all, so at a zipper rooted anywhere else the step rewrote the last byte + /// of the root path: `at_root()` and `path()` still said "at my root" while + /// `origin_path()` had moved into a sibling subtrie the zipper was never granted. + /// `ZipperHead` relies on that containment, and the corrupt root also left the + /// zipper deregularized, so a later `descend_to` looped forever in release. + #[test] + fn read_zipper_sibling_step_never_leaves_its_root() { + let mut map = PathMap::::new(); + map.insert(&[0u8, 0, 1], 7); + map.insert(&[0u8, 0, 3], 161); + + //A root that does not exist, with an existing sibling on one side or both + for root in [&[0u8, 0, 2][..], &[0u8, 0, 0], &[0u8, 0, 4]] { + let mut rz = map.read_zipper_at_path(root); + assert!(!rz.path_exists()); + assert_eq!(rz.to_next_sibling_byte(), None, "root {root:?}"); + assert_eq!(rz.origin_path(), root, "root {root:?}"); + assert_eq!(rz.val(), None); + assert_eq!(rz.to_prev_sibling_byte(), None, "root {root:?}"); + assert_eq!(rz.origin_path(), root, "root {root:?}"); + assert_eq!(rz.val(), None); + assert!(rz.at_root()); + assert_eq!(rz.path(), &[]); + //The zipper must still be usable afterwards + rz.reset(); + assert!(!rz.path_exists()); + assert_eq!(rz.origin_path(), root); + } + + //A root that exists, with an existing sibling + for (root, val) in [(&[0u8, 0, 1][..], 7u64), (&[0u8, 0, 3], 161)] { + let mut rz = map.read_zipper_at_path(root); + assert_eq!(rz.to_next_sibling_byte(), None); + assert_eq!(rz.to_prev_sibling_byte(), None); + assert_eq!(rz.origin_path(), root); + assert_eq!(rz.val(), Some(&val)); + } + + //Below the root the step works as before, in both directions + let mut rz = map.read_zipper_at_path(&[0u8, 0]); + rz.descend_to(&[1u8]); + assert_eq!(rz.to_next_sibling_byte(), Some(3)); + assert_eq!(rz.val(), Some(&161)); + assert_eq!(rz.to_prev_sibling_byte(), Some(1)); + assert_eq!(rz.val(), Some(&7)); + assert_eq!(rz.origin_path(), &[0u8, 0, 1]); + } + + /// Sibling steps from zippers rooted at every depth inside a shared prefix, each + /// followed by further movement, so a step that hands back a deregularized zipper + /// trips the invariant here rather than somewhere downstream. (The re-descent + /// after a sibling step used to test `key_bytes.len() == 1`, which is "landed at + /// the end of the node key" only for a single-byte key; it now tests against + /// `node_key.len()`. This test did not reach that condition on the shapes tried, + /// so it guards the neighbourhood rather than reproducing that half.) + #[test] + fn read_zipper_sibling_steps_when_rooted_inside_a_node() { + let mut map = PathMap::::new(); + //Two long lines sharing a prefix, each with a fan-out below so that the + // subtrie under each sibling byte is a node of its own + for a in 0u8..4 { + for b in 0u8..4 { + map.insert(&[1u8, 1, 1, 1, 2, a, b], 20 + a as u64); + map.insert(&[1u8, 1, 1, 1, 3, a, b], 30 + a as u64); + } + } + //Roots at every depth inside the shared prefix + for root_len in 0..=4 { + let root = &[1u8, 1, 1, 1][..root_len]; + let mut rz = map.read_zipper_at_path(root); + rz.descend_to(&[1u8, 1, 1, 1, 2][root_len..]); + assert!(rz.path_exists(), "root {root:?}"); + assert_eq!(rz.to_next_sibling_byte(), Some(3), "root {root:?}"); + //Movement after the sibling step exercises the regularized invariant + rz.descend_to(&[2u8, 2]); + assert_eq!(rz.val(), Some(&32), "root {root:?}"); + rz.ascend(2); + assert_eq!(rz.to_prev_sibling_byte(), Some(2), "root {root:?}"); + rz.descend_to(&[3u8, 1]); + assert_eq!(rz.val(), Some(&23), "root {root:?}"); + rz.reset(); + rz.descend_to(&[1u8, 1, 1, 1, 3, 0, 0][root_len..]); + assert_eq!(rz.val(), Some(&30), "root {root:?}"); + } + } }