feat(state): owning accessors + pinned resolveLink — fix cross-node UAF (lifetime Phase 1) - #492
Merged
Merged
Conversation
…s-node UAF (Phase 1) 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<state> (boost variant, to match state_ptr = boost::shared_ptr<state>). 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.
…/ canonical 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.
…ot a post-drop fullName() 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()).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 / Option A of the merged design
docs/roadmap/STATE_LIFETIME_AND_ATOMICITY.md(#486). Closes the cross-node use-after-free incvc::state.The bug
operator()returns a barestate&andfindDescendant()a barestate*, whose only owner is the parent's_childrenmap. A concurrentsweepExpired()(the sole production node-freeing path) can free a node under a caller still holding the bare reference — a UAF. The confirmed trigger (adversarial review "H1"): astate://…resolve on a compute-pool worker callingeff->fullName()while a tree-wide sweep on the scheduler/pycvc thread orphans an intermediate ancestor.Core (
state.h/state.cpp)statenow derivesboost::enable_shared_from_this<state>(boost variant, matchingstate_ptr = boost::shared_ptr<state>; std's would throwbad_weak_ptr). Zero minting changes — every live node is alreadyshared_ptr-owned.findDescendantShared(path)— owning analogue offindDescendant: copies the map'sshared_ptrat each hop, so the result is pinned (a concurrent sweep can only unlink it, never free it under the caller).sharedChild(name)— owning create-or-get analogue ofoperator()(same lock/create semantics).state::handle— thin RAII pin.resolveLinknow pins its walk (each hop held as astate_ptr) and returns the terminal pinned inlink_resolution::target_owned.Resolver (
uri_state.cpp)state_resolve/state_storenow pin the addressed + effective node and buildcanonicalfrom path strings (the input path, orresolveLink's capturedvisitedchain) instead of a post-resolutioneff->fullName()— the exact walk that UAF'd on an orphaned node.Scope / residual
operator()/findDescendantare unchanged — Option A is an opt-in owning accessor, not a transparent upgrade (changing their return type would break fluent chaining + 82 call sites). The pin covers a node's own storage, not its ancestor chain, sofullName()/parentName()on a pinned-but-orphaned node stays unsafe;resolveLink's internalfullName()during the walk is the narrower remaining window — both tracked with the fullName hardening (Phase 3 / the design's open questions). Multi-node atomicity is Phase 2 (the cache keepscache_mutexuntil then).Test —
state_lifetime_stress_test.cpp~state/destroyedto the last owner (Option A's one observable change; keepsstate_expiry_testgreen since it holds no pin).state*would UAF here).All pass locally; the concurrent case is meant to run under TSan (
setarch -R) + ASan in CI.