nexus: fix GroupId::operator!= typo and UAF in pack_iodelay - #1661
Closed
taskx6004 wants to merge 1 commit into
Closed
nexus: fix GroupId::operator!= typo and UAF in pack_iodelay#1661taskx6004 wants to merge 1 commit into
taskx6004 wants to merge 1 commit into
Conversation
archdefs.h: GroupId::operator!= had `y == other.y` instead of `y != other.y`, making inequality checks on groups incorrect. pack.cc: In pack_iodelay(), the line `base_iodelay_rules[id_DELAYA] = base_iodelay_rules[id_DELAYB]` causes use-after-free when the dict rehashes during insert — the reference to `base_iodelay_rules[id_DELAYB]` on the RHS becomes dangling when `operator[]` on the LHS triggers a rehash. Fixed by copying the value before inserting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
|
I think these are reasonable changes but I'd need to check the rehashing part (also no need for the std::move in the usual nextpnr style for non-perf-critical sections). However after the amount of upset it caused last time I'd prefer it if you resubmitted this without Claude. As for the various LIFCL-33U PRs, this work is currently ongoing in gatecat/prjoxide#49 along with bigger prjoxide changes and more extensive testing/change handling so I won't be merging that. |
Author
|
fair enough. If need brute force with something, I have the board and time
for help.
…On Sun, Mar 8, 2026 at 5:36 AM myrtle ***@***.***> wrote:
*gatecat* left a comment (YosysHQ/nextpnr#1661)
<#1661 (comment)>
I think these are reasonable changes but I'd need to check the rehashing
part (also no need for the std::move in the usual nextpnr style for
non-perf-critical sections).
However after the amount of upset it caused last time I'd prefer it if you
resubmitted this without Claude.
As for the various LIFCL-33U PRs, this work is currently ongoing in
gatecat/prjoxide#49 <gatecat/prjoxide#49> along
with bigger prjoxide changes and more extensive testing/change handling so
I won't be merging that.
—
Reply to this email directly, view it on GitHub
<#1661 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3EWQAMQDJ33QFVKJXYNS34PU5K3AVCNFSM6AAAAACWK3FNX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DAMJYG4ZTAMBYHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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.
Summary
GroupId::operator!=usesy == other.yinstead ofy != other.yin the last term, making the operator always return true when onlyydiffers. This is a copy-paste error fromoperator==.base_iodelay_rules[id_DELAYA] = base_iodelay_rules[id_DELAYB]is undefined behavior —operator[]on the LHS may rehash the dict, invalidating the reference returned byoperator[]on the RHS. Fixed by copying the value to a local before inserting.Both bugs were found while working on LIFCL-33U bringup but are device-independent.
Test plan
🤖 Generated with Claude Code