Fix bpf2bpf bug when multiple branches in a subprogram go to exit #715
Conversation
The bug was that the successor label could be processed multiple times if there were multiple predecessors, resulting in multiple exit instructions appearing in the block, which in turn would result in restore_callee_saved_registers() being called multiple times, giving incorrect results. Added a YAML test case for this. Signed-off-by: Dave Thaler <[email protected]>
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range comments (3)
test-data/calllocal.yaml (3)
Line range hint
230-246: LGTM: Important test for disallowing tail calls in subprograms.This test case effectively verifies that tail calls are not allowed within subprograms, which is crucial for maintaining the integrity of the call stack.
Consider enhancing the post-conditions to check the states of other registers (R1, R2, R3) to ensure they remain unchanged after the disallowed tail call attempt.
Line range hint
258-271: LGTM: Important test for preventing infinite recursion between subprograms.This test case effectively verifies that infinite recursion between two subprograms is detected and prevented, which is crucial for avoiding stack overflow issues in more complex scenarios. The error message is clear and accurately describes the problem.
Consider adding a test case with a finite number of allowed recursive calls to ensure that legitimate recursive patterns are still permitted.
Line range hint
206-271: Overall assessment: Comprehensive enhancement of test coverage.The new test cases significantly improve the robustness of the eBPF verifier by covering various edge cases and potential issues:
- Register preservation in subroutine calls
- Multiple branches within subprograms
- Disallowing tail calls in subprograms
- Prevention of infinite recursion (both self-recursion and between subprograms)
While not all test cases directly address the stated PR objective of fixing multiple branches in subprograms, they collectively contribute to a more thorough verification process. These additions will help ensure the correctness and safety of eBPF programs in various scenarios.
Consider adding more test cases that specifically target the interaction between multiple branches and register preservation to further address the main PR objective.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/asm_cfg.cpp (2 hunks)
- test-data/calllocal.yaml (1 hunks)
🔇 Additional comments (3)
test-data/calllocal.yaml (2)
206-213: LGTM: New test case for register preservation.This test case effectively verifies that the callee-saved register R6 is preserved after a subroutine call. It aligns well with the PR objective of ensuring correct register handling in subroutine calls.
Line range hint
247-257: LGTM: Crucial test for preventing infinite self-recursion.This test case effectively verifies that infinite self-recursion is detected and prevented, which is essential for avoiding stack overflow issues. The error message is clear and accurately describes the problem.
src/asm_cfg.cpp (1)
62-66:⚠️ Potential issuePotential impact of changing
macro_labelsfromstd::queuetostd::setThe change from using a
std::queue<label_t>to astd::set<label_t>formacro_labels(lines 62-66) alters the data structure managing pending labels. Unlike a queue, a set does not preserve insertion order, which could affect the order in which labels are processed. If the processing order is significant for the correctness of CFG construction or for performance reasons, this change might introduce unintended behavior.To verify whether the order of label processing impacts the CFG construction, consider reviewing test cases that depend on specific traversal orders or add new tests to check for order-sensitive behavior.
Signed-off-by: Elazar Gershuni <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/asm_cfg.cpp (4 hunks)
🔇 Additional comments (3)
src/asm_cfg.cpp (3)
107-109: Logic for label tracking appears correctThe code correctly checks for the existence of
next_macro_labelinmacro_labelsbefore inserting it. The use ofseen_labelsensures that each label is processed only once.
61-65:⚠️ Potential issuePotential issue with processing order due to change from
std::queuetostd::setThe change from
std::queuetostd::setformacro_labelschanges the processing order from FIFO (First-In-First-Out) to an order determined by the set's sorting criteria. If the processing logic depends on the order in which labels are processed, this alteration could lead to unexpected behavior.Consider verifying whether the processing order of
macro_labelsaffects the correctness of the CFG construction. If order is significant, you might need to use an ordered container that maintains insertion order, likestd::queueorstd::deque, while still allowing efficient existence checks.
253-253:⚠️ Potential issueEnsure the basic block is not empty before dereferencing
When accessing
*bb.rbegin(), there's an implicit assumption thatbbis not empty. Dereferencing an iterator from an empty container leads to undefined behavior.Consider adding a check to ensure that
bbis not empty before dereferencing:if (!bb.empty()) { auto jmp = std::get<Jmp>(*bb.rbegin()); // Proceed with logic } else { // Handle the empty case if necessary }Alternatively, ensure through program logic that
bbcannot be empty at this point. Ifbbis guaranteed to be non-empty, a comment explaining this assumption can enhance code readability.
The bug was that the successor label could be processed multiple times if there were multiple predecessors, resulting in multiple exit instructions appearing in the block, which in turn would result in restore_callee_saved_registers() being called multiple times, giving incorrect results.
Added a YAML test case for this.
Summary by CodeRabbit
New Features
Bug Fixes
Tests