From d1db8c6f75f1f717652b1df56ad7061bc74bddda Mon Sep 17 00:00:00 2001 From: Zahari Zahariev Date: Thu, 25 Jun 2026 23:42:11 +0300 Subject: [PATCH] Fix uninitialized members, deferred-invalidation reset, and erase-remove idiom These are latent on a fresh/zeroed heap but corrupt under heap reuse (found while running Compute under churn on a 32-bit target; all are platform-agnostic). Uninitialized members (a `= default` ctor / member-init list leaves them indeterminate; they're read as pointers, presence checks, or lazy-init guards): - indirect_pointer_vector::_data (backs Subgraph::_parents): 0 == empty; garbage here yields a bogus parent pointer. - Graph::_main_handler (+_main_handler_context): has_main_handler() == (_main_handler != nullptr); garbage -> call_main_handler() invokes a junk function pointer. - Graph::_keys: lazy-init `if (_keys == nullptr) _keys = new KeyTable` + ~Graph `if (_keys) delete _keys` -> garbage skips init / deletes a junk pointer. - Graph::_trace_recorder: read as `if (_trace_recorder)`. - Graph::_deferring_subgraph_invalidation, _needs_update; Context::{_graph_version, _needs_update, _invalidated}; Subgraph::{_traversal_seed, _index, _flags, _descendent_flags, _dirty_flags, _descendent_dirty_flags}. UpdateStack deferred-invalidation reset: the "end deferring on exit" flag was combined with `&` (which clears it) instead of `|` (which sets it). The destructor checks this flag to clear _deferring_subgraph_invalidation, so after the first attribute update the graph stays in the deferring state permanently, and every later Subgraph::invalidate() takes the deferred path that is only flushed in the non-deferred branch -> invalidated subgraphs and their values are never reclaimed. erase-remove idiom (Subgraph::remove_child, invalidate_now, remove_observer; Graph subgraph/trace removal): the erase-remove uses the single-element erase(it) instead of the range erase(it, end()), leaving stale tail elements whenever more than one element is removed. --- Sources/ComputeCxx/Graph/Context.h | 6 +++--- Sources/ComputeCxx/Graph/Graph.cpp | 6 +++--- Sources/ComputeCxx/Graph/Graph.h | 12 ++++++------ Sources/ComputeCxx/Graph/UpdateStack.cpp | 2 +- Sources/ComputeCxx/Subgraph/Subgraph.cpp | 10 +++++----- Sources/ComputeCxx/Subgraph/Subgraph.h | 12 ++++++------ Sources/ComputeCxx/Vector/IndirectPointerVector.h | 2 +- 7 files changed, 25 insertions(+), 25 deletions(-) diff --git a/Sources/ComputeCxx/Graph/Context.h b/Sources/ComputeCxx/Graph/Context.h index 09abc562..e98e7aab 100644 --- a/Sources/ComputeCxx/Graph/Context.h +++ b/Sources/ComputeCxx/Graph/Context.h @@ -26,9 +26,9 @@ class Graph::Context { ClosureFunctionVV _update_callback = {nullptr, nullptr}; uint64_t _deadline = UINT64_MAX; - uint64_t _graph_version; - bool _needs_update; - bool _invalidated; + uint64_t _graph_version = 0; + bool _needs_update = false; + bool _invalidated = false; void call_invalidation(AttributeID attribute); diff --git a/Sources/ComputeCxx/Graph/Graph.cpp b/Sources/ComputeCxx/Graph/Graph.cpp index 01b773dc..82258396 100644 --- a/Sources/ComputeCxx/Graph/Graph.cpp +++ b/Sources/ComputeCxx/Graph/Graph.cpp @@ -235,7 +235,7 @@ void Graph::add_subgraph(Subgraph &subgraph) { void Graph::remove_subgraph(Subgraph &subgraph) { auto iter = std::remove(_subgraphs.begin(), _subgraphs.end(), &subgraph); - _subgraphs.erase(iter); + _subgraphs.erase(iter, _subgraphs.end()); if (auto map = _tree_data_elements_by_subgraph.get()) { auto iter = map->find(&subgraph); @@ -247,7 +247,7 @@ void Graph::remove_subgraph(Subgraph &subgraph) { if (subgraph.has_cached_nodes()) { subgraph.set_has_cached_nodes(false); auto iter = std::remove(_subgraphs_with_cached_nodes.begin(), _subgraphs_with_cached_nodes.end(), &subgraph); - _subgraphs_with_cached_nodes.erase(iter); + _subgraphs_with_cached_nodes.erase(iter, _subgraphs_with_cached_nodes.end()); } _num_subgraphs -= 1; @@ -2011,7 +2011,7 @@ void Graph::remove_trace(uint64_t trace_id) { Trace *trace = *iter; trace->end_trace(*this); trace->trace_removed(); - _traces.erase(iter); + _traces.erase(iter, _traces.end()); } } diff --git a/Sources/ComputeCxx/Graph/Graph.h b/Sources/ComputeCxx/Graph/Graph.h index e7063296..4fed1c88 100644 --- a/Sources/ComputeCxx/Graph/Graph.h +++ b/Sources/ComputeCxx/Graph/Graph.h @@ -87,8 +87,8 @@ class Graph { vector _traces; // Main thread handler - MainHandler _Nullable _main_handler; - const void *_Nullable _main_handler_context; + MainHandler _Nullable _main_handler = nullptr; + const void *_Nullable _main_handler_context = nullptr; // Metrics uint64_t _num_nodes = 0; @@ -98,20 +98,20 @@ class Graph { uint64_t _num_value_bytes = 0; // Trace recorder - TraceRecorder *_trace_recorder; + TraceRecorder *_trace_recorder = nullptr; // Tree std::unique_ptr> _tree_data_elements_by_subgraph; - KeyTable *_Nullable _keys; + KeyTable *_Nullable _keys = nullptr; // Subgraphs vector _subgraphs; vector _subgraphs_with_cached_nodes; vector _invalidating_subgraphs; - bool _deferring_subgraph_invalidation; + bool _deferring_subgraph_invalidation = false; // Threads - bool _needs_update; + bool _needs_update = false; uint32_t _ref_count = 1; pthread_t _current_update_thread = 0; diff --git a/Sources/ComputeCxx/Graph/UpdateStack.cpp b/Sources/ComputeCxx/Graph/UpdateStack.cpp index d7b62708..2958f962 100644 --- a/Sources/ComputeCxx/Graph/UpdateStack.cpp +++ b/Sources/ComputeCxx/Graph/UpdateStack.cpp @@ -22,7 +22,7 @@ Graph::UpdateStack::UpdateStack(Graph *graph, IAGGraphUpdateOptions options) if (graph->_deferring_subgraph_invalidation == false) { graph->_deferring_subgraph_invalidation = true; - _options = IAGGraphUpdateOptions(_options & IAGGraphUpdateOptionsEndDeferringSubgraphInvalidationOnExit); + _options = IAGGraphUpdateOptions(_options | IAGGraphUpdateOptionsEndDeferringSubgraphInvalidationOnExit); // set, not clear: the dtor checks this flag to reset _deferring_subgraph_invalidation } Graph::set_current_update(util::tagged_ptr(this, options & IAGGraphUpdateOptionsInitializeCleared)); diff --git a/Sources/ComputeCxx/Subgraph/Subgraph.cpp b/Sources/ComputeCxx/Subgraph/Subgraph.cpp index fb4571b2..d8920f2e 100644 --- a/Sources/ComputeCxx/Subgraph/Subgraph.cpp +++ b/Sources/ComputeCxx/Subgraph/Subgraph.cpp @@ -110,7 +110,7 @@ void Subgraph::remove_observer(IAGUniqueID observer_id) { } return false; }); - observers->erase(iter); + observers->erase(iter, observers->end()); } } @@ -204,7 +204,7 @@ void Subgraph::invalidate_now(Graph &graph) { [&child](auto other_parent_child) -> bool { return other_parent_child.subgraph() == child.subgraph(); }); - other_parent->_children.erase(iter); + other_parent->_children.erase(iter, other_parent->_children.end()); } child.subgraph()->_parents.clear(); @@ -224,7 +224,7 @@ void Subgraph::invalidate_now(Graph &graph) { // its parents vector auto iter = std::remove(child.subgraph()->_parents.begin(), child.subgraph()->_parents.end(), subgraph); - child.subgraph()->_parents.erase(iter); + child.subgraph()->_parents.erase(iter, child.subgraph()->_parents.end()); } } } @@ -356,7 +356,7 @@ void Subgraph::add_child(Subgraph &child, uint8_t tag) { void Subgraph::remove_child(Subgraph &child, bool suppress_trace) { auto parent_iter = std::remove(child._parents.begin(), child._parents.end(), this); - child._parents.erase(parent_iter); + child._parents.erase(parent_iter, child._parents.end()); if (!suppress_trace) { graph()->foreach_trace([this, &child](Trace &trace) { trace.remove_child(*this, child); }); @@ -365,7 +365,7 @@ void Subgraph::remove_child(Subgraph &child, bool suppress_trace) { auto child_iter = std::remove_if(_children.begin(), _children.end(), [&child](auto subgraph_child) -> bool { return subgraph_child.subgraph() == &child; }); - _children.erase(child_iter); + _children.erase(child_iter, _children.end()); } bool Subgraph::ancestor_of(const Subgraph &other) { diff --git a/Sources/ComputeCxx/Subgraph/Subgraph.h b/Sources/ComputeCxx/Subgraph/Subgraph.h index a2a01bff..dfc50b3f 100644 --- a/Sources/ComputeCxx/Subgraph/Subgraph.h +++ b/Sources/ComputeCxx/Subgraph/Subgraph.h @@ -58,16 +58,16 @@ class Subgraph : public data::zone { uint64_t observer_id; }; data::ptr *> _observers; - uint32_t _traversal_seed; - uint32_t _index; + uint32_t _traversal_seed = 0; + uint32_t _index = 0; data::ptr _cache = nullptr; Graph::TreeElementID _tree_root; - IAGAttributeFlags _flags; - IAGAttributeFlags _descendent_flags; - IAGAttributeFlags _dirty_flags; - IAGAttributeFlags _descendent_dirty_flags; + IAGAttributeFlags _flags = {}; + IAGAttributeFlags _descendent_flags = {}; + IAGAttributeFlags _dirty_flags = {}; + IAGAttributeFlags _descendent_dirty_flags = {}; enum class InvalidationState : uint8_t { None = 0, diff --git a/Sources/ComputeCxx/Vector/IndirectPointerVector.h b/Sources/ComputeCxx/Vector/IndirectPointerVector.h index 821c731d..0c2f3c56 100644 --- a/Sources/ComputeCxx/Vector/IndirectPointerVector.h +++ b/Sources/ComputeCxx/Vector/IndirectPointerVector.h @@ -30,7 +30,7 @@ class indirect_pointer_vector { NullElement = 0x2, }; - uintptr_t _data; + uintptr_t _data = 0; // 0 == empty; `= default` ctor would otherwise leave this indeterminate using vector_type = vector;