From 9ec822f848edeccc9117bd4b2b413ea620c7a8a9 Mon Sep 17 00:00:00 2001 From: Joe Rivera Date: Tue, 29 Sep 2026 20:16:58 -0500 Subject: [PATCH 1/3] feat(state): owning accessors + pinned resolveLink; fix state:// cross-node UAF (Phase 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 / Option A of docs/roadmap/STATE_LIFETIME_AND_ATOMICITY.md. cvc::state's operator()/findDescendant hand back a bare state&/state* whose owner is the parent's child map, so a concurrent sweepExpired() can free a node under a caller still holding the bare reference — a cross-node use-after-free (confirmed for a state:// resolve running on a compute-pool worker while a tree-wide sweep runs on the scheduler/pycvc thread). Core (state.h/state.cpp): - state now derives boost::enable_shared_from_this (boost variant, to match state_ptr = boost::shared_ptr). Every live node is already shared_ptr-owned (root via instancePtr, children via the map), so this needs no minting changes. - findDescendantShared(path): owning analogue of findDescendant — copies the map's shared_ptr at each hop, so the returned node is PINNED (a concurrent sweep can only unlink it, never free it under the caller). - sharedChild(name): owning create-or-get analogue of operator(), same lock/create semantics, returns the pinning state_ptr. - state::handle: thin RAII pin (operator-> / operator*). - resolveLink now pins its walk (each hop held as a state_ptr via findDescendantShared) and returns the terminal pinned in link_resolution::target_owned. Resolver (uri_state.cpp): state_resolve/state_store now PIN the addressed + effective node and derive the canonical path from strings (the input path, or resolveLink's captured visited chain) instead of a post-resolution eff->fullName() — which walked the node's _parent chain and UAF'd if a concurrent sweep had orphaned it (the confirmed H1). Residual (documented): the pin covers a node's own storage, NOT its ancestor chain, so fullName()/parentName() on a pinned-but-orphaned node stays unsafe; resolveLink's internal fullName() during the walk is the narrower remaining window, tracked with the fullName hardening. operator()/findDescendant are unchanged (opt-in migration). Test: state_lifetime_stress_test.cpp — PinDefersDestruction (sweep unlinks synchronously but a pin defers ~state/destroyed), PinnedLeafSurvivesAncestorSweep (the H1 core: a pinned leaf reads safely after its intermediate ancestor is swept), SharedChildCreatesAndPins, and ConcurrentPinnedReadVsSweep (4 readers pin+read while a sweeper churns the subtree; no UAF). All pass locally; run under TSan (setarch -R) + ASan in CI. --- inc/cvc/core/state.h | 47 +++++- src/cvc/ariadne/uri_state.cpp | 50 ++++-- src/cvc/core/state.cpp | 85 ++++++++++- src/cvc/tests/CMakeLists.txt | 10 ++ src/cvc/tests/state_lifetime_stress_test.cpp | 153 +++++++++++++++++++ 5 files changed, 325 insertions(+), 20 deletions(-) create mode 100644 src/cvc/tests/state_lifetime_stress_test.cpp diff --git a/inc/cvc/core/state.h b/inc/cvc/core/state.h index 90d50ade..a6b64f96 100644 --- a/inc/cvc/core/state.h +++ b/inc/cvc/core/state.h @@ -28,10 +28,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -147,7 +149,14 @@ template class state_future { // 01/12/2014 -- Joe R. -- Added init_funcs and json() // 01/13/2014 -- Joe R. -- Removing notifyXmlRpc() once and for all. // 12/08/2025 -- Added futures API for async value retrieval. -class state { +// +// enable_shared_from_this (boost variant, matching state_ptr = boost::shared_ptr): every +// live node is already shared_ptr-owned (the root via instancePtr(), children via the _children +// map), so a node can hand out its own owning pointer. This backs the owning accessors +// (findDescendantShared / sharedChild / handle) that let a caller PIN a node across a concurrent +// sweepExpired() — the bare state& / state* that operator() / findDescendant return do NOT keep the +// node alive, so a concurrent structural mutation can free it under the caller (a cross-node UAF). +class state : public boost::enable_shared_from_this { public: typedef boost::shared_ptr state_ptr; typedef std::map child_map; @@ -510,6 +519,10 @@ class state { struct link_resolution { link_resolution_kind kind = link_resolution_kind::resolved; state *target = nullptr; + // Owning pin of `target` (non-null exactly when `target` is): resolveLink walks and returns the + // terminal held as a state_ptr, so a caller that keeps `target_owned` (or the whole result) + // alive cannot have the resolved node freed under it by a concurrent sweepExpired(). + state_ptr target_owned; std::vector visited; // ordered absolute paths std::size_t hops = 0; }; @@ -605,6 +618,38 @@ class state { // Useful for link resolution and any other read-only navigation. state *findDescendant(const std::string &path); + // Owning analogues of findDescendant() and operator(): they return the map's shared_ptr rather + // than a bare state* / state&, so the returned node stays alive for as long as the caller holds + // the returned state_ptr (or a `handle` wrapping it) — a concurrent sweepExpired() can then only + // UNLINK the node from its parent, never free it under the caller. Use these (not the bare + // accessors) whenever a node is read/used on a thread that a tree-wide sweep could run against + // (e.g. a compute-pool worker resolving `state://…`). NOTE: the pin covers the returned node's + // own storage (value/data/children/mutex); it does NOT pin the node's ancestor chain, so + // fullName() / parentName() on a node whose ancestor was concurrently swept is still unsafe — + // avoid those on a pinned-but-possibly-orphaned node (see + // docs/roadmap/STATE_LIFETIME_AND_ATOMICITY.md). + // + // findDescendantShared: read-only, returns a null state_ptr when any segment is absent. + // sharedChild: create-or-get (like operator()), returns the pinning state_ptr for the terminal. + state_ptr findDescendantShared(const std::string &path); + state_ptr sharedChild(const std::string &childname = std::string()); + + // RAII sugar: a movable handle that reads like a node (operator-> / operator*) while pinning it + // alive. `handle h = node.sharedChild("a.b"); h->value("x");` keeps a.b alive for h's lifetime. + class handle { + public: + handle() = default; + explicit handle(state_ptr p) : _p(std::move(p)) {} + state *operator->() const { return _p.get(); } + state &operator*() const { return *_p; } + state *get() const { return _p.get(); } + const state_ptr &ptr() const { return _p; } + explicit operator bool() const { return static_cast(_p); } + + private: + state_ptr _p; + }; + // -------- Phase 8 slice 6: pull-on-demand remote link resolution -------- // // resolveRemote() extends resolveLink() by consulting the diff --git a/src/cvc/ariadne/uri_state.cpp b/src/cvc/ariadne/uri_state.cpp index a0eb668d..2513ce15 100644 --- a/src/cvc/ariadne/uri_state.cpp +++ b/src/cvc/ariadne/uri_state.cpp @@ -27,18 +27,32 @@ std::string channel_of(const std::string &query) { return ch; } +// The effective (link-followed) node, PINNED, plus its canonical absolute path. `node` is held as +// an owning state_ptr and `path` is captured from strings we already hold (the addressed path, or +// resolveLink's visited chain) — never a post-resolution fullName() on `node`, which would walk the +// node's _parent chain and UAF if a concurrent sweepExpired() orphaned it (the confirmed cross-node +// hazard for a `state://` resolve on a compute-pool worker). +struct effective { + cvc::state::state_ptr node; + std::string path; +}; + // Follow a TRANSPARENT link to its terminal target; a non-link / opaque / broken / cyclic // transparent link stays put (resolvedValue's fallback). SHARED by read and write so both address // the identical node — crucially even a DEFAULT (non-writable) transparent link: a read follows it, // so a write must follow it too, or a store would be shadowed on the link node and unreadable via // the same URI (the write-through routing in state::value() only fires for a WRITABLE link). -cvc::state *effective_node(cvc::state *node) { +// `addressed_path` is the canonical path of `node` itself (its normalized `state://` path), used +// when the node is not a followed link. +effective effective_node(cvc::state::state_ptr node, const std::string &addressed_path) { if (node && node->isLink() && node->linkMode() == cvc::state::link_mode::transparent) { const cvc::state::link_resolution lr = node->resolveLink(); - if (lr.kind == cvc::state::link_resolution_kind::resolved && lr.target) - return lr.target; + if (lr.kind == cvc::state::link_resolution_kind::resolved && lr.target_owned) + // target_owned pins the terminal; visited.back() is its absolute path captured during the + // walk (safe), avoiding a fullName() on a possibly-orphaned node. + return {lr.target_owned, lr.visited.empty() ? addressed_path : lr.visited.back()}; } - return node; + return {std::move(node), addressed_path}; } // The `?value` / `?data` channels are served; `?children` and anything else are not (they need the @@ -56,18 +70,22 @@ UriResult state_resolve(cvc::state &root, const Uri &u) { "' is not served by the built-in state handler (only '?value' / '?data'); register " "a custom handler for '?children'"}; - // Navigate WITHOUT creating nodes; an empty path is the root itself. - cvc::state *node = u.path.empty() ? &root : root.findDescendant(u.path); + // Navigate WITHOUT creating nodes; an empty path is the root itself. PIN the addressed node so a + // concurrent sweepExpired() (on the scheduler/pycvc thread) cannot free it while this resolve — + // which may run on a compute-pool worker — reads its value/data below. + cvc::state::state_ptr node = + u.path.empty() ? root.shared_from_this() : root.findDescendantShared(u.path); if (!node) return {false, std::string(), std::string(), "ari: state node '" + u.path + "' not found"}; - cvc::state *eff = effective_node(node); // read follows a transparent link to its target - const std::string canonical = "state://" + eff->fullName() + "?" + channel; + const effective eff = + effective_node(node, u.path); // read follows a transparent link to its target + const std::string canonical = "state://" + eff.path + "?" + channel; if (channel == "data") { // The data channel carries a raw string/byte blob (what state_store writes here, and what the // §13.9 HTTP cache parks on a node). Typed data() payloads (a value_t / geometry) are a // different consumer (state-data-get) and are not byte-serialized here. - const boost::any d = eff->data(); + const boost::any d = eff.node->data(); if (const std::string *s = boost::any_cast(&d)) return {true, *s, canonical, std::string()}; if (d.empty()) @@ -75,7 +93,7 @@ UriResult state_resolve(cvc::state &root, const Uri &u) { return {false, std::string(), std::string(), "ari: state '" + u.path + "?data' holds a non-string payload (not byte-serializable)"}; } - return {true, eff->value(), canonical, std::string()}; + return {true, eff.node->value(), canonical, std::string()}; } // §13.10 the write analogue: store `content` into the addressed node's `?value` (default) or @@ -89,12 +107,16 @@ StoreResult state_store(cvc::state &root, const Uri &u, const std::string &conte return {false, std::string(), "ari: state channel '?" + channel + "' is not writable by the built-in state handler (only '?value' / '?data')"}; - cvc::state *eff = effective_node(u.path.empty() ? &root : &root(u.path)); + // sharedChild CREATES the path if absent (a store may target a not-yet-existing node) and PINS + // the terminal, so the write below is safe against a concurrent sweep — same rationale as the + // read. + cvc::state::state_ptr node = u.path.empty() ? root.shared_from_this() : root.sharedChild(u.path); + const effective eff = effective_node(node, u.path); if (channel == "data") - eff->data(boost::any(content)); // store the bytes as a string blob on the data channel + eff.node->data(boost::any(content)); // store the bytes as a string blob on the data channel else - eff->value(content); - return {true, "state://" + eff->fullName() + "?" + channel, std::string()}; + eff.node->value(content); + return {true, "state://" + eff.path + "?" + channel, std::string()}; } } // namespace diff --git a/src/cvc/core/state.cpp b/src/cvc/core/state.cpp index e1318077..53553b46 100644 --- a/src/cvc/core/state.cpp +++ b/src/cvc/core/state.cpp @@ -784,6 +784,47 @@ state &state::operator()(const std::string &childname) { return (*child)(join(keys, SEPARATOR)); } +// state::sharedChild +// ----------------- +// Owning create-or-get analogue of operator(): walks (creating as needed) the child path, holding +// each hop as a state_ptr, and returns the terminal PINNED. Same create/lock semantics as +// operator() (child under the parent's lock, released before descending); the difference is the +// return type — the caller gets an owning pointer that keeps the node alive across a concurrent +// sweepExpired(), where operator()'s bare state& would dangle. +state::state_ptr state::sharedChild(const std::string &childname) { + using namespace boost::algorithm; + + boost::this_thread::interruption_point(); + + std::vector keys; + split(keys, childname, is_any_of(SEPARATOR)); + BOOST_FOREACH (std::string &key, keys) + trim(key); + while (!keys.empty() && keys.front().empty()) + keys.erase(keys.begin()); + + state_ptr cur = shared_from_this(); + BOOST_FOREACH (std::string &key, keys) { + if (key.empty()) + continue; // skip empty interior/trailing segments, as operator() does via its leading-drop + state_ptr next; + { + boost::mutex::scoped_lock lock(cur->_mutex); + auto it = cur->_children.find(key); + if (it != cur->_children.end() && it->second) { + next = it->second; + } else { + next.reset(new state(cur->_ctx, key, cur.get())); + cur->_children[key] = next; + cur->_lastMod = boost::posix_time::microsec_clock::universal_time(); + cur->_initialized = true; + } + } + cur = next; + } + return cur; +} + // ---------------- // state::linkTo / clearLink / isLink / linkTarget / resolveLink // ---------------- @@ -999,6 +1040,31 @@ state *state::findDescendant(const std::string &path) { return cur; } +state::state_ptr state::findDescendantShared(const std::string &path) { + using namespace boost::algorithm; + std::string normalized = normalize_state_path(path); + if (normalized.empty()) + return shared_from_this(); + std::vector keys; + split(keys, normalized, is_any_of(SEPARATOR)); + state_ptr cur = shared_from_this(); + for (auto &k : keys) { + trim(k); + if (k.empty()) + continue; + state_ptr next; + { + boost::mutex::scoped_lock lock(cur->_mutex); + auto it = cur->_children.find(k); + if (it == cur->_children.end() || !it->second) + return state_ptr(); + next = it->second; // copy the OWNING shared_ptr — pins this hop past a concurrent sweep + } + cur = next; + } + return cur; +} + state::link_resolution state::resolveLink(std::size_t hop_budget) { link_resolution result; @@ -1009,7 +1075,14 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { // when multi-tree lands the key extends to (tree_id, path). std::unordered_set seen; - state *cur = this; + // Pin the walk: hold each hop as a state_ptr (start via shared_from_this(), each subsequent hop + // via findDescendantShared) so a concurrent sweepExpired() cannot free the node we are reading + // the link target / path from. The terminal is returned pinned in result.target_owned, so a + // caller that keeps the result can safely use `target` after we return. (Residual: fullName() + // below still walks each hop's unpinned ANCESTOR chain — a narrower window tracked with the + // fullName hardening in docs/roadmap/STATE_LIFETIME_AND_ATOMICITY.md.) + state_ptr cur_owned = shared_from_this(); + state *cur = cur_owned.get(); // Record the starting node's path so cycles that loop back to // the start (including a self-link) are detected as cycles // rather than mistakenly classified as "resolved". @@ -1026,6 +1099,7 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { // Terminal node: not a link. result.kind = (cur == this) ? link_resolution_kind::none : link_resolution_kind::resolved; result.target = cur; + result.target_owned = cur_owned; // pin the resolved terminal for the caller return result; } @@ -1035,8 +1109,8 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { return result; } - state *next = root.findDescendant(target); - if (next == nullptr) { + state_ptr next_owned = root.findDescendantShared(target); + if (!next_owned) { result.kind = link_resolution_kind::broken; result.target = nullptr; result.visited.push_back(target); @@ -1044,7 +1118,7 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { } ++result.hops; - std::string next_path = next->fullName(); + std::string next_path = next_owned->fullName(); if (!seen.insert(next_path).second) { result.kind = link_resolution_kind::cycle_detected; result.target = nullptr; @@ -1052,7 +1126,8 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { return result; } result.visited.push_back(next_path); - cur = next; + cur_owned = next_owned; + cur = cur_owned.get(); } } diff --git a/src/cvc/tests/CMakeLists.txt b/src/cvc/tests/CMakeLists.txt index cde17d06..d148fb91 100644 --- a/src/cvc/tests/CMakeLists.txt +++ b/src/cvc/tests/CMakeLists.txt @@ -11,6 +11,7 @@ add_executable(ariadne_bind_test ariadne_bind_test.cpp) add_executable(net_http_client_test net_http_client_test.cpp) add_executable(uri_http_cache_test uri_http_cache_test.cpp) add_executable(state_test state_test.cpp) +add_executable(state_lifetime_stress_test state_lifetime_stress_test.cpp) add_executable(state_change_journal_test state_change_journal_test.cpp) add_executable(state_subscription_router_test state_subscription_router_test.cpp) add_executable(state_sync_adapter_test state_sync_adapter_test.cpp) @@ -169,6 +170,7 @@ set(TEST_TARGETS net_http_client_test uri_http_cache_test state_test + state_lifetime_stress_test state_change_journal_test state_subscription_router_test state_sync_adapter_test @@ -429,6 +431,13 @@ target_link_libraries(state_test GTest::gtest_main ) +target_link_libraries(state_lifetime_stress_test + PRIVATE + cvc + GTest::gtest + GTest::gtest_main +) + target_link_libraries(voxels_test PRIVATE cvc @@ -1596,6 +1605,7 @@ cvc_discover_tests(ariadne_bind_test) cvc_discover_tests(net_http_client_test) cvc_discover_tests(uri_http_cache_test) cvc_discover_tests(state_test) +cvc_discover_tests(state_lifetime_stress_test) cvc_discover_tests(voxels_test) cvc_discover_tests(volume_test) cvc_discover_tests(volren_math_test) diff --git a/src/cvc/tests/state_lifetime_stress_test.cpp b/src/cvc/tests/state_lifetime_stress_test.cpp new file mode 100644 index 00000000..c711bda0 --- /dev/null +++ b/src/cvc/tests/state_lifetime_stress_test.cpp @@ -0,0 +1,153 @@ +/* + Copyright 2026 The University of Texas at Austin + + This file is part of libcvc. + + libcvc is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License version 2.1 as published by the Free Software Foundation. +*/ + +// Cross-node lifetime tests for the owning accessors (findDescendantShared / sharedChild / handle) +// and the pin they provide against a concurrent sweepExpired(). +// +// The bare accessors operator() / findDescendant hand back a state& / state* whose owner is the +// parent's child map, so a concurrent sweepExpired() (or reset(false)) can free the node under a +// caller that still holds the bare reference — a use-after-free. The owning accessors return the +// map's shared_ptr, which PINS the node: a concurrent sweep can then only unlink it, never free it. +// See docs/roadmap/STATE_LIFETIME_AND_ATOMICITY.md (Phase 1 / Option A). +// +// Run the concurrent case under TSan/ASan for the strongest signal (this repo needs `setarch -R` to +// disable ASLR under TSan — see the TSan gotcha in the memory index). + +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +using cvc::state; +namespace pt = boost::posix_time; + +pt::ptime now_utc() { return pt::microsec_clock::universal_time(); } + +} // namespace + +// A pin defers destruction: sweepExpired() unlinks the node from the tree synchronously, but the +// node itself is not freed (and `destroyed` does not fire) until the last owning pin drops. This is +// the one observable behavior change of Option A, and it is what keeps a pinned node safe. +TEST(StateLifetime, PinDefersDestruction) { + cvc::app a; + auto &root = state::instance(a); + root("a.gone").value("V"); + + std::atomic destroyed{0}; + root("a.gone").destroyed.connect([&]() { destroyed.fetch_add(1); }); + + state::state_ptr pin = root.findDescendantShared("a.gone"); + ASSERT_TRUE(static_cast(pin)); + + root("a.gone").expireAt(now_utc() - pt::seconds(1)); + root.sweepExpired(); + + EXPECT_EQ(root.findDescendant("a.gone"), nullptr) << "swept node must be unlinked immediately"; + EXPECT_EQ(destroyed.load(), 0) << "a pinned node must not be destroyed while a handle holds it"; + EXPECT_EQ(pin->value(), "V") << "the pinned node's own storage stays readable after its unlink"; + + pin.reset(); + EXPECT_EQ(destroyed.load(), 1) << "destruction is deferred to the last owning reference"; +} + +// The confirmed cross-node hazard (H1): a pinned LEAF stays readable even after its INTERMEDIATE +// ancestor is swept. With a bare state* the value() read below would be a use-after-free. +TEST(StateLifetime, PinnedLeafSurvivesAncestorSweep) { + cvc::app a; + auto &root = state::instance(a); + root("s.item.leaf").value("V"); + + state::state_ptr leaf = root.findDescendantShared("s.item.leaf"); + ASSERT_TRUE(static_cast(leaf)); + + // Expire + sweep the INTERMEDIATE ancestor: the whole s.item subtree is unlinked and (for the + // unpinned parts) freed. + root("s.item").expireAt(now_utc() - pt::seconds(1)); + root.sweepExpired(); + EXPECT_EQ(root.findDescendant("s.item"), nullptr); + EXPECT_EQ(root.findDescendant("s.item.leaf"), nullptr); + + // Reading the pinned leaf's OWN storage is safe even though its parent was freed. (We + // deliberately do NOT call leaf->fullName(): that walks the freed _parent chain — the documented + // ancestor-walk residual that the state:// resolver now avoids by deriving its canonical path + // from strings.) + EXPECT_EQ(leaf->value(), "V"); +} + +// sharedChild is the create-or-get owning analogue of operator(): same path semantics, but pins. +TEST(StateLifetime, SharedChildCreatesAndPins) { + cvc::app a; + auto &root = state::instance(a); + + state::state_ptr n = root.sharedChild("x.y.z"); + ASSERT_TRUE(static_cast(n)); + n->value("42"); + // Same node as operator() reaches. + EXPECT_EQ(root("x.y.z").value(), "42"); + EXPECT_EQ(root.sharedChild("x.y.z")->value(), "42"); + // An empty childname returns this node itself (like operator()). + EXPECT_EQ(root.sharedChild("").get(), &root); +} + +// Readers pin+read leaves via the owning accessor (the resolver's shape) while a sweeper churns the +// subtree — expire the intermediate, sweep (freeing the unpinned leaves), recreate. Under the fix, +// findDescendantShared either returns a pinned node (safe to read) or nullptr (already unlinked); +// it never yields a dangling pointer, so there is no crash/UAF. (With bare findDescendant this +// races on a freed node.) +TEST(StateLifetime, ConcurrentPinnedReadVsSweep) { + cvc::app a; + auto &root = state::instance(a); + constexpr int kLeaves = 16; + constexpr int kIters = 300; + + const auto seed = [&]() { + for (int i = 0; i < kLeaves; ++i) + root("s.item." + std::to_string(i)).value("v" + std::to_string(i)); + }; + seed(); + + std::atomic stop{false}; + std::atomic reads{0}; + + auto reader = [&]() { + while (!stop.load(std::memory_order_relaxed)) { + for (int i = 0; i < kLeaves; ++i) { + state::state_ptr p = root.findDescendantShared("s.item." + std::to_string(i)); + if (p) { + const std::string v = p->value(); // touch pinned storage — must never UAF + if (!v.empty()) + reads.fetch_add(1, std::memory_order_relaxed); + } + } + } + }; + + std::vector readers; + for (int t = 0; t < 4; ++t) + readers.emplace_back(reader); + + for (int it = 0; it < kIters; ++it) { + root("s.item").expireAt(now_utc() - pt::seconds(1)); + root.sweepExpired(); + seed(); + } + + stop.store(true, std::memory_order_relaxed); + for (auto &t : readers) + t.join(); + + EXPECT_GT(reads.load(), 0) << "readers should have observed live values across the churn"; +} From 84006bd57b269f887eeca1c9e90ff85e503cfc11 Mon Sep 17 00:00:00 2001 From: Joe Rivera Date: Tue, 29 Sep 2026 20:49:33 -0500 Subject: [PATCH 2/3] fix(state): pin ancestor chain for the write path + normalize state:// canonical MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the adversarial review of the Phase 1 change: - HIGH: state_store (state:// write) still UAF'd. The read path is safe (getters are _parent-free + canonical from strings), but state::value()/data() SETTERS internally call fullName() and parent()->childChanged(), which walk the node's _parent chain — so a leaf-only pin left ancestors exposed to a concurrent sweep. Fix: findDescendantShared/sharedChild gain an optional out-vector that collects the whole ANCESTOR CHAIN (root-child..terminal); resolveLink collects the resolved terminal's chain into link_resolution::target_pins. state_store now pins the chain across the setter (target chain for a followed transparent link, else the addressed chain). New test PinnedChainMakesWriteSafeAfterAncestorSweep. - MEDIUM: the state:// canonical echoed the raw input path instead of the normalized fullName(), breaking same-resource->same-canonical dedup for non-normalized spellings. Added state::normalize_path() (pure string op, no _parent walk) and use it for the canonical. - MEDIUM: resolveLink's per-hop next_owned->fullName() (an ancestor walk on each hop) is replaced by normalize_path(target) — the target is app-root-absolute so the string IS that node's fullName. Only the START node's fullName() remains, and it is safe because both state:// callers now pin the addressed chain before resolving. - MEDIUM/tests: added the write-path test above; existing AriadneStateUri suite already covers resolve/store/transparent-link/canonical (all pass); noted the concurrent test needs ASan/TSan to reliably surface a UAF. Full consistent build clean; 118/118 across the state:// resolver, link-resolution, transparent-link, expiry and lifetime suites pass. --- inc/cvc/core/state.h | 23 ++++++++- src/cvc/ariadne/uri_state.cpp | 51 +++++++++++++------- src/cvc/core/state.cpp | 37 ++++++++++---- src/cvc/tests/state_lifetime_stress_test.cpp | 27 ++++++++++- 4 files changed, 109 insertions(+), 29 deletions(-) diff --git a/inc/cvc/core/state.h b/inc/cvc/core/state.h index a6b64f96..d21c0308 100644 --- a/inc/cvc/core/state.h +++ b/inc/cvc/core/state.h @@ -523,6 +523,10 @@ class state : public boost::enable_shared_from_this { // terminal held as a state_ptr, so a caller that keeps `target_owned` (or the whole result) // alive cannot have the resolved node freed under it by a concurrent sweepExpired(). state_ptr target_owned; + // Owning pins of the resolved terminal's ancestor chain (root's child .. terminal), so a caller + // that MUTATES `target` (writes walk its _parent chain) is safe against a concurrent sweep. + // Empty when the start node was itself the terminal (a non-link resolveLink caller). + std::vector target_pins; std::vector visited; // ordered absolute paths std::size_t hops = 0; }; @@ -618,6 +622,13 @@ class state : public boost::enable_shared_from_this { // Useful for link resolution and any other read-only navigation. state *findDescendant(const std::string &path); + // Normalize a state path to its canonical dot-separated form (trim; drop + // leading/trailing/duplicate separators). A pure STRING operation — unlike fullName(), it never + // walks the _parent chain, so it is the safe way to obtain a node's canonical absolute path when + // the node may have been orphaned by a concurrent sweep (used by the state:// resolver to build + // its canonical URI). + static std::string normalize_path(const std::string &path); + // Owning analogues of findDescendant() and operator(): they return the map's shared_ptr rather // than a bare state* / state&, so the returned node stays alive for as long as the caller holds // the returned state_ptr (or a `handle` wrapping it) — a concurrent sweepExpired() can then only @@ -631,8 +642,16 @@ class state : public boost::enable_shared_from_this { // // findDescendantShared: read-only, returns a null state_ptr when any segment is absent. // sharedChild: create-or-get (like operator()), returns the pinning state_ptr for the terminal. - state_ptr findDescendantShared(const std::string &path); - state_ptr sharedChild(const std::string &childname = std::string()); + // + // The optional `pins` out-vector collects an owning state_ptr for EVERY node on the resolved path + // (root's child .. the returned node), so a caller can keep the whole ANCESTOR CHAIN alive. Pass + // it when the caller will MUTATE the returned node (value()/data() internally call fullName() and + // parent()->childChanged(), which walk _parent — a leaf-only pin leaves those ancestors exposed + // to a concurrent sweep). Pass nullptr for a bare leaf pin — a read via the parent-free getters, + // or a caller that never walks _parent, needs no more. + state_ptr findDescendantShared(const std::string &path, std::vector *pins = nullptr); + state_ptr sharedChild(const std::string &childname = std::string(), + std::vector *pins = nullptr); // RAII sugar: a movable handle that reads like a node (operator-> / operator*) while pinning it // alive. `handle h = node.sharedChild("a.b"); h->value("x");` keeps a.b alive for h's lifetime. diff --git a/src/cvc/ariadne/uri_state.cpp b/src/cvc/ariadne/uri_state.cpp index 2513ce15..b0e8f53d 100644 --- a/src/cvc/ariadne/uri_state.cpp +++ b/src/cvc/ariadne/uri_state.cpp @@ -34,6 +34,10 @@ std::string channel_of(const std::string &query) { // hazard for a `state://` resolve on a compute-pool worker). struct effective { cvc::state::state_ptr node; + // The effective node PLUS its ancestor chain, kept alive so a write (value()/data() walk the + // node's _parent chain via fullName()/childChanged()) is safe against a concurrent sweep. For a + // followed transparent link this is the TARGET's chain; otherwise the addressed node's chain. + std::vector pins; std::string path; }; @@ -42,17 +46,20 @@ struct effective { // the identical node — crucially even a DEFAULT (non-writable) transparent link: a read follows it, // so a write must follow it too, or a store would be shadowed on the link node and unreadable via // the same URI (the write-through routing in state::value() only fires for a WRITABLE link). -// `addressed_path` is the canonical path of `node` itself (its normalized `state://` path), used -// when the node is not a followed link. -effective effective_node(cvc::state::state_ptr node, const std::string &addressed_path) { +// `addressed_pins` is the addressed node's own ancestor chain (from the caller's navigation), used +// when the node is not a followed link; `addressed_path` is its normalized `state://` path. +effective effective_node(cvc::state::state_ptr node, + std::vector addressed_pins, + const std::string &addressed_path) { if (node && node->isLink() && node->linkMode() == cvc::state::link_mode::transparent) { const cvc::state::link_resolution lr = node->resolveLink(); if (lr.kind == cvc::state::link_resolution_kind::resolved && lr.target_owned) - // target_owned pins the terminal; visited.back() is its absolute path captured during the - // walk (safe), avoiding a fullName() on a possibly-orphaned node. - return {lr.target_owned, lr.visited.empty() ? addressed_path : lr.visited.back()}; + // target_owned pins the terminal and target_pins its ancestor chain; visited.back() is its + // absolute path captured during the walk (safe) — no fullName() on a possibly-orphaned node. + return {lr.target_owned, lr.target_pins, + lr.visited.empty() ? addressed_path : lr.visited.back()}; } - return {std::move(node), addressed_path}; + return {std::move(node), std::move(addressed_pins), addressed_path}; } // The `?value` / `?data` channels are served; `?children` and anything else are not (they need the @@ -70,15 +77,20 @@ UriResult state_resolve(cvc::state &root, const Uri &u) { "' is not served by the built-in state handler (only '?value' / '?data'); register " "a custom handler for '?children'"}; - // Navigate WITHOUT creating nodes; an empty path is the root itself. PIN the addressed node so a - // concurrent sweepExpired() (on the scheduler/pycvc thread) cannot free it while this resolve — - // which may run on a compute-pool worker — reads its value/data below. + // Navigate WITHOUT creating nodes; an empty path is the root itself. PIN the addressed node AND + // its ancestor chain so a concurrent sweepExpired() (on the scheduler/pycvc thread) cannot free + // it — or an ancestor it walks — while this resolve, which may run on a compute-pool worker, + // reads its value/data (parent-free getters) and, for a transparent link, resolves through it + // (resolveLink touches the start node's fullName). Canonical is built from the normalized path + // STRING, never a fullName() on the returned node. + std::vector chain; cvc::state::state_ptr node = - u.path.empty() ? root.shared_from_this() : root.findDescendantShared(u.path); + u.path.empty() ? root.shared_from_this() : root.findDescendantShared(u.path, &chain); if (!node) return {false, std::string(), std::string(), "ari: state node '" + u.path + "' not found"}; const effective eff = - effective_node(node, u.path); // read follows a transparent link to its target + effective_node(node, std::move(chain), + cvc::state::normalize_path(u.path)); // follows a transparent link const std::string canonical = "state://" + eff.path + "?" + channel; if (channel == "data") { @@ -107,11 +119,16 @@ StoreResult state_store(cvc::state &root, const Uri &u, const std::string &conte return {false, std::string(), "ari: state channel '?" + channel + "' is not writable by the built-in state handler (only '?value' / '?data')"}; - // sharedChild CREATES the path if absent (a store may target a not-yet-existing node) and PINS - // the terminal, so the write below is safe against a concurrent sweep — same rationale as the - // read. - cvc::state::state_ptr node = u.path.empty() ? root.shared_from_this() : root.sharedChild(u.path); - const effective eff = effective_node(node, u.path); + // sharedChild CREATES the path if absent (a store may target a not-yet-existing node) and pins + // the WHOLE chain (into `chain`). Unlike the read, the write below MUST pin ancestors: + // value()/data() internally call fullName() and parent()->childChanged(), which walk the node's + // _parent chain — a leaf-only pin would leave those ancestors exposed to a concurrent sweep (a + // use-after-free). The pins (eff.pins for a followed link, else `chain`) are held alive across + // the setter call below. + std::vector chain; + cvc::state::state_ptr node = + u.path.empty() ? root.shared_from_this() : root.sharedChild(u.path, &chain); + const effective eff = effective_node(node, std::move(chain), cvc::state::normalize_path(u.path)); if (channel == "data") eff.node->data(boost::any(content)); // store the bytes as a string blob on the data channel else diff --git a/src/cvc/core/state.cpp b/src/cvc/core/state.cpp index 53553b46..cc74e8d0 100644 --- a/src/cvc/core/state.cpp +++ b/src/cvc/core/state.cpp @@ -791,7 +791,7 @@ state &state::operator()(const std::string &childname) { // operator() (child under the parent's lock, released before descending); the difference is the // return type — the caller gets an owning pointer that keeps the node alive across a concurrent // sweepExpired(), where operator()'s bare state& would dangle. -state::state_ptr state::sharedChild(const std::string &childname) { +state::state_ptr state::sharedChild(const std::string &childname, std::vector *pins) { using namespace boost::algorithm; boost::this_thread::interruption_point(); @@ -821,6 +821,9 @@ state::state_ptr state::sharedChild(const std::string &childname) { } } cur = next; + if (pins) + pins->push_back( + cur); // pin every hop so the caller can safely walk the result's _parent chain } return cur; } @@ -1040,7 +1043,8 @@ state *state::findDescendant(const std::string &path) { return cur; } -state::state_ptr state::findDescendantShared(const std::string &path) { +state::state_ptr state::findDescendantShared(const std::string &path, + std::vector *pins) { using namespace boost::algorithm; std::string normalized = normalize_state_path(path); if (normalized.empty()) @@ -1061,10 +1065,16 @@ state::state_ptr state::findDescendantShared(const std::string &path) { next = it->second; // copy the OWNING shared_ptr — pins this hop past a concurrent sweep } cur = next; + if (pins) + pins->push_back( + cur); // pin every hop so the caller can safely walk the result's _parent chain } return cur; } +// static +std::string state::normalize_path(const std::string &path) { return normalize_state_path(path); } + state::link_resolution state::resolveLink(std::size_t hop_budget) { link_resolution result; @@ -1076,11 +1086,14 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { std::unordered_set seen; // Pin the walk: hold each hop as a state_ptr (start via shared_from_this(), each subsequent hop - // via findDescendantShared) so a concurrent sweepExpired() cannot free the node we are reading - // the link target / path from. The terminal is returned pinned in result.target_owned, so a - // caller that keeps the result can safely use `target` after we return. (Residual: fullName() - // below still walks each hop's unpinned ANCESTOR chain — a narrower window tracked with the - // fullName hardening in docs/roadmap/STATE_LIFETIME_AND_ATOMICITY.md.) + // via findDescendantShared) so a concurrent sweepExpired() cannot free the node we read the link + // target from, and collect the terminal's ancestor chain into result.target_pins so a caller that + // MUTATES the terminal (a write walks its _parent chain) is safe too. Each hop's path comes from + // the target STRING (normalize_path == that node's fullName, since the target is absolute) rather + // than a fullName() _parent walk. The one remaining _parent walk is the START node's fullName() + // just below; it is safe when the caller pinned the addressed node's chain (the state:// resolver + // does), and is otherwise the caller's responsibility (a resolvedValue/resolvedData caller must + // not hold an orphaned start). See docs/roadmap/STATE_LIFETIME_AND_ATOMICITY.md. state_ptr cur_owned = shared_from_this(); state *cur = cur_owned.get(); // Record the starting node's path so cycles that loop back to @@ -1100,6 +1113,8 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { result.kind = (cur == this) ? link_resolution_kind::none : link_resolution_kind::resolved; result.target = cur; result.target_owned = cur_owned; // pin the resolved terminal for the caller + // result.target_pins already holds this terminal's ancestor chain (set on the hop that + // reached it); empty iff the start node was itself the terminal. return result; } @@ -1109,7 +1124,8 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { return result; } - state_ptr next_owned = root.findDescendantShared(target); + std::vector hop_pins; + state_ptr next_owned = root.findDescendantShared(target, &hop_pins); if (!next_owned) { result.kind = link_resolution_kind::broken; result.target = nullptr; @@ -1118,7 +1134,9 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { } ++result.hops; - std::string next_path = next_owned->fullName(); + // The target is an app-root-absolute path, so its normalized form IS next_owned->fullName() — + // use the string to avoid walking next_owned's (unpinned) ancestor chain. + std::string next_path = normalize_state_path(target); if (!seen.insert(next_path).second) { result.kind = link_resolution_kind::cycle_detected; result.target = nullptr; @@ -1126,6 +1144,7 @@ state::link_resolution state::resolveLink(std::size_t hop_budget) { return result; } result.visited.push_back(next_path); + result.target_pins = std::move(hop_pins); // this hop's chain; kept iff it becomes the terminal cur_owned = next_owned; cur = cur_owned.get(); } diff --git a/src/cvc/tests/state_lifetime_stress_test.cpp b/src/cvc/tests/state_lifetime_stress_test.cpp index c711bda0..cebce706 100644 --- a/src/cvc/tests/state_lifetime_stress_test.cpp +++ b/src/cvc/tests/state_lifetime_stress_test.cpp @@ -87,6 +87,30 @@ TEST(StateLifetime, PinnedLeafSurvivesAncestorSweep) { EXPECT_EQ(leaf->value(), "V"); } +// The WRITE analogue: mutating a node (value()/data()) internally walks its _parent chain +// (fullName() + parent()->childChanged()), so a safe write after an ancestor sweep needs the whole +// ANCESTOR CHAIN pinned — not just the leaf. This is what the state:// store path now does via +// sharedChild(path, &chain); a leaf-only pin would UAF here. +TEST(StateLifetime, PinnedChainMakesWriteSafeAfterAncestorSweep) { + cvc::app a; + auto &root = state::instance(a); + root("s.item.leaf").value("V"); + + std::vector chain; // pins root-child .. leaf + state::state_ptr leaf = root.sharedChild("s.item.leaf", &chain); + ASSERT_TRUE(static_cast(leaf)); + ASSERT_FALSE(chain.empty()); + + root("s.item").expireAt(now_utc() - pt::seconds(1)); + root.sweepExpired(); + EXPECT_EQ(root.findDescendant("s.item"), nullptr) << "intermediate unlinked"; + + // Writing the leaf walks its _parent chain; with the chain pinned every ancestor is still alive, + // so the write is UAF-free (with only a leaf pin this would be a use-after-free). + leaf->value("W"); + EXPECT_EQ(leaf->value(), "W"); +} + // sharedChild is the create-or-get owning analogue of operator(): same path semantics, but pins. TEST(StateLifetime, SharedChildCreatesAndPins) { cvc::app a; @@ -106,7 +130,8 @@ TEST(StateLifetime, SharedChildCreatesAndPins) { // subtree — expire the intermediate, sweep (freeing the unpinned leaves), recreate. Under the fix, // findDescendantShared either returns a pinned node (safe to read) or nullptr (already unlinked); // it never yields a dangling pointer, so there is no crash/UAF. (With bare findDescendant this -// races on a freed node.) +// races on a freed node.) NOTE: a plain run only catches a UAF if it happens to corrupt memory +// observably; run under ASan/TSan (this repo needs `setarch -R` for TSan) for a reliable signal. TEST(StateLifetime, ConcurrentPinnedReadVsSweep) { cvc::app a; auto &root = state::instance(a); From 778c3426fd6959f446614440b1286665c69edcfa Mon Sep 17 00:00:00 2001 From: Joe Rivera Date: Tue, 29 Sep 2026 21:14:53 -0500 Subject: [PATCH 3/3] fix(state): sendMessage takes the resolved path from the link walk, not a post-drop fullName() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Latent-hazard cleanup flagged by the messaging safety review: state::sendMessage resolved a transparent link, then (after the link_resolution and its Phase-1 pins went out of scope) called target->fullName() to set resolved_path — a _parent walk on the now-unpinned resolved terminal. Not exploitable today (sendMessage runs only on the scheduler thread, where no concurrent sweep can interleave) and not a regression (no pins existed pre-Phase-1), but the exact 'walk _parent after the pin is released' shape the lifetime work is closing. Use lr.visited.back() (the resolved absolute path, captured during the walk) for the link-resolved/none cases; only the non-link node (target == this) falls back to fullName() on itself. Messaging tests (StateSendMessage/StateMessage/StateMessageBus) 37/37 green; resolved_path is byte-identical (visited.back() == the old fullName()). --- src/cvc/core/state.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/cvc/core/state.cpp b/src/cvc/core/state.cpp index cc74e8d0..25abd444 100644 --- a/src/cvc/core/state.cpp +++ b/src/cvc/core/state.cpp @@ -1270,6 +1270,12 @@ state::send_message_result state::sendMessage(const std::string &payload, case link_resolution_kind::resolved: case link_resolution_kind::none: target = lr.target; + // Take the resolved absolute path from the walk while `lr` (and its pins) are still alive. + // Calling target->fullName() AFTER `lr` is dropped would walk the resolved terminal's _parent + // chain unpinned — safe today (sendMessage is scheduler-thread only) but a latent cross-node + // hazard if this ever runs off-thread; visited.back() is already that absolute path. + if (!lr.visited.empty()) + r.resolved_path = lr.visited.back(); break; case link_resolution_kind::broken: r.status = send_message_result::status_kind::broken_link; @@ -1288,7 +1294,8 @@ state::send_message_result state::sendMessage(const std::string &payload, return r; } } - r.resolved_path = target->fullName(); + if (r.resolved_path.empty()) + r.resolved_path = target->fullName(); // non-link node (target == this): its own path // 2. Find the default shard for this app context. With no // shard registered (common in unit tests of pure-state code)