diff --git a/graph/Graph.i b/graph/Graph.i index d6fb1769..904e3fe5 100644 --- a/graph/Graph.i +++ b/graph/Graph.i @@ -166,7 +166,10 @@ VertexPathIterator * path_iterator(const RiseFall *rf, const MinMax *min_max) { - return new VertexPathIterator(self, rf, min_max, Sta::sta()); + VertexPathIterator *iter = new VertexPathIterator(self, rf, min_max, Sta::sta()); + // The iterator caches a raw Path*; register it for the stale-handle guard. + Sta::sta()->search()->registerValidHandle(iter); + return iter; } } // Vertex methods diff --git a/include/sta/Search.hh b/include/sta/Search.hh index 13352846..839eaec4 100644 --- a/include/sta/Search.hh +++ b/include/sta/Search.hh @@ -179,15 +179,16 @@ public: void deleteFilter(); void deleteFilteredArrivals(); - // Stale path-handle guard (OpenROAD #10210). PathEnds are freed when path - // groups are deleted, so a handle held across a search update dangles. These - // track the currently-valid PathEnds so the Tcl accessors report a clean - // error instead of crashing (best-effort: a reused address is not detected). - // The Tcl-only guard does not cover C++ callers holding a Path*/PathEnd* - // directly; the contract is that a path is valid only until the next search - // update, so copy stable fields out first (cf. the rsz Path::prevArc() fix). - void registerValidPathEnds(const PathEndSeq &ends); - bool pathEndValid(const PathEnd *path_end) const; + // Stale path-handle guard (OpenROAD #10210). PathEnds and the Paths they + // expose are freed/reallocated on a search update, so a handle held across + // one dangles. These track the handles currently handed to Tcl so the Tcl + // accessors report a clean error instead of crashing (best-effort: a reused + // address is not detected). The Tcl-only guard does not cover C++ callers + // holding a Path*/PathEnd* directly; the contract is that a path is valid + // only until the next search update, so copy stable fields out first (cf. the + // rsz Path::prevArc() fix). + void registerValidHandle(const void *handle); + bool handleValid(const void *handle) const; VertexSet &endpoints(); void endpointsInvalid(); @@ -681,8 +682,8 @@ protected: bool postpone_latch_outputs_{false}; std::vector enum_paths_; - // Currently-valid PathEnds returned to external callers (see guard methods). - std::unordered_set valid_path_ends_; + // PathEnd*/Path* handles currently handed to external callers (guard methods). + std::unordered_set valid_handles_; VisitPathEnds *visit_path_ends_; GatedClk *gated_clk_; diff --git a/search/Search.cc b/search/Search.cc index cd5e0d70..65b05928 100644 --- a/search/Search.cc +++ b/search/Search.cc @@ -334,24 +334,24 @@ Search::clear() void Search::deletePathGroups() { - // PathEnds (including any handed to external callers) are destroyed here, so - // clear the stale-handle guard set at this free site. - valid_path_ends_.clear(); + // Free site for the stale-handle guard set (see Search.hh). + valid_handles_.clear(); for (Mode *mode : modes_) mode->deletePathGroups(); } -// Stale path-handle guard. +// Stale path-handle guard (see Search.hh). void -Search::registerValidPathEnds(const PathEndSeq &ends) +Search::registerValidHandle(const void *handle) { - valid_path_ends_.insert(ends.begin(), ends.end()); + if (handle) + valid_handles_.insert(handle); } bool -Search::pathEndValid(const PathEnd *path_end) const +Search::handleValid(const void *handle) const { - return valid_path_ends_.contains(path_end); + return valid_handles_.contains(handle); } bool diff --git a/search/Search.i b/search/Search.i index fd4ba946..e3f8ae21 100644 --- a/search/Search.i +++ b/search/Search.i @@ -42,33 +42,53 @@ using namespace sta; -%} +// Stale path-handle guard (OpenROAD #10210). Error on a dangling handle; throws. +// Use directly inside %extend bodies (the global %exception catches the throw). +static void +reportStaleHandle() +{ + Sta::sta()->report()->error(2310, + "stale path handle: a path or path end is only" + " valid until the next timing search update."); +} -// Stale path-handle guard. One typemap validates every -// PathEnd* crossing the Tcl boundary -- all %extend accessors (the self arg), -// report_path_end, and any future ones. Declared before the PathEnd class so it -// applies to the %extend methods below. -%typemap(check) PathEnd * { - if (!Sta::sta()->search()->pathEndValid($1)) { - // A check typemap runs outside the global %exception try/catch - // (tcl/Exception.i), so catch report_'s ExceptionMsg locally and convert it - // to a Tcl error the same way -- giving the usual "Error: ..." output. - try { - Sta::sta()->report()->error(2310, - "path end used after a timing search update;" - " a path object is only valid until the next" - " search update."); - } - catch (ExceptionMsg &excp) { - if (!excp.suppressed()) { - Tcl_ResetResult(interp); - Tcl_AppendResult(interp, "Error: ", excp.what(), nullptr); - } - SWIG_fail; +// Same error for check typemaps, which run outside the global %exception: catch +// and reformat to a Tcl error here. Follow with SWIG_fail at the call site. +static void +staleHandleError(Tcl_Interp *interp) +{ + try { + reportStaleHandle(); + } + catch (ExceptionMsg &excp) { + if (!excp.suppressed()) { + Tcl_ResetResult(interp); + Tcl_AppendResult(interp, "Error: ", excp.what(), nullptr); } } } +%} + +// Validate every PathEnd*/Path* entering Tcl (accessor self args + free +// functions). Declared before the classes so it covers their %extend methods. +// VertexPathIterator is guarded inline in has_next()/next() instead -- a typemap +// would also block finish(), leaking a stale iterator that can't be deleted. +%typemap(check) PathEnd *, Path * { + if (!Sta::sta()->search()->handleValid($1)) { + staleHandleError(interp); + SWIG_fail; + } +} + +// Register every Path* returned to Tcl so the check above can detect staleness. +// (PathEnds are registered in find_path_ends.) +%typemap(out) Path *, const Path * { + Sta::sta()->search()->registerValidHandle($1); + Tcl_SetObjResult(interp, SWIG_NewInstanceObj(SWIG_as_voidptr($1), + SWIGTYPE_p_Path, 0)); +} + //////////////////////////////////////////////////////////////// // // Empty class definitions to make swig happy. @@ -408,9 +428,10 @@ find_path_ends(ExceptionFrom *from, setup, hold, recovery, removal, clk_gating_setup, clk_gating_hold); - // Register for the Tcl stale-handle guard. Tcl-only: - // internal C++ Sta::findPathEnds callers do not register and pay no cost. - sta->search()->registerValidPathEnds(ends); + // Register for the Tcl stale-handle guard. Tcl-only: internal C++ + // Sta::findPathEnds callers do not register and pay no cost. + for (PathEnd *end : ends) + sta->search()->registerValidHandle(end); return ends; } @@ -1314,10 +1335,20 @@ start_path() } %extend VertexPathIterator { -bool has_next() { return self->hasNext(); } +// has_next()/next() deref the iterator's cached Path*, which a search update +// frees, so validate the handle first (registered in Vertex::path_iterator). +// finish() only deletes the iterator, so it stays unguarded. +bool has_next() +{ + if (!Sta::sta()->search()->handleValid(self)) + reportStaleHandle(); + return self->hasNext(); +} Path * next() { + if (!Sta::sta()->search()->handleValid(self)) + reportStaleHandle(); return self->next(); } diff --git a/tcl/StaTclTypes.i b/tcl/StaTclTypes.i index c5eb3420..c7d362ed 100644 --- a/tcl/StaTclTypes.i +++ b/tcl/StaTclTypes.i @@ -1149,6 +1149,8 @@ using namespace sta; while (path_iter.hasNext()) { Path *path = &path_iter.next(); Path *copy = new Path(path); + // Register for the stale-handle guard (see Search guard methods). + Sta::sta()->search()->registerValidHandle(copy); Tcl_Obj *obj = SWIG_NewInstanceObj(copy, SWIGTYPE_p_Path, false); Tcl_ListObjAppendElement(interp, list, obj); } @@ -1399,6 +1401,8 @@ using namespace sta; case PropertyValue::Type::paths: { Tcl_Obj *list = Tcl_NewListObj(0, nullptr); for (const Path *path : *value.paths()) { + // Register for the stale-handle guard (see Search guard methods). + Sta::sta()->search()->registerValidHandle(path); Tcl_Obj *obj = SWIG_NewInstanceObj(const_cast(path), SWIGTYPE_p_Path, false); Tcl_ListObjAppendElement(interp, list, obj); } diff --git a/test/stale_path_uaf.ok b/test/stale_path_uaf.ok index 2c6296da..51906b6e 100644 --- a/test/stale_path_uaf.ok +++ b/test/stale_path_uaf.ok @@ -1,2 +1,3 @@ -Error: 2310 path end used after a timing search update; a path object is only valid until the next search update. -Error: 2310 path end used after a timing search update; a path object is only valid until the next search update. +Error: 2310 stale path handle: a path or path end is only valid until the next timing search update. +Error: 2310 stale path handle: a path or path end is only valid until the next timing search update. +Error: 2310 stale path handle: a path or path end is only valid until the next timing search update. diff --git a/test/stale_path_uaf.tcl b/test/stale_path_uaf.tcl index 6fa9083a..615a13d4 100644 --- a/test/stale_path_uaf.tcl +++ b/test/stale_path_uaf.tcl @@ -1,21 +1,37 @@ # Guard for stale path handles across a search update (OpenROAD #10210). -# Holding a PathEnd past the next query must error cleanly, not crash. +# Holding a PathEnd, or a Path saved separately, past the next query must error +# cleanly, not crash. read_liberty ../examples/nangate45_typ.lib.gz read_verilog ../examples/example1.v link_design top create_clock -name clk -period 10 {clk1 clk2 clk3} set_input_delay -clock clk 0 {in1 in2} -# Second query = find_timing_paths. -set ends [find_timing_paths -through u1/Z] -set pe [lindex $ends 0] -set junk [find_timing_paths -through u2/ZN] -catch { puts "stale : slack=[$pe slack] arrival=[[$pe path] arrival]" } msg +report_checks + +# Stale PathEnd, second query = find_timing_paths. +set pe [lindex [find_timing_paths -through u1/Z] 0] +find_timing_paths -through u2/ZN +catch { puts "stale PathEnd : slack=[$pe slack]" } msg puts $msg -# Second query = report_checks. -set ends [find_timing_paths -through u1/Z] -set pe [lindex $ends 0] +# Stale PathEnd, second query = report_checks. +set pe [lindex [find_timing_paths -through u1/Z] 0] report_checks -path_delay max -catch { puts "stale : slack=[$pe slack] arrival=[[$pe path] arrival]" } msg +catch { puts "stale PathEnd : slack=[$pe slack]" } msg +puts $msg + +# Stale Path saved separately (set p [$pe path]) across the next query. +set pe [lindex [find_timing_paths -through u1/Z] 0] +set p [$pe path] +find_timing_paths -through u2/ZN +catch { puts "stale Path : arrival=[$p arrival]" } msg +puts $msg + +# Stale VertexPathIterator held across the next query. Iterate the filtered +# endpoint vertex, whose paths_ array the next query frees/reallocates. +set pe [lindex [find_timing_paths -through u1/Z] 0] +set it [[$pe vertex] path_iterator rise max] +find_timing_paths -through u2/ZN +catch { while {[$it has_next]} { set p [$it next]; puts "stale PathIter : arrival=[$p arrival]" } } msg puts $msg