Skip to content
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
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion amendments.csv
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ c,MISRA-C-2012,Amendment4,RULE-11-3,Yes,Expand,No,Easy
c,MISRA-C-2012,Amendment4,RULE-11-8,Yes,Expand,No,Easy
c,MISRA-C-2012,Amendment4,RULE-13-2,Yes,Expand,No,Very Hard
c,MISRA-C-2012,Amendment4,RULE-18-6,Yes,Expand,No,Medium
c,MISRA-C-2012,Amendment4,RULE-18-8,Yes,Split,No,Easy
c,MISRA-C-2012,Amendment4,RULE-18-8,Yes,Split,Yes,Easy
c,MISRA-C-2012,Corrigendum2,RULE-2-2,Yes,Clarification,No,Import
c,MISRA-C-2012,Corrigendum2,RULE-2-7,Yes,Clarification,No,Import
c,MISRA-C-2012,Corrigendum2,RULE-3-1,Yes,Refine,No,Easy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,7 @@

import cpp
import codingstandards.c.cert

/**
* A struct or union type that contains an array type
*/
class StructOrUnionTypeWithArrayField extends Struct {
StructOrUnionTypeWithArrayField() {
this.getAField().getUnspecifiedType() instanceof ArrayType
or
// nested struct or union containing an array type
this.getAField().getUnspecifiedType().(Struct) instanceof StructOrUnionTypeWithArrayField
}
}
import codingstandards.cpp.lifetimes.CLifetimes

// Note: Undefined behavior is possible regardless of whether the accessed field from the returned
// struct is an array or a scalar (i.e. arithmetic and pointer types) member, according to the standard.
Expand Down
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)
Copy link
Collaborator

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.

)
)
}

/**
* 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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

(
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()
39 changes: 10 additions & 29 deletions c/misra/src/rules/RULE-18-8/VariableLengthArrayTypesUsed.ql
Original file line number Diff line number Diff line change
Expand Up @@ -15,34 +15,15 @@
import cpp
import codingstandards.c.misra

/**
* A variable length array (VLA)
* ie an array where the size
* is not an integer constant expression
*/
class VariableLengthArray extends VariableDeclarationEntry {
VariableLengthArray() {
//VLAs will not have: static/extern specifiers (compilation error)
not this.hasSpecifier("static") and
not this.hasSpecifier("extern") and
//VLAs are not allowed to be initialized
not this.getDeclaration().hasInitializer() and
exists(ArrayType a |
//a.hasArraySize() does not catch multidimensional VLAs like a[1][]
a.toString().matches("%[]%") and
this.getUnspecifiedType() = a and
//variable length array is one declared in block or function prototype
(
this.getDeclaration().getParentScope() instanceof Function or
this.getDeclaration().getParentScope() instanceof BlockStmt
)
)
}
}

from VariableLengthArray v
from VlaDeclStmt v, Expr size, ArrayType arrayType, string typeStr
where
not isExcluded(v, Declarations7Package::variableLengthArrayTypesUsedQuery()) and
//an exception, argv in : int main(int argc, char *argv[])
not v.getDeclaration().getParentScope().(Function).hasName("main")
select v, "Variable length array declared."
size = v.getVlaDimensionStmt(0).getDimensionExpr() and
(
arrayType = v.getVariable().getType()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to strip top-level specifiers here as well?

or
arrayType = v.getType().getUnspecifiedType()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than getUnspecifiedType(), I think it would be better to use stripTopLevelSpecifiers. getUnspecifiedType resolves typedefs, which can be confusing if the base type of the array is a complex type in itself.

) and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment to the two cases above, noting that the getVariable() holds if this is a direct declaration of a variable, and the .getType() case holds is this is a typedef (but not vice versa). I wasn't sure when I first read this why we had both cases.

typeStr = arrayType.getBaseType().toString()
select v, "Variable length array of element type '" + typeStr + "' with non-constant size $@.",
size, size.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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter is unused. I'd suggest using the same model as isStored(..), and instead do:

predicate isUsed(Expr e) {

I would also integrate the isStored predicate here.

result = expr.(BinaryOperation).getLeftOperand()
or
result = expr.(BinaryOperation).getRightOperand()
or
result = expr.(UnaryOperation).getOperand()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three above can be replaced with Operation.getAnOperand()

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(_)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integrate this predicate into the usedValuesOf predicate.

}

/**
* 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
Loading
Loading