-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement InvalidMemory3, Rule 18-8 amendment. #750
Open
MichaelRFairhurst
wants to merge
10
commits into
main
Choose a base branch
from
michaelrfairhurst/implement-package-invalid-memory3
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
dd475be
Implement InvalidMemory, Rule 18-8 amendment.
MichaelRFairhurst 5b5777f
Fix formatting
MichaelRFairhurst 01e51e8
Merge remote-tracking branch 'origin/main' into michaelrfairhurst/imp…
MichaelRFairhurst ab4a7bf
Merge branch 'main' into michaelrfairhurst/implement-package-invalid-…
MichaelRFairhurst 06ba26d
Address feedback
MichaelRFairhurst 7014931
Merge branch 'main' into michaelrfairhurst/implement-package-invalid-…
lcartey ef76126
Merge branch 'main' into michaelrfairhurst/implement-package-invalid-…
lcartey 791526e
Address next round of feedback
MichaelRFairhurst b477e34
Fix macro testcase formatting
MichaelRFairhurst 3873be7
Commit changes to expected files, improve problem messages
MichaelRFairhurst File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
122 changes: 122 additions & 0 deletions
122
c/misra/src/rules/RULE-18-10/PointersToVariablyModifiedArrayTypesUsed.ql
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
/** | ||
* @id c/misra/pointers-to-variably-modified-array-types-used | ||
* @name RULE-18-10: Pointers to variably-modified array types shall not be used | ||
* @description Pointers to variably-modified array types shall not be used, as these pointer types | ||
* are frequently incompatible with other fixed or variably sized arrays, resulting in | ||
* undefined behavior. | ||
* @kind problem | ||
* @precision high | ||
* @problem.severity error | ||
* @tags external/misra/id/rule-18-10 | ||
* external/misra/c/2012/amendment4 | ||
* correctness | ||
* security | ||
* external/misra/obligation/mandatory | ||
*/ | ||
|
||
import cpp | ||
import codingstandards.c.misra | ||
import codingstandards.cpp.VariablyModifiedTypes | ||
|
||
/** | ||
* Check that the declaration entry, which may be a parameter or a variable | ||
* etc., seems to subsume the location of `inner`, including the declaration | ||
* type text. | ||
* | ||
* The location of the `DeclarationEntry` itself points to the _identifier_ | ||
* that is declared. This range will not include the type of the declaration. | ||
* | ||
* For parameters, the `before` and `end` `Location` objects will be | ||
* constrained to the closest earlier element (parameter or function body), | ||
* these values can therefore be captured and inspected for debugging. | ||
* | ||
* For declarations which occur in statements, the `before` and `end` | ||
* `Location` objects will be both constrained to be equal, and equal to, | ||
* the `Location` of the containing `DeclStmt`. | ||
*/ | ||
predicate declarationSubsumes( | ||
DeclarationEntry entry, Location inner, Location before, Location after | ||
) { | ||
inner.getFile() = entry.getLocation().getFile() and | ||
( | ||
exists(ParameterDeclarationEntry param, FunctionDeclarationEntry func, int i | | ||
param = entry and | ||
func = param.getFunctionDeclarationEntry() and | ||
func.getParameterDeclarationEntry(i) = param and | ||
before = entry.getLocation() and | ||
( | ||
after = func.getParameterDeclarationEntry(i + 1).getLocation() | ||
or | ||
not exists(ParameterDeclarationEntry afterParam | | ||
afterParam = func.getParameterDeclarationEntry(i + 1) | ||
) and | ||
after = func.getBlock().getLocation() | ||
) | ||
) and | ||
before.isBefore(inner, _) and | ||
inner.isBefore(after, _) | ||
or | ||
exists(DeclStmt s | | ||
s.getADeclaration() = entry.getDeclaration() and | ||
before = s.getLocation() and | ||
after = before and | ||
before.subsumes(inner) | ||
) | ||
) | ||
} | ||
|
||
/** | ||
* A declaration involving a pointer to a variably-modified type. | ||
*/ | ||
class InvalidDeclaration extends DeclarationEntry { | ||
Expr sizeExpr; | ||
CandidateVlaType vlaType; | ||
// `before` and `after` are captured for debugging, see doc comment for | ||
// `declarationSubsumes`. | ||
Location before; | ||
Location after; | ||
|
||
InvalidDeclaration() { | ||
sizeExpr = any(VlaDimensionStmt vla).getDimensionExpr() and | ||
declarationSubsumes(this, sizeExpr.getLocation(), before, after) and | ||
lcartey marked this conversation as resolved.
Show resolved
Hide resolved
|
||
( | ||
if this instanceof ParameterDeclarationEntry | ||
then vlaType = this.getType().(VariablyModifiedTypeIfAdjusted).getInnerVlaType() | ||
else vlaType = this.getType().(VariablyModifiedTypeIfUnadjusted).getInnerVlaType() | ||
) and | ||
// Capture only pointers to VLA types, not raw VLA types. | ||
not vlaType = this.getType() | ||
} | ||
|
||
Expr getSizeExpr() { result = sizeExpr } | ||
|
||
CandidateVlaType getVlaType() { result = vlaType } | ||
} | ||
|
||
from InvalidDeclaration v, string declstr, string adjuststr, string relationstr | ||
where | ||
not isExcluded(v, InvalidMemory3Package::pointersToVariablyModifiedArrayTypesUsedQuery()) and | ||
( | ||
if v instanceof ParameterDeclarationEntry | ||
then declstr = "Parameter " | ||
else | ||
if v instanceof VariableDeclarationEntry | ||
then declstr = "Variable " | ||
else declstr = "Declaration " | ||
) and | ||
( | ||
if | ||
v instanceof ParameterDeclarationEntry and | ||
v.getType() instanceof ParameterAdjustedVariablyModifiedType | ||
then adjuststr = "adjusted to" | ||
else adjuststr = "declared with" | ||
) and | ||
( | ||
if v.getType().(PointerType).getBaseType() instanceof CandidateVlaType | ||
then relationstr = "pointer to" | ||
else relationstr = "with inner" | ||
) | ||
select v, | ||
declstr + v.getName() + " is " + adjuststr + " variably-modified type, " + relationstr + | ||
" variable length array of non constant size $@ and element type '" + | ||
v.getVlaType().getVariableBaseType() + "'", v.getSizeExpr(), v.getSizeExpr().toString() |
This file contains 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
86 changes: 86 additions & 0 deletions
86
c/misra/src/rules/RULE-18-9/ArrayToPointerConversionOfTemporaryObject.ql
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
/** | ||
* @id c/misra/array-to-pointer-conversion-of-temporary-object | ||
* @name RULE-18-9: An object with temporary lifetime shall not undergo array to pointer conversion | ||
* @description Modifying or accessing elements of an array with temporary lifetime that has been | ||
* converted to a pointer will result in undefined behavior. | ||
* @kind problem | ||
* @precision high | ||
* @problem.severity error | ||
* @tags external/misra/id/rule-18-9 | ||
* external/misra/c/2012/amendment3 | ||
* correctness | ||
* security | ||
* external/misra/obligation/required | ||
*/ | ||
|
||
import cpp | ||
import codingstandards.c.misra | ||
import codingstandards.cpp.lifetimes.CLifetimes | ||
|
||
/** | ||
* Get the expression(s) whose value is "used" by this expression. | ||
* | ||
* For instance, `(x)` does not use any values, but `x + y` uses `x` and `y`. | ||
* | ||
* A pointer-to-array conversion does not need to be flagged if the result of | ||
* that conversion is not used or stored. | ||
*/ | ||
Expr usedValuesOf(Expr expr) { | ||
MichaelRFairhurst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
result = expr.(BinaryOperation).getLeftOperand() | ||
or | ||
result = expr.(BinaryOperation).getRightOperand() | ||
or | ||
result = expr.(UnaryOperation).getOperand() | ||
MichaelRFairhurst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
or | ||
result = expr.(ConditionalExpr).getCondition() | ||
or | ||
result = expr.(Call).getAnArgument() | ||
} | ||
|
||
/** | ||
* Get the expression(s) whose value is stored by this declaration. | ||
* | ||
* A pointer-to-array conversion does not need to be flagged if the result of | ||
* that conversion is not used or stored. | ||
*/ | ||
predicate isStored(Expr e) { | ||
e = any(VariableDeclarationEntry d).getDeclaration().getInitializer().getExpr() | ||
or | ||
e = any(ClassAggregateLiteral l).getAFieldExpr(_) | ||
MichaelRFairhurst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
* Find expressions that defer their value directly to an inner expression | ||
* value. | ||
* | ||
* When an array is on the rhs of a comma expr, or in the then/else branch of a | ||
* ternary expr, and the result us used as a pointer, then the ArrayToPointer | ||
* conversion is marked inside comma expr/ternary expr, on the operands. These | ||
* conversions are only non-compliant if they flow into an operation or store. | ||
* | ||
* Full flow analysis with localFlowStep should not be necessary, and may cast a | ||
* wider net than needed for some queries, potentially resulting in false | ||
* positives. | ||
*/ | ||
Expr temporaryObjectFlowStep(Expr e) { | ||
e = result.(CommaExpr).getRightOperand() | ||
or | ||
e = result.(ConditionalExpr).getThen() | ||
or | ||
e = result.(ConditionalExpr).getElse() | ||
} | ||
|
||
from | ||
TemporaryLifetimeArrayAccess fa, TemporaryLifetimeExpr temporary, | ||
ArrayToPointerConversion conversion | ||
where | ||
not isExcluded(conversion, InvalidMemory3Package::arrayToPointerConversionOfTemporaryObjectQuery()) and | ||
fa.getTemporary() = temporary and | ||
conversion.getExpr() = fa and | ||
( | ||
temporaryObjectFlowStep*(conversion.getExpr()) = usedValuesOf(any(Expr e)) | ||
or | ||
isStored(temporaryObjectFlowStep*(conversion.getExpr())) | ||
) | ||
select conversion, "Array to pointer conversion of array $@ from temporary object $@", | ||
MichaelRFairhurst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fa.getTarget(), fa.getTarget().getName(), temporary, temporary.toString() |
59 changes: 59 additions & 0 deletions
59
c/misra/src/rules/RULE-18-9/ModifiableLValueSubscriptedWithTemporaryLifetime.ql
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/** | ||
* @id c/misra/modifiable-l-value-subscripted-with-temporary-lifetime | ||
* @name RULE-18-9: Usage of the subscript operator on an object with temporary lifetime shall not return a modifiable value | ||
* @description Modifying elements of an array with temporary lifetime will result in undefined | ||
* behavior. | ||
* @kind problem | ||
* @precision high | ||
* @problem.severity error | ||
* @tags external/misra/id/rule-18-9 | ||
* external/misra/c/2012/amendment3 | ||
* correctness | ||
* security | ||
* external/misra/obligation/required | ||
*/ | ||
|
||
import cpp | ||
import codingstandards.c.misra | ||
import codingstandards.cpp.lifetimes.CLifetimes | ||
|
||
class TemporaryLifetimeArrayExpr extends ArrayExpr { | ||
TemporaryLifetimeArrayAccess member; | ||
Type elementType; | ||
|
||
TemporaryLifetimeArrayExpr() { | ||
member = getArrayBase() and | ||
elementType = member.getType().(ArrayType).getBaseType() | ||
or | ||
exists(TemporaryLifetimeArrayExpr inner | | ||
inner = getArrayBase() and | ||
member = inner.getMember() and | ||
elementType = inner.getElementType().(ArrayType).getBaseType() | ||
) | ||
} | ||
|
||
TemporaryLifetimeArrayAccess getMember() { result = member } | ||
|
||
Type getElementType() { result = elementType } | ||
} | ||
|
||
predicate usedAsModifiableLvalue(Expr expr) { | ||
exists(Assignment parent | parent.getLValue() = expr) | ||
or | ||
exists(CrementOperation parent | parent.getOperand() = expr) | ||
or | ||
exists(AddressOfExpr parent | parent.getOperand() = expr) | ||
or | ||
exists(FieldAccess parent | parent.getQualifier() = expr and usedAsModifiableLvalue(parent)) | ||
} | ||
|
||
from TemporaryLifetimeArrayExpr expr, TemporaryLifetimeArrayAccess member | ||
where | ||
not isExcluded(expr, | ||
InvalidMemory3Package::modifiableLValueSubscriptedWithTemporaryLifetimeQuery()) and | ||
member = expr.getMember() and | ||
not expr.isUnevaluated() and | ||
usedAsModifiableLvalue(expr) | ||
select expr, | ||
"Modifiable lvalue produced by subscripting array member $@ of temporary lifetime object $@ ", | ||
member, member.getTarget().getName(), member.getTemporary(), member.getTemporary().toString() |
17 changes: 17 additions & 0 deletions
17
c/misra/test/rules/RULE-18-10/PointersToVariablyModifiedArrayTypesUsed.expected
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
| test.c:17:11:17:12 | definition of p5 | Parameter p5 is declared with variably-modified type, pointer to variable length array of non constant size $@ and element type 'int' | test.c:17:15:17:16 | p0 | p0 | | ||
| test.c:18:11:18:12 | definition of p6 | Parameter p6 is declared with variably-modified type, with inner variable length array of non constant size $@ and element type 'int' | test.c:18:18:18:19 | p0 | p0 | | ||
| test.c:19:11:19:12 | definition of p7 | Parameter p7 is declared with variably-modified type, pointer to variable length array of non constant size $@ and element type 'int[2]' | test.c:19:15:19:16 | p0 | p0 | | ||
| test.c:20:11:20:12 | definition of p8 | Parameter p8 is declared with variably-modified type, pointer to variable length array of non constant size $@ and element type 'int[]' | test.c:20:15:20:16 | p0 | p0 | | ||
| test.c:20:11:20:12 | definition of p8 | Parameter p8 is declared with variably-modified type, pointer to variable length array of non constant size $@ and element type 'int[]' | test.c:20:19:20:20 | p0 | p0 | | ||
| test.c:24:12:24:13 | definition of p9 | Parameter p9 is declared with variably-modified type, pointer to variable length array of non constant size $@ and element type 'int *' | test.c:24:16:24:17 | p0 | p0 | | ||
| test.c:25:13:25:15 | definition of p10 | Parameter p10 is declared with variably-modified type, with inner variable length array of non constant size $@ and element type 'int *' | test.c:25:18:25:19 | p0 | p0 | | ||
| test.c:28:12:28:14 | definition of p11 | Parameter p11 is adjusted to variably-modified type, with inner variable length array of non constant size $@ and element type 'int' | test.c:28:21:28:22 | p0 | p0 | | ||
| test.c:32:17:32:19 | definition of p13 | Parameter p13 is declared with variably-modified type, pointer to variable length array of non constant size $@ and element type 'const int' | test.c:32:22:32:23 | p0 | p0 | | ||
| test.c:33:17:33:19 | definition of p14 | Parameter p14 is declared with variably-modified type, with inner variable length array of non constant size $@ and element type 'int' | test.c:33:22:33:23 | p0 | p0 | | ||
| test.c:40:12:40:14 | definition of p17 | Parameter p17 is declared with variably-modified type, with inner variable length array of non constant size $@ and element type 'int' | test.c:40:24:40:25 | p0 | p0 | | ||
| test.c:41:14:41:16 | definition of p18 | Parameter p18 is declared with variably-modified type, with inner variable length array of non constant size $@ and element type 'int' | test.c:41:27:41:28 | p0 | p0 | | ||
| test.c:68:9:68:11 | definition of p27 | Parameter p27 is adjusted to variably-modified type, with inner variable length array of non constant size $@ and element type 'int' | test.c:68:13:68:14 | p0 | p0 | | ||
| test.c:68:9:68:11 | definition of p27 | Parameter p27 is adjusted to variably-modified type, with inner variable length array of non constant size $@ and element type 'int' | test.c:68:17:68:18 | p0 | p0 | | ||
| test.c:74:8:74:9 | definition of l3 | Variable l3 is declared with variably-modified type, pointer to variable length array of non constant size $@ and element type 'int' | test.c:74:12:74:13 | p0 | p0 | | ||
| test.c:79:15:79:16 | definition of l4 | Variable l4 is declared with variably-modified type, pointer to variable length array of non constant size $@ and element type 'int' | test.c:79:19:79:20 | p0 | p0 | | ||
| test.c:84:16:84:18 | declaration of td3 | Declaration td3 is declared with variably-modified type, with inner variable length array of non constant size $@ and element type 'int' | test.c:84:21:84:22 | p0 | p0 | |
1 change: 1 addition & 0 deletions
1
c/misra/test/rules/RULE-18-10/PointersToVariablyModifiedArrayTypesUsed.qlref
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
rules/RULE-18-10/PointersToVariablyModifiedArrayTypesUsed.ql |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider what happens if the declaration is in a macro. Specifically, what happens if a macro has multiple array declarations, only one of which is a VM. I suspect we'll end up flagging all of them, as they will all have the same location.
Note: that is not a problem for the parameter case, because we require a strict before/after location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can exclude that case. In general, it'd be easy to exclude all cases where the declaration entry isGeneratedByMacro.
However, it's worth noting that the false positives are incomplete array types. Variables can't be declared with incomplete array types. So the cases where this would flag multiple things because they all match would be a pretty unusual case where, for instance, multiple declared variables have function types with incomplete array types as parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we detect when this issue had occurred by determining if the number of VlaDeclStmt and the number candidates that occur at the same location match? Then avoid reporting in the case where it doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very reasonable, will do!