-
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
dd475be
5b5777f
01e51e8
ab4a7bf
06ba26d
7014931
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be helpful to provide a comment here on why we're looking for a subsumed dimension expression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I added one. I also realized this class probably belongs in |
||
( | ||
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() |
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 $@", | ||
fa.getTarget(), fa.getTarget().getName(), temporary, temporary.toString() |
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() |
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 | |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
rules/RULE-18-10/PointersToVariablyModifiedArrayTypesUsed.ql |
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.