From 2316f5932acb19eb49773e1f8cd19500df88ec37 Mon Sep 17 00:00:00 2001 From: Kasper Munch Date: Wed, 1 Jul 2026 08:22:56 +0200 Subject: [PATCH] Fix use-after-free on Node's cached mutation iterator in write_state Node caches an iterator `it` into its `mutation_sites` map to speed up repeated positional lookups (get_state/move_iterator). write_state(pos, 0) erased the element by key and only *then* tested `it->first == pos` to decide whether to reset the cached iterator: mutation_sites.erase(pos); // invalidates `it` if it pointed at pos if (it->first == pos) { // <-- reads the just-freed tree node it = mutation_sites.begin(); } std::map::erase invalidates iterators to the erased element, so when `it` pointed at `pos` the subsequent `it->first` dereferenced a freed red-black tree node (use-after-free). The stale read usually did not equal `pos`, so `it` was left dangling; a later get_state -> move_iterator -> next(it)/prev(it) then walked a wild pointer and crashed with EXC_BAD_ACCESS / SIGSEGV. Because the crash only surfaces once the freed slot is reused, it manifested as intermittent heap corruption during MCMC sampling that scaled with ARG size x iterations and moved around with parameters/seed, rather than at the erase itself. Fix: decide before erasing. If `it` points at the element being removed, use the iterator-returning map::erase(it), which erases and repositions `it` to the following element in one step so it is never left dangling; otherwise erase by key and leave `it` untouched. The successor is always valid and interior (the INT_MAX sentinel is never erased), matching the invariant that move_iterator relies on. Co-Authored-By: Claude Opus 4.8 (1M context) --- SINGER/SINGER/Node.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/SINGER/SINGER/Node.cpp b/SINGER/SINGER/Node.cpp index b17c9f9..39930cb 100644 --- a/SINGER/SINGER/Node.cpp +++ b/SINGER/SINGER/Node.cpp @@ -31,9 +31,15 @@ double Node::get_state(double pos) { void Node::write_state(double pos, double s) { if (s == 0) { - mutation_sites.erase(pos); + // Erasing the element that the cached iterator `it` points at + // invalidates `it`. Detect that case *before* erasing (while `it` is + // still valid) and let map::erase(it) return the following element, so + // `it` is never left dangling. Reading `it->first` after erase(pos) + // would be a use-after-free. if (it->first == pos) { - it = mutation_sites.begin(); + it = mutation_sites.erase(it); + } else { + mutation_sites.erase(pos); } return; } else if (s == 1) {