syn: fix stack-use-after-return in constant fold sliceDff, revised#10771
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 updates the return type of the Graph::add function from Bundle to BundleView in both its declaration and definition. The review feedback suggests updating the corresponding documentation comment to reflect this change. Additionally, it recommends optimizing the Bundle::Bundle(BundleView) constructor to prevent unnecessary heap allocations when callers assign the returned BundleView to a Bundle, particularly since Graph::add returns consecutive nets.
|
The LLM has good suggestions, I'll take them up later. |
sliceDff() was declared to return a BundleView (a non-owning view) but returned the owning Bundle produced by Graph::add<Dff>(). The implicit BundleView(const Bundle&) conversion stores a pointer to that temporary, which is destroyed when sliceDff() returns, so foldSequentials() then indexes a dangling view (BundleView::operator[] -> Bundle::operator[]). This is undefined behavior that optimized builds tolerate (sliceDff() is inlined, so the temporary's storage survives the read), but a DEBUG (unoptimized) build faults on. It reproduces as a SIGSEGV during synthesis of larger designs using the integrated syn flow (e.g. asap7/aes, asap7/jpeg with SYNTH_USE_SYN=1); AddressSanitizer reports a stack-use-after-return at ir/Bundle.cc. Return an owning Bundle from sliceDff() and hold it as a Bundle at the call site (a BundleView local would re-create the dangle). Signed-off-by: Matthew Guthaus <mrg@ucsc.edu>
Since the return value of `add` represents a contiguous block of net indices we can use `BundleView` for it which is both more efficient and prevents use-after-free should the caller cast to BundleView. Signed-off-by: Martin Povišer <povik@cutebit.org>
Signed-off-by: Martin Povišer <povik@cutebit.org>
openroad-ci
force-pushed
the
syn-fix-sliceDff-uar
branch
from
a815a5e to
060af12
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ 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". |
3 participants
Summary
This is a revised fix from #10768.
Since the return value of
addrepresents a contiguous block of net indices we can useBundleViewfor it which is both more efficient and prevents use-after-free should the caller cast to BundleView.Type of Change