gpl: Fix multiple out-of-bounds, null dereference, and UB bugs in MBFF#10750
gpl: Fix multiple out-of-bounds, null dereference, and UB bugs in MBFF#10750calewis wants to merge 2 commits into
Conversation
calewis
force-pushed
the
fix-mbff-ub-safety
branch
from
d7d8f07 to
64bb23d
Compare
![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 introduces several robustness improvements to src/gpl/src/mbff.cpp to prevent out-of-bounds array accesses and null pointer dereferences during library reading. Key changes include adding checks for null cells, validating slot counts, verifying pin lookups, and simplifying slot coordinate calculations when pins are missing. The review feedback points out that the error-handling check for GetBitIdx returning -1 is redundant because logger_->error already terminates the program, and suggests using float literals (2.0f instead of 2.0) to avoid unnecessary double-precision promotions during coordinate calculations.
calewis
force-pushed
the
fix-mbff-ub-safety
branch
from
64bb23d to
971ef71
Compare
- Add bounds checks on output pin vectors in GetPinMapping to handle asymmetric pin configurations. - Validate that dbToSta and LibertyCell are non-null in ReadLibs before creating temporary test trays. - Add explicit null checks and graceful warn-and-skip logic on DB ITerms (d_pin, q_pin, qn_pin) in ReadLibs before calling getBBox() to prevent dereferencing null pointers. - Prevent out-of-bounds access on d vector in ReadLibs by verifying that d.size() is at least num_slots. - Prevent out-of-bounds access on q and qn vectors in ReadLibs' slot geometry loop by adding bounds checks and falling back to d coordinates. - Utilize single-precision float division literals (2.0f) in GetTrayCenter and slot geometry calculations to avoid unnecessary double promotions/demotions. Signed-off-by: Drew Lewis <cannada@google.com>
calewis
force-pushed
the
fix-mbff-ub-safety
branch
from
971ef71 to
7de82da
Compare
Signed-off-by: Drew Lewis <cannada@google.com>
|
@codex review |
| ret[d_pins[i]] = {nullptr, qn_pins[i]}; | ||
| } | ||
| for (size_t i = 0; i < d_pins.size(); i++) { | ||
| // Avoid out-of-bounds access if the cell has asymmetric output pin counts. |
There was a problem hiding this comment.
How could this happen for an MBFF? The bits are supposed to be symmetric. Is this a real problem or just a hypothetical one from a code analysis tool?
| = (p.second.qn ? tmp_tray->findITerm(p.second.qn->name().c_str()) | ||
| : nullptr); | ||
|
|
||
| if (d_pin == nullptr) { |
There was a problem hiding this comment.
Is this just defensive programming? The tray candidate should never mismatch like this so it seems redundant.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f32e64c5fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| const sta::LibertyPort* q_pin = (i < q_pins.size()) ? q_pins[i] : nullptr; | ||
| const sta::LibertyPort* qn_pin | ||
| = (i < qn_pins.size()) ? qn_pins[i] : nullptr; | ||
| ret[d_pins[i]] = {q_pin, qn_pin}; |
There was a problem hiding this comment.
When a tray has both Q and QN overall but fewer pins of one polarity than D, dbNetwork::isValidTray still accepts it because it only requires #D == max(q, qn). This padding records nullptr for the missing per-slot output while GetArrayMask still advertises that the tray supports both outputs, so if the ILP assigns a flop with that missing output to such a slot, ModifyPinConnections later dereferences q_pin/qn_pin while reconnecting the original output net and MBFF can crash instead of rejecting the candidate. Please discard candidates or slots that cannot provide all outputs required by the mask rather than padding them with nulls.
Useful? React with 👍 / 👎.
2 participants
Fix UB in the mbff code, note I don't full understand what this code is trying to do, I tried to pick what I thought were reasonable fixes in cases like where we would have indexed past the end of an array, but i'm not super confident in my choices so hopefully someone who is comfortable with this code can double check my UB avoiding solutions.