Don't permit truncating non-numbers #772
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)test-data/pointer.yaml (1)🔇 Additional comments (3)test-data/pointer.yaml (3)Line range hint The test cases thoroughly cover all ALU operations (+, -, *, /, %, s/, s%, &, |, ^, <<, >>, s>>, NOT) and verify that attempting to perform 32-bit operations on pointers is properly caught with appropriate error messages.
The test case properly verifies that direct pointer assignment maintains the context type and offset, with appropriate pre-conditions, post-conditions, and error messages.
The test case effectively verifies that truncating a pointer to 32 bits triggers an invalid type warning. The test value 1234567890 is well-chosen as it fits within 32 bits while being large enough to be meaningful. 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: 1
🧹 Outside diff range comments (1)
test-data/pointer.yaml (1)
Line range hint
1-357: Inconsistent post-conditions in 32-bit pointer truncation testsAccording to the previous learning, the verifier doesn't emit post-conditions for 32-bit pointer truncation test cases. However, some test cases (multiplication, division, modulo, etc.) include post-conditions while others (addition, subtraction) don't. This inconsistency should be addressed.
Consider removing post-conditions from all 32-bit pointer truncation test cases to maintain consistency with the verifier's behavior.
🧰 Tools
🪛 GitHub Check: validate-yaml
[warning] 4-4:
4:1 [document-start] missing document start "---"
[warning] 4-4:
4:1 [document-start] missing document start "---"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
src/assertions.cpp(1 hunks)test-data/assign.yaml(6 hunks)test-data/pointer.yaml(2 hunks)
🧰 Additional context used
📓 Learnings (1)
test-data/pointer.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#742
File: test-data/pointer.yaml:1-357
Timestamp: 2024-10-25T18:49:20.625Z
Learning: In `test-data/pointer.yaml`, for the 32-bit pointer truncation test cases, the verifier doesn't emit post conditions, so adding post-conditions is not applicable.
🪛 yamllint
test-data/assign.yaml
[error] 442-442: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: validate-yaml
test-data/assign.yaml
[failure] 442-442:
442:42 [new-line-at-end-of-file] no new line character at the end of file
[failure] 442-442:
442:42 [new-line-at-end-of-file] no new line character at the end of file
🔇 Additional comments (9)
test-data/assign.yaml (6)
189-191: LGTM: Appropriate error message for stack pointer truncation.
The error message correctly prevents truncating a stack pointer to a 32-bit value, which could lead to memory corruption.
226-228: LGTM: Appropriate error message for shared memory pointer truncation.
The error message correctly prevents truncating a shared memory pointer to a 32-bit value, which could lead to memory corruption.
332-334: LGTM: Appropriate error message for packet pointer truncation.
The error message correctly prevents truncating a packet pointer to a 32-bit value, which could lead to memory corruption.
372-374: LGTM: Appropriate error message for context pointer truncation.
The error message correctly prevents truncating a context pointer to a 32-bit value, which could lead to memory corruption.
406-408: LGTM: Appropriate error message for map file descriptor truncation.
The error message correctly prevents truncating a map file descriptor to a 32-bit value, which could lead to resource leaks.
440-442: LGTM: Appropriate error message for map programs file descriptor truncation.
The error message correctly prevents truncating a map programs file descriptor to a 32-bit value, which could lead to resource leaks.
🧰 Tools
🪛 yamllint
[error] 442-442: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: validate-yaml
[failure] 442-442:
442:42 [new-line-at-end-of-file] no new line character at the end of file
[failure] 442-442:
442:42 [new-line-at-end-of-file] no new line character at the end of file
test-data/pointer.yaml (2)
359-394: LGTM! Well-structured test case for 64-bit pointer assignment
The test case thoroughly verifies that assigning a context pointer to a number register is not allowed, with comprehensive pre-conditions and post-conditions that validate both type and value propagation.
397-424: LGTM! Well-structured test case for 32-bit pointer truncation
The test case effectively verifies that truncating a context pointer to 32-bits is not allowed, with comprehensive pre-conditions and post-conditions that validate both type and value preservation.
src/assertions.cpp (1)
232-238: 🧹 Nitpick (assertive)
LGTM! Consider adding a clarifying comment.
The added type check correctly prevents truncation of non-number types during MOV operations. This is essential for type safety as truncating pointers or other non-number types could lead to memory corruption or undefined behavior.
Consider adding a comment explaining the rationale:
case Bin::Op::MOV:
if (const auto src = std::get_if<Reg>(&ins.v)) {
+ // Prevent truncation of pointers and other non-number types
+ // which could lead to memory corruption
if (!ins.is64) {
return {Assert{TypeConstraint{*src, TypeGroup::number}}};
}
}
return {};Let's verify the test coverage for this change:
✅ Verification successful
Let me check the content of the test file to understand the coverage better.
Let me check for additional test files that might cover MOV operations, particularly focusing on type-related tests.
Let me check the content of movsx.yaml and assign.yaml as they are most likely to contain relevant test cases for non-64-bit MOV operations and type constraints.
Based on the comprehensive test coverage found in the test files, particularly in movsx.yaml and assign.yaml, I can now generate the final response:
The type check for non-64-bit MOV operations is well-verified
The test files demonstrate thorough coverage of type checking for MOV operations:
- Test cases verify that non-number types are rejected for truncating moves (32-bit and smaller)
- Both regular MOV and sign-extended MOV operations are tested
- Type constraints are properly enforced across different bit widths (8, 16, 32-bit)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering non-64-bit MOV operations
rg -l "MOV.*32|MOV.*16|MOV.*8" test-data/
Length of output: 65
Script:
#!/bin/bash
# Check the content of the test file for MOV operations
cat test-data/uninit.yaml
Length of output: 9462
Script:
#!/bin/bash
# Search for test files containing MOV operations and type checks
rg -l "type.*number|MOV" test-data/
Length of output: 556
Script:
#!/bin/bash
# Check content of relevant test files
echo "=== movsx.yaml ==="
cat test-data/movsx.yaml
echo -e "\n=== assign.yaml ==="
cat test-data/assign.yaml
Length of output: 14228
Signed-off-by: Alan Jowett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (4)
test-data/assign.yaml (4)
Line range hint
1-54: Consider adding edge case tests for basic assignments.The basic assignment test cases look good but could benefit from additional coverage:
- Immediate assignments with maximum/minimum values
- Register assignments with undefined source registers
- Assignments with different register sizes (8-bit, 16-bit)
Line range hint
55-151: Add explicit stack alignment test cases.While the stack assignment tests are comprehensive, consider adding test cases that explicitly verify:
- Stack access with different alignments (4-byte, 8-byte)
- Stack assignments at boundary conditions (near stack limits)
- Invalid stack access patterns
Line range hint
152-405: Enhance error path coverage for type-specific assignments.The type-specific assignment tests are thorough for success cases. Consider adding:
- Tests for invalid offset values in context/shared accesses
- Boundary tests for packet data access
- Tests for invalid map_fd values
Line range hint
189-442: Consider enhancing error messages for clarity.The error messages for invalid type truncation are consistent but could be more descriptive. Instead of:
"0: Invalid type (r1.type == number)"Consider:
"0: Cannot truncate non-numeric type to 32-bits (expected: number, actual: {actual_type})"This would make debugging easier by explicitly stating the actual type that was encountered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
src/assertions.cpp(1 hunks)test-data/assign.yaml(6 hunks)test-data/pointer.yaml(2 hunks)
🧰 Additional context used
📓 Learnings (1)
test-data/pointer.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#742
File: test-data/pointer.yaml:1-357
Timestamp: 2024-10-25T18:49:20.625Z
Learning: In `test-data/pointer.yaml`, for the 32-bit pointer truncation test cases, the verifier doesn't emit post conditions, so adding post-conditions is not applicable.
🔇 Additional comments (4)
test-data/assign.yaml (1)
442-442: Add newline at end of file.
test-data/pointer.yaml (2)
Line range hint 1-357: LGTM! Comprehensive test coverage for 32-bit pointer truncation operations.
The test cases thoroughly cover all ALU operations while maintaining consistent pre-conditions, post-conditions, and error messages. Each operation is tested with values that shouldn't change the result (e.g., += 0, *= 1) to isolate the type checking behavior.
🧰 Tools
🪛 GitHub Check: validate-yaml
[warning] 4-4:
4:1 [document-start] missing document start "---"
[warning] 4-4:
4:1 [document-start] missing document start "---"
358-395: LGTM! Well-structured test case for 64-bit pointer context.
The test case effectively verifies type safety for pointer assignments, with comprehensive post-conditions checking both value preservation and type propagation.
src/assertions.cpp (1)
232-238: LGTM! Type safety enhancement for truncating MOV operations.
The implementation correctly prevents truncation of non-number types by adding type constraints for non-64-bit MOV operations with register sources. This aligns with the PR objective and makes the MOV operation's type checking consistent with other move operations.
Let's verify the impact of this stricter type checking:
✅ Verification successful
The type constraint for non-64-bit MOV operations is consistent with existing patterns
Based on the verification results:
- The codebase already enforces similar type constraints for other operations (MOVSX) as seen in test-data/movsx.yaml
- The TypeGroup::number constraint is consistently used across the codebase for numeric operations
- The change aligns with existing type safety patterns in src/assertions.cpp
The implementation is correct and follows established patterns in the codebase for preventing type-unsafe operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential MOV operations that might be affected by the new type constraint
# Look for MOV instructions that might be truncating values
rg -A 3 "(?i)\bmov\b.*(?:32|16|8)"
# Look for test cases related to MOV operations
rg -A 5 "mov.*:" test-data/
Length of output: 66
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Look for test files that might contain MOV test cases
fd -e txt -e data test-data/
# 2. Search for MOV instructions in assembly-like syntax
rg -i "mov.*[^a-z]" test-data/
# 3. Look for test cases or verifier code related to MOV operations
rg -l "MOV" src/
# 4. Search for any type verification or assertion tests
rg "TypeGroup::number" src/
Length of output: 4341
Signed-off-by: Alan Jowett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
test-data/pointer.yaml (1)
Inconsistent post-conditions need to be fixed in 32-bit pointer truncation test cases
Based on the retrieved learning and the verification results, there are inconsistencies in the post-conditions:
- Addition and subtraction test cases correctly have empty post-conditions (
post: [])- Other test cases incorrectly include non-empty post-conditions
- The NOT test case has a different post-condition format
All test cases should follow the same pattern as addition and subtraction with empty post-conditions (
post: []), as per the learning that "the verifier doesn't emit post conditions for 32-bit pointer truncation test cases."🔗 Analysis chain
Line range hint
3-357: Verify post-conditions in 32-bit pointer truncation test casesAccording to the retrieved learning, the verifier doesn't emit post conditions for 32-bit pointer truncation test cases. However, some test cases include post-conditions. Please verify if these should be removed for consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check which 32-bit pointer truncation test cases have post conditions rg -A 1 "test-case: 32-bit pointer truncation.*\npost:" test-data/pointer.yamlLength of output: 272
Script:
#!/bin/bash # Description: Check which 32-bit pointer truncation test cases have post conditions rg -U -A 1 "test-case: 32-bit pointer truncation.*\npost:" test-data/pointer.yamlLength of output: 83
Script:
#!/bin/bash # Let's try a different approach by looking at the content between test cases rg -U "test-case: 32-bit pointer truncation.*(?s).*?---" test-data/pointer.yamlLength of output: 8028
🧰 Tools
🪛 GitHub Check: validate-yaml
[warning] 4-4:
4:1 [document-start] missing document start "---"
[warning] 4-4:
4:1 [document-start] missing document start "---"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
test-data/pointer.yaml(2 hunks)
🧰 Additional context used
📓 Learnings (1)
test-data/pointer.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#742
File: test-data/pointer.yaml:1-357
Timestamp: 2024-10-25T18:49:20.625Z
Learning: In `test-data/pointer.yaml`, for the 32-bit pointer truncation test cases, the verifier doesn't emit post conditions, so adding post-conditions is not applicable.
🔇 Additional comments (4)
test-data/pointer.yaml (4)
1-2: LGTM!
The license header is properly formatted with copyright notice and SPDX identifier.
Line range hint 3-357: LGTM! Comprehensive test coverage for ALU operations
The test cases thoroughly cover all ALU operations (addition, subtraction, multiplication, division, modulo, signed operations, bitwise operations, and shifts) with consistent structure and validation messages.
🧰 Tools
🪛 GitHub Check: validate-yaml
[warning] 4-4:
4:1 [document-start] missing document start "---"
[warning] 4-4:
4:1 [document-start] missing document start "---"
359-395: LGTM! Well-structured test for pointer assignment
The test case effectively verifies the behavior of assigning a context pointer to the return register, with comprehensive pre and post conditions checking type conversion and value preservation.
396-425: LGTM! Clear test case for pointer truncation warning
The test case effectively verifies that truncating a pointer to 32-bit and converting it to a number triggers a warning. The pre and post conditions comprehensively validate the behavior.
Signed-off-by: Alan Jowett <[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)
test-data/pointer.yaml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
test-data/pointer.yaml (1)
Learnt from: Alan-Jowett
PR: vbpf/ebpf-verifier#742
File: test-data/pointer.yaml:1-357
Timestamp: 2024-10-25T18:49:20.625Z
Learning: In `test-data/pointer.yaml`, for the 32-bit pointer truncation test cases, the verifier doesn't emit post conditions, so adding post-conditions is not applicable.
🔇 Additional comments (2)
test-data/pointer.yaml (2)
359-394: LGTM! Well-structured test case for 64-bit pointer context.
The test case effectively verifies that assigning a context pointer to the return register preserves type information and triggers appropriate type checking. The pre and post conditions are comprehensive and properly validate the behavior.
414-423: Verify post-conditions behavior with verifier.
Based on previous learnings from PR #742, the verifier doesn't emit post-conditions for 32-bit pointer truncation test cases. However, this test case includes post-conditions. Please verify if this is intentional or if the verifier behavior has changed.
Signed-off-by: Alan Jowett <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Tests