rsz: use prev_arc input port to fix repair_timing SIGSEGV (#10210)#10225
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors input port resolution across several optimization modules, including RecoverPower, SizeUpMove, SwapPinsMove, and UnbufferMove, to utilize timing arcs instead of direct path indexing. Feedback was provided to address potential null pointer dereferences in SizeUpMove.cc and UnbufferMove.cc by implementing earlier null checks for resolved ports and arcs to ensure stability during path traversal.
| const sta::TimingArc* in_arc = drvr_path->prevArc(sta_); | ||
| sta::LibertyPort* in_port = in_arc ? in_arc->from() : nullptr; | ||
| sta::Path* prev_drvr_path = drvr_path->prevPath()->prevPath(); | ||
| sta::Pin* prev_drvr_pin | ||
| = prev_drvr_path ? prev_drvr_path->pin(sta_) : nullptr; | ||
|
|
||
| float prev_drive; | ||
| float prev_drive = 0.0; | ||
| if (prev_drvr_pin) { | ||
| prev_drive = 0.0; | ||
| sta::LibertyPort* prev_drvr_port = network_->libertyPort(prev_drvr_pin); | ||
| if (prev_drvr_port) { | ||
| prev_drive = prev_drvr_port->driveResistance(); | ||
| } | ||
| } else { | ||
| prev_drive = 0.0; | ||
| } | ||
|
|
||
| if (in_port == nullptr) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The null check for in_port should be performed immediately after its resolution. If in_arc is null (which can occur in rare cases like paths starting at a generated clock source), drvr_path->prevPath() will return nullptr, leading to a SIGSEGV when calling prevPath() on it at line 54. Moving the check up ensures safety before any further path traversal.
const sta::TimingArc* in_arc = drvr_path->prevArc(sta_);
sta::LibertyPort* in_port = in_arc ? in_arc->from() : nullptr;
if (in_port == nullptr) {
return false;
}
sta::Path* prev_drvr_path = drvr_path->prevPath()->prevPath();
sta::Pin* prev_drvr_pin
= prev_drvr_path ? prev_drvr_path->pin(sta_) : nullptr;
float prev_drive = 0.0;
if (prev_drvr_pin) {
sta::LibertyPort* prev_drvr_port = network_->libertyPort(prev_drvr_pin);
if (prev_drvr_port) {
prev_drive = prev_drvr_port->driveResistance();
}
}
|
clang-tidy review says "All clean, LGTM! 👍" |
When drvr_path->prevArc() returns null (rare genclk source / latch D->Q), prevPath() is also null and dereferencing it in the subsequent prevPath()->prevPath() call would SIGSEGV. Hoist the null-check on the resolved input LibertyPort (or LibertyPort+pin in UnbufferMove) to immediately after it is computed, before any further path traversal. Also matches the pattern already used in SwapPinsMove and avoids wasted prev_drive / prev_drvr work on the reject path. In UnbufferMove this also keeps drvr_input_pin non-null on the happy path, so estimatedSlackOK correctly skips the buffer instance being removed when scanning the driving net's fanouts. Addresses PR review feedback on The-OpenROAD-Project#10225 (issue The-OpenROAD-Project#10210). Signed-off-by: Minju Kim <mkim@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
The issue shows this step in repair_timing:
[INFO RSZ-0099] Repairing 1098 out of 1098 (100.00%) violating endpoints...
Iter | Removed | Resized | Inserted | Cloned | Pin | Area | WNS | TNS | Viol | Worst
| Buffers | Gates | Buffers | Gates | Swaps | | | | Endpts | Endpt
--------------------------------------------------------------------------------------------------------------
0 | 0 | 0 | 0 | 0 | 0 | +0.0% | -88.979 | -17535.9 | 1098 | maccontrol1.receivecontrol1.PauseTimer[8]$_DFFE_PP0P_/D
Signal 11 received
Stack trace:
...
This indicates this is using the STA version that is prior to the non-deterministic delay calc fix we made few weeks ago in STA (TNS of -17535.9 is suspicious). I will request this to be first verified with latest OpenROAD master if the crash is still persistent.
If racy dcalc produced non-deterministic slew/delay numbers, the repair loop picks different paths/moves each run. Some of those sequences may be the only ones that hit the prevPath()->pin() edge case. Fix the dcalc race and the pathological sequence may simply never be generated, the crash may stop reproducing.
The OpenSTA fix may well make this specific crash stop appearing, but this PR is good change as it is defensive hardening of a real fragility -- the prevArc()->from() form is strictly better than relying on prevPath()->pin() being on the same instance. But I'd still prefer to confirm if crash is still reproducible with latest OpenROAD before claiming the PR is the root-cause fix vs. belt-and-suspenders on top of the dcalc fix.
| if (in_lib_port == nullptr) { | ||
| return false; | ||
| } | ||
| sta::Pin* drvr_input_pin = network_->findPin(drvr, in_lib_port); |
There was a problem hiding this comment.
behavior change: drvr_input_pin used to come from prevPath()->pin() and is now network_->findPin(drvr, in_lib_port) - always a pin on the driver instance. This flows into BaseMove.cc:413 (side_input_pin == params.driver_input_pin skip). This now refers to a different thing than before - intentional? Wondering if it changes the skip behavior on side-fanouts where the old code compared against the wrong pin.
There was a problem hiding this comment.
Yes, intentional.
driver_input_pin, as the name suggests, is meant to be the driver cell's input pin on the current path.
The old prevPath()->pin() happened to return that in stable STA state, but not robustly (Please check my latest comment)
- In
PathExpandedcases where cell-internal pins get collapsed,previs a load pin on a different instance — the== driver_input_pincomparison never matches the real driver input, and may spuriously skip an unrelated fanout.
network_->findPin(drvr, in_arc->from()) is "the input port of the arc ending at drvr" resolved against the current driver instance. Correct pin by construction, regardless of PathExpanded quirks or allocator behavior. The skip now does what the variable name promises — so side-fanout iteration in repair_timing should be strictly more correct after this change.
There was a problem hiding this comment.
early-return on in_arc == nullptr in all 4 sites (genclk source / latch
D->Q roots) means those paths are now skipped by size/swap/unbuffer/recover-power
instead of using whatever the old prevPath()->pin() returned. Any QoR delta on
designs with generated clocks or latch-based pipelines
There was a problem hiding this comment.
No expected QoR delta.
in_arc == nullptr only fires at genuine path startpoints with no upstream arc. Latch D→Q has a real TimingArc → prevArc() returns it, unaffected. Clock-source BTerms are filtered earlier by isTopLevelPort/isLogicStdCell. Generated-clock source pins either go through a divider cell (has input→output arc) or are BTerms (filtered).
At those true startpoints the old code wasn't doing useful work anyway — prevPath()->pin() was either null (crash) or an unrelated pin feeding the wrong port into upsizeCell (crash or luck-based sizing). New code just skips cleanly.
openroad-ci
force-pushed
the
secure-fix-10210-sizeup-stale-prev-pin
branch
from
606343a to
a3523e7
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| // 3. prev_path_ on a driver-output path lands on the driver's own input | ||
| // pin -- same instance -- which is the master invariant the fix | ||
| // preserves without dereferencing the raw pointer. | ||
| TEST_F(BufRemTest, PrevArcFromMatchesDriverInputPort) |
There was a problem hiding this comment.
This test is not related to buffer removal. So here is not a good place.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| auto* pins = new sta::PinSet(sta_->network()); | ||
| pins->insert(db_network_->dbToSta(bt)); | ||
| auto* waveform = new sta::FloatSeq{0.0f, 0.1f}; | ||
| sta_->makeClock(clk_name, pins, false, 0.2f, waveform, "", sta_->cmdMode()); |
There was a problem hiding this comment.
warning: reference to type 'const PinSet' could not bind to an lvalue of type 'sta::PinSet *' [clang-diagnostic-error]
sta_->makeClock(clk_name, pins, false, 0.2f, waveform, "", sta_->cmdMode());
^Additional context
src/sta/include/sta/Sta.hh:349: passing argument to parameter 'pins' here
const PinSet &pins,
^
Root cause: stale
|
| Build | Result |
|---|---|
Pre-fix (drvr_path->prevPath()->pin(sta_)) |
SIGSEGV in doMove on the stale prev |
Post-fix (prevArc(sta_)->from()) |
clean early-return, exit 0 |
Why prevArc()->from() fixes it
Path::prevArc() does not dereference prev_path_. It resolves an integer edge id stored on the Path against the current graph (graph->arc(id)). The returned TimingArc and TimingArc::from() are owned by the liberty cell — stable across all of repair_timing. The stale-deref surface is eliminated: an invalid edge id yields a null TimingArc and the move cleanly early-returns.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@dsengupta0628 approve or are there unresolved concerns? |
|
All good. |
The driver cell's input LibertyPort on a timing path is read from drvr_path->prevPath()->pin() in SizeUpMove, SwapPinsMove, UnbufferMove and RecoverPower::downsizeDrvr. When PathExpanded collapses intermediate cell pins, that pin can belong to a different instance than the driver, yielding a port name that does not exist on the driver's swappable cells. upsizeCell/downsizeCell then call LibertyCell::findLibertyPort with that name, which returns nullptr, and the subsequent capacitance() dereference crashes with SIGSEGV. Read the driver's input port from drvr_path->prevArc()->from() instead. The timing arc is carried on the path itself and is immune to PathExpanded pin collapsing. Also bail out early when prevArc is null (generated clock source or latch D->Q roots). See issue The-OpenROAD-Project#10210. Signed-off-by: Minju Kim <mkim@precisioninno.com>
Fixes clang-tidy misc-include-cleaner warning: these files call drvr_path->prevArc(sta_)->from() and use sta::TimingArc directly, so the header must be included directly rather than via transitive include. Signed-off-by: Minju Kim <mkim@precisioninno.com>
When drvr_path->prevArc() returns null (rare genclk source / latch D->Q), prevPath() is also null and dereferencing it in the subsequent prevPath()->prevPath() call would SIGSEGV. Hoist the null-check on the resolved input LibertyPort (or LibertyPort+pin in UnbufferMove) to immediately after it is computed, before any further path traversal. Also matches the pattern already used in SwapPinsMove and avoids wasted prev_drive / prev_drvr work on the reject path. In UnbufferMove this also keeps drvr_input_pin non-null on the happy path, so estimatedSlackOK correctly skips the buffer instance being removed when scanning the driving net's fanouts. Addresses PR review feedback on The-OpenROAD-Project#10225 (issue The-OpenROAD-Project#10210). Signed-off-by: Minju Kim <mkim@precisioninno.com>
Covers the -sequence "swap,sizeup,unbuffer" path that triggered the SIGSEGV in issue The-OpenROAD-Project#10210. Exercises SwapPinsMove, SizeUpMove, and UnbufferMove, each of which now resolves the driver input port via prev_arc->from() instead of prevPath()->pin(). Registered for both CMake and Bazel. Signed-off-by: Minju Kim <mkim@precisioninno.com>
Reverts the test added for issue The-OpenROAD-Project#10210. The UAF in prevPath()->pin() requires specific post-CTS path-enumeration and allocator timing that no minimal public testcase (Nangate45 small, ASAP7 gcd 3-tier VT, ASAP7 aes_place.def at 22k components) reproduces under ASAN. The Tcl test therefore exercises the code path but does not actually guard the regression -- reverting it would still pass. Verification of the fix is via the original customer post-CTS ODB under ASAN. Keeping a non-guarding test file adds noise without signal. Signed-off-by: Minju Kim <mkim@precisioninno.com>
…nce + UAF proof + stability) Three regression tests pin the The-OpenROAD-Project#10210 fix in SizeUpMove / SwapPinsMove / UnbufferMove / RecoverPower. The fix replaces drvr_path->prevPath()->pin()->libertyPort() (pre-fix) and expanded->path(drvr_index - 1)->pin() (b984a8d era) with drvr_path->prevArc()->from(), which reads prev_edge_id_/prev_arc_idx_ off the Path object itself -- no raw prev_path_ dereference, no expanded-vector indirection. 1. PrevArcFromMatchesDriverInputPort: equivalence test. On the BufRemTest buffer chain, prevArc()->from() resolves to the driver's own input LibertyPort, and agrees with the pre-fix prevPath()->pin()->libertyPort() form on non-stale paths. 2. PrevArcFromStableAcrossCellReplace: stability test. After Sta::replaceCell + updateTiming perturbs the chain, a fresh drvr_path query at b3/Z still returns the same LibertyPort (b3's A port) via prevArc()->from(), both in absolute terms and relative to the pre-perturbation result. Confirms the fix is semantically stable across the topology change that causes the UAF; does not dereference any stale pointer. 3. ExpandedPathPointerGoesStaleAfterCellReplace: deterministic UAF proof. After Sta::replaceCell + updateTiming triggers Search::setVertexArrivals -> Vertex::makePaths (delete[] paths_; new Path[N]), the Path pointer previously captured inside a PathExpanded vector lies outside the vertex's live paths_ range. Detection uses pointer-address comparison only (never dereferences the stale pointer, so it is ASAN-safe); when the allocator happens to reuse the same base the test skips. On glibc with Nangate45 it deterministically exercises the staleness path. The test observes three OpenSTA implementation details (PathExpanded storing raw Path* pointers, Vertex::makePaths doing delete[]/new[], Vertex::paths() returning the array base). If OpenSTA refactors any of these -- e.g., arena/pool allocation with stable addresses, in-place grow, or PathExpanded value-copying -- the test will skip rather than fail, which is an acceptable signal that the UAF shape no longer applies. The equivalence test remains the semantic guard regardless. Signed-off-by: Minju Kim <mkim@precisioninno.com>
Add <cstddef> / <cstdint> for size_t / uintptr_t used in the UAF test, and drop the int-to-pointer cast in the EXPECT_TRUE stream message by printing the live-range as 'base + N bytes' instead of a synthesized void*. Signed-off-by: Minju Kim <mkim@precisioninno.com>
…le Path* regression
Replaces the abandoned pointer-equality UAF test that was part of
TestBufferRemoval.cc with a standalone gtest exercising the actual
crash path:
1. Capture drvr_path at nd1/ZN.
2. Delete upstream b1 + updateTiming -> free b1/inv1 Path[] slots.
3. Add a fresh cone + clk2 + updateTiming -> recycle those slots.
4. Assert drvr_path->prevPath()->pin() decodes to a stale pin
(Phase-2 BTerm, not nd1's real input A1/A2).
5. Call SizeUpMove::doMove.
With the fix reverted the call SIGSEGVs on the stale prev pointer.
With the fix applied (prev_arc->from()) the stale deref is avoided
and doMove cleanly early-returns.
Registered in both CMake and Bazel.
Signed-off-by: Minju Kim <mkim@precisioninno.com>
Add direct stale-pointer signature check: snapshot drvr_path->prevPath() address and its decoded pin name BEFORE the free+recycle phases, then assert after-mutation that the pointer address is unchanged while the decoded pin differs -- raw pointer not refreshed by STA's incremental update, but underlying slot was freed and reused. Also fix misc-include-cleaner warnings on <string>, odb/dbTypes.h, and sta/SdcClass.hh. Signed-off-by: Minju Kim <mkim@precisioninno.com>
Bazel CI failed to compile the test because `SizeUpMove.hh` is a private rsz impl header and the Bazel cc_test did not have `src/rsz/src` on the include path (CMake's target_include_directories covered it, so local CMake build was green). Add the same `copts = ["-Isrc/rsz/src"]` + `features = ["-layering_check"]` pattern used by //src/web/test targets that reach into their module's internal headers. With the test now building under Bazel the strict assertion `drvr_path->pin() == "nd1/ZN"` started failing in sandbox (Bazel runs under a different allocator: the drvr Path[] slot itself got recycled with `ndnew/ZN` content, while CMake/glibc keeps the drvr slot and only recycles prev). Either form is legitimate stale-slot evidence the fix has to tolerate. Relax step 4 to accept both: assert "at least one slot got recycled (drvr or prev)", and only require the tight same- address-different-decode signature when the drvr slot was preserved. Also fix misc-include-cleaner warning on `std::fprintf` (`<cstdio>`). Signed-off-by: Minju Kim <mkim@precisioninno.com>
Slim the The-OpenROAD-Project#10210 regression down to the essential claim -- that the raw prev_path_ pointer survives free+reuse of its underlying slot: - Shrink the Phase-3 recycle trigger from a 3-stage BUF/INV/NAND2 cone + 2 extra BTerms + 3 nets to a single fresh BUF + clock. Both glibc (CMake) and the Bazel sandbox allocator still reliably recycle the freed slot with just this one allocation. - Drop the SizeUpMove::doMove invocation and its diagnostic fprintf. The staleness itself is the regression the fix addresses; the crash-on-deref follow-up is no longer needed for the assertion to land. Also drops the #include of the private "SizeUpMove.hh" header and the Bazel copts/features hack that enabled it. - Rename file/fixture/test to topology-based names so future tests sharing the same small BUF->INV->NAND2+side_input circuit can just add a TEST_F alongside this one: TestPathExpandedStaleness.cc -> TestBufInvNand2.cc class StalePathTest -> class BufInvNand2Test SizeUpMoveSurvivesStale... -> StalePrevPathAfterUpdateTiming Net effect: -42 LOC, clearer file/test scoping, no internal-header dependency in Bazel. CMake + Bazel builds both pass, clang-tidy clean. Signed-off-by: Minju Kim <mkim@precisioninno.com>
…gratedFixture Signed-off-by: Minju Kim <mkim@precisioninno.com>
Collapse the dual allocator-branch assertion (drvr_recycled vs prev_recycled) into a single strict stale-pointer signature: same prev_path_ address, different decoded pin name. Rename pre/post snapshot vars for clarity and drop the unused drvr_pin_after local. Signed-off-by: Minju Kim <mkim@precisioninno.com>
…st info Tighten the test-name <-> verilog-file coupling so future stale-path variants can just drop a new TestDbSta_<Name>.v alongside a new TEST_F without touching the data list: use gtest's current_test_info() at runtime to build the filename, matching the pattern already used in src/rsz/test/cpp/TestInsertBuffer.cpp. Test renamed StalePrevPathAfterUpdateTiming -> StalePrevPath (the "AfterUpdateTiming" qualifier was redundant with the flow header). Verilog renamed TestDbSta_StalePath.v -> TestDbSta_StalePrevPath.v to match. Signed-off-by: Minju Kim <mkim@precisioninno.com>
openroad-ci
force-pushed
the
secure-fix-10210-sizeup-stale-prev-pin
branch
from
e3bc5cd to
5a31bf9
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
5 participants
Summary
SizeUpMove,SwapPinsMove,UnbufferMove, andRecoverPower::downsizeDrvrresolve the driver cell's input
LibertyPortviadrvr_path->prevPath()->pin(). WhenPathExpandedcollapses intermediatecell pins, that pin can belong to a different instance than the driver,
so the port name does not exist on any swappable cell returned by
getSwappableCells.LibertyCell::findLibertyPortthen returnsnullptrinsideupsizeCell/downsizeCell, and the nextLibertyPort::capacitance()call crashes with SIGSEGV.Read the driver's input port from
drvr_path->prevArc()->from()instead.The timing arc is carried on the path itself and is immune to
PathExpandedpin collapsing. An explicit null-check on the resolvedport short-circuits the rare genclk source / latch D->Q cases where
prevArcis absent.Type of Change
Impact
Fixes deterministic SIGSEGV in
repair_timing -setupon a customer designwhen
-sequence swap,sizeup,unbuffer -repair_tns 100 -max_passes 100 -max_repairs_per_pass 2. No behavior change on paths where the oldprevPath()->pin()lookup already returned the driver's own input pin.Verification
./etc/Build.shsucceeds.SizeUpMove::doMovecall [replace/fastroute] Update replace to have routability-driven feature. #237 now completesrepair_timingtocompletion (
PASSED).src/rsz/testregressions run clean.clang-formatclean on the four touched files.Related Issues
See issue #10210.