Conversation
…terminator, remove file pointer cases 1. Add headers, Adding missing headers: For obvious reasons. 2. Remove cases without null terminator: Both clang and g++ do not permit strings to be allocated that are declared to be shorter than the actual initializing expression. Since this is a C++ rule, we rule them out. 3. File pointer manipulation functions (e.g. fgets): Not required by the rule.
Too many of the negative offsets alerts were false positives. We leave it for future work.
There was a problem hiding this comment.
Pull request overview
This PR implements MISRA C++ 2023 Rule 8-7-1 (Memory1 package) which validates pointer arithmetic to ensure pointers don't exceed array bounds. The PR updates the rules.csv to organize memory-related rules into separate packages (Memory1-Memory6) and adds two queries for detecting invalid pointer arithmetic.
Changes:
- Implements two queries for RULE-8-7-1: PointerArithmeticFormsAnInvalidPointer (path-problem) and PointerArgumentToCstringFunctionIsInvalid (problem)
- Adds a comprehensive OutOfBounds.qll module (1358 lines) for buffer overflow analysis
- Updates rules.csv to assign memory-related rules to Memory1-Memory6 packages
- Adds Memory1.json rule package description file and Memory1.qll exclusions file
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rules.csv | Updates package names from "Memory" to "Memory1" through "Memory6" for memory-related rules |
| rule_packages/cpp/Memory1.json | Adds package description for Memory1 with metadata for both queries |
| cpp/misra/src/rules/RULE-8-7-1/PointerArithmeticFormsAnInvalidPointer.ql | Implements path-problem query for detecting invalid pointer arithmetic |
| cpp/misra/src/rules/RULE-8-7-1/PointerArgumentToCstringFunctionIsInvalid.ql | Implements problem query for invalid cstring function arguments |
| cpp/misra/test/rules/RULE-8-7-1/test.cpp | Comprehensive test file with 519 lines covering various pointer arithmetic scenarios |
| cpp/misra/test/rules/RULE-8-7-1/*.expected | Expected results files for both queries |
| cpp/misra/test/rules/RULE-8-7-1/*.qlref | Query reference files |
| cpp/common/src/codingstandards/cpp/OutOfBounds.qll | New module providing buffer overflow analysis infrastructure |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/Memory1.qll | Auto-generated exclusions file for Memory1 queries |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Updates to include Memory1 query metadata |
| cpp/common/test/includes/standard-library/*.h | Adds missing C standard library function declarations |
| cpp/common/test/includes/standard-library/cstdlib | Adds using declarations for malloc, calloc, realloc |
Comments suppressed due to low confidence (1)
cpp/misra/src/rules/RULE-8-7-1/PointerArithmeticFormsAnInvalidPointer.ql:171
- Similar to the PointerAddExpr case, this predicate uses getAnOperand() which could match either operand. For PointerSubExpr, you need to specifically get the right operand (the value being subtracted). The current implementation could incorrectly negate the pointer operand instead of the offset value.
exists(PointerSubExpr pointerSubtraction | pointerSubtraction = this.asPointerArithmetic() |
result = -pointerSubtraction.getAnOperand().getValue().toInt()
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import codingstandards.cpp.exclusions.c.RuleMetadata | ||
|
|
||
| from | ||
| OOB::BufferAccessLibraryFunctionCall fc, string message, Expr bufferArg, string bufferArgStr, | ||
| Expr sizeOrOtherBufferArg, string otherStr | ||
| where | ||
| not isExcluded(fc, OutOfBoundsPackage::libraryFunctionArgumentOutOfBoundsQuery()) and |
There was a problem hiding this comment.
This query imports from codingstandards.cpp.exclusions.c.RuleMetadata but the OutOfBoundsPackage::libraryFunctionArgumentOutOfBoundsQuery is defined for C, not C++. This is a MISRA C++ rule (RULE-8-7-1), so it should either use the C++ exclusions system or verify that using the C exclusions is intentional. This could cause issues with the exclusion system not properly recognizing this as a C++ query.
There was a problem hiding this comment.
I believe this should be not isExcluded(Memory1Package::theQueryName())
| } | ||
|
|
||
| /** | ||
| * The |
There was a problem hiding this comment.
Incomplete documentation: The class documentation starts with "The" but is incomplete. Please complete the documentation explaining what this class represents, such as "A heap allocation function call that is cast to a specific pointer type."
| * The | |
| * A heap allocation function call that is cast to a specific pointer type. |
| result = pointerAddition.getAnOperand().getValue().toInt() // TODO: only get the number being added | ||
| ) | ||
| or | ||
| exists(PointerSubExpr pointerSubtraction | pointerSubtraction = this.asPointerArithmetic() | | ||
| result = -pointerSubtraction.getAnOperand().getValue().toInt() |
There was a problem hiding this comment.
The getOffset predicate has a TODO comment indicating incomplete implementation. For PointerAddExpr, the current logic uses getAnOperand() which could match either the pointer or the integer operand. This should specifically get the right operand (the offset value) to avoid incorrect results. Consider using getRightOperand() or a similar specific accessor to ensure the correct operand is used.
| result = pointerAddition.getAnOperand().getValue().toInt() // TODO: only get the number being added | |
| ) | |
| or | |
| exists(PointerSubExpr pointerSubtraction | pointerSubtraction = this.asPointerArithmetic() | | |
| result = -pointerSubtraction.getAnOperand().getValue().toInt() | |
| result = pointerAddition.getRightOperand().getValue().toInt() | |
| ) | |
| or | |
| exists(PointerSubExpr pointerSubtraction | pointerSubtraction = this.asPointerArithmetic() | | |
| result = -pointerSubtraction.getRightOperand().getValue().toInt() |
There was a problem hiding this comment.
Let's handle this todo, but not how copilot is suggesting, so that we can handle both p + n and n + p.
MichaelRFairhurst
left a comment
MichaelRFairhurst
left a comment
There was a problem hiding this comment.
Very nice work, Jeongsoo! The work you've put into this really shows, its elegant and focused. In terms of the taint tracking edge cases, I think we can handle those cleanly on your foundation here if we focus on the array-to-pointer conversion cases, which we can talk more about later! Also, I have to say the tests you've made are awesomely comprehensive, nicely done. That is huge and really shows all your attention to detail!
| /** | ||
| * This module provides classes and predicates for analyzing the size of buffers | ||
| * or objects from their base or a byte-offset, and identifying the potential for | ||
| * expressions accessing those buffers to overflow. |
There was a problem hiding this comment.
Is this a direct copy? We should probably state that, and/or, list modifications that have been made.
| import cpp | ||
| import codingstandards.cpp.OutOfBounds // for OOB::problems | ||
| import codingstandards.cpp.Exclusions // for isExcluded(Element, Query) | ||
| import codingstandards.cpp.exclusions.c.RuleMetadata |
There was a problem hiding this comment.
I think this should be codingstandards.cpp.exclusions.cpp.RuleMetadata, or deleted?
| import codingstandards.cpp.exclusions.c.RuleMetadata | ||
|
|
||
| from | ||
| OOB::BufferAccessLibraryFunctionCall fc, string message, Expr bufferArg, string bufferArgStr, | ||
| Expr sizeOrOtherBufferArg, string otherStr | ||
| where | ||
| not isExcluded(fc, OutOfBoundsPackage::libraryFunctionArgumentOutOfBoundsQuery()) and |
There was a problem hiding this comment.
I believe this should be not isExcluded(Memory1Package::theQueryName())
| /** | ||
| * A call to a function that dynamically allocates memory on the heap. | ||
| */ | ||
| class HeapAllocationFunctionCall extends FunctionCall { |
There was a problem hiding this comment.
Maybe we should put this logic in common somewhere, maybe standardlibrary/memory?
| CallocFunctionCall() { this.isCallocCall() } | ||
|
|
||
| override int getMinNumBytes() { | ||
| result = lowerBound(this.getArgument(0)) * lowerBound(this.getArgument(1)) |
There was a problem hiding this comment.
Do we want to use the minimum, or the maximum?
I'd suggest we run this on MRVA and see how many false positives we get. If it is a lot, I'd suggest using upperBound().
| * Gets the offset of this pointer formation as calculated in relation to the base pointer. | ||
| */ | ||
| int getOffset() { | ||
| result = this.asArrayExpr().getArrayOffset().getValue().toInt() |
There was a problem hiding this comment.
instead of getArrayOffset().getValue().toInt(), which only handles constants, we likely want to use range analysis, either upperBound (noisiest) or lowerBound (quietest).
| result = pointerAddition.getAnOperand().getValue().toInt() // TODO: only get the number being added | ||
| ) | ||
| or | ||
| exists(PointerSubExpr pointerSubtraction | pointerSubtraction = this.asPointerArithmetic() | | ||
| result = -pointerSubtraction.getAnOperand().getValue().toInt() |
There was a problem hiding this comment.
Let's handle this todo, but not how copilot is suggesting, so that we can handle both p + n and n + p.
cpp/misra/src/rules/RULE-8-7-1/PointerArithmeticFormsAnInvalidPointer.ql
Outdated
Show resolved
Hide resolved
…/MISRA-C++-2023-Memory-Experimental
…w-nodes-MISRA-C++-2023-Memory-Experimental' into jeongsoolee09/MISRA-C++-2023-Memory-Experimental
Description
Implement Memory1 (
RULE-8-7-1) and add rule package description files for the rest of the rules (Memory2-Memory6).Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
RULE-8-7-1Release change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
Query development review checklist
For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:
Author
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Reviewer
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.