Skip to content

Implement C Pointers3 package #139

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

Merged
merged 41 commits into from
Feb 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
efb286d
EXP32-C: Add test case
lcartey Nov 7, 2022
ee22665
EXP36-C: Add test case
lcartey Nov 7, 2022
94347c4
EXP43-C: Add test case
lcartey Nov 7, 2022
741ab39
EXP39-C: Add malloc/realloc test
lcartey Nov 7, 2022
8462844
EXP43-C: Add test cases
lcartey Nov 7, 2022
e0e4181
Add Pointers3 package
Nov 16, 2022
1bfa1b1
Fix RULE-11-7 query output message
Nov 16, 2022
833d834
Implement EXP32-C and EXP36-C
Nov 16, 2022
3d337ce
Merge pull request #127 from lcartey/lcartey/pointers3
Nov 16, 2022
d483c3c
Update EXP32-C query and test
Nov 17, 2022
6733786
Add comments to EXP32-C query
Nov 17, 2022
a157089
Add comments and implementation scope for EXP32-C
Nov 17, 2022
03bac50
Update EXP36-C query and test
Nov 17, 2022
0c28310
Update EXP39-C test and implement query
Nov 18, 2022
87d52ae
Update Pointers3.json
Nov 18, 2022
c0f9767
Update DoNotAccessVariableViaPointerOfIncompatibleType.ql
Nov 18, 2022
0431c73
Resolve EXP32-C performance issue
Jan 16, 2023
8633a30
Update DoNotAccessVolatileObjectWithNonVolatileReference.ql
Jan 16, 2023
469e42c
Implement EXP43-C and add help files
Jan 27, 2023
0332744
Merge branch 'main' into pointers3
Jan 27, 2023
72e9fd6
Update RuleMetadata.qll
Jan 27, 2023
0e9df1e
Update test.c
Jan 27, 2023
f2659d6
Merge branch 'pointers3' of https://github.com/github/codeql-coding-s…
Jan 27, 2023
7bb5665
Regenerate Pointers3 package files
Jan 27, 2023
6d25cd6
Update test.c
Jan 27, 2023
23961bc
Correct whitespace in rule help files (regen package)
Jan 27, 2023
e2e6590
Update EXP43-C message and expected output
Jan 27, 2023
5f09408
Revert accidental change to EXP39-C and update .expected results
Jan 27, 2023
68b3f39
EXP43-C: add test-case and update expected results
Jan 28, 2023
934811d
EXP43-C: remove redundant check
Jan 28, 2023
de6cd01
Move `PointerOrArrayType` into `Pointers.qll` to deduplicate usage
Jan 28, 2023
a390e54
Update EXP43-C query and test
Feb 6, 2023
c239197
EXP43-C: swap PointsTo with data-flow and fix typo
Feb 6, 2023
2dd045c
EXP43-C: Revert IR dataflow to AST data-flow
Feb 15, 2023
563e084
EXP43-C: Replace `!=` with `not =`
Feb 15, 2023
3a3a6a5
EXP36-C: Output cast instead of sink node
Feb 15, 2023
009fc65
EXP39-C: Remove redundant "TODO" comment from test-case
Feb 15, 2023
6a554ac
EXP43-C: Refine data-flow and add context to output
Feb 15, 2023
46abe38
Merge branch 'pointers3' of https://github.com/github/codeql-coding-s…
Feb 15, 2023
2f91e33
EXP43-C: Refactor and remove test code
Feb 15, 2023
0b564db
Merge branch 'main' into pointers3
lcartey Feb 15, 2023
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
1 change: 1 addition & 0 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@
"Pointers",
"Pointers1",
"Pointers2",
"Pointers3",
"Scope",
"SideEffects1",
"SideEffects2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,50 +13,10 @@

import cpp
import codingstandards.c.cert
import codingstandards.c.Pointers
import semmle.code.cpp.dataflow.TaintTracking
import DataFlow::PathGraph

/**
* An expression which performs pointer arithmetic
*/
abstract class PointerArithmeticExpr extends Expr {
abstract Expr getPointer();

abstract Expr getOperand();
}

/**
* A pointer arithmetic binary operation expression.
*/
class SimplePointerArithmeticExpr extends PointerArithmeticExpr, PointerArithmeticOperation {
override Expr getPointer() { result = this.getLeftOperand() }

override Expr getOperand() { result = this.getRightOperand() }
}

/**
* A pointer arithmetic assignment expression.
*/
class AssignPointerArithmeticExpr extends PointerArithmeticExpr, AssignOperation {
AssignPointerArithmeticExpr() {
this instanceof AssignPointerAddExpr or
this instanceof AssignPointerSubExpr
}

override Expr getPointer() { result = this.getLValue() }

override Expr getOperand() { result = this.getRValue() }
}

/**
* A pointer arithmetic array access expression.
*/
class ArrayPointerArithmeticExpr extends PointerArithmeticExpr, ArrayExpr {
override Expr getPointer() { result = this.getArrayBase() }

override Expr getOperand() { result = this.getArrayOffset() }
}

/**
* An expression which invokes the `offsetof` macro or `__builtin_offsetof` operation.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# EXP32-C: Do not access a volatile object through a nonvolatile reference

This query implements the CERT-C rule EXP32-C:

> Do not access a volatile object through a nonvolatile reference


## Description

An object that has volatile-qualified type may be modified in ways unknown to the [implementation](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-implementation) or have other unknown [side effects](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-sideeffect). Referencing a volatile object by using a non-volatile lvalue is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior). The C Standard, 6.7.3 \[[ISO/IEC 9899:2011](https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO-IEC9899-2011)\], states

> If an attempt is made to refer to an object defined with a volatile-qualified type through use of an lvalue with non-volatile-qualified type, the behavior is undefined.


See [undefined behavior 65](https://wiki.sei.cmu.edu/confluence/display/c/CC.+Undefined+Behavior#CC.UndefinedBehavior-ub_65).

## Noncompliant Code Example

In this noncompliant code example, a volatile object is accessed through a non-volatile-qualified reference, resulting in [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior):

```cpp
#include <stdio.h>

void func(void) {
static volatile int **ipp;
static int *ip;
static volatile int i = 0;

printf("i = %d.\n", i);

ipp = &ip; /* May produce a warning diagnostic */
ipp = (int**) &ip; /* Constraint violation; may produce a warning diagnostic */
*ipp = &i; /* Valid */
if (*ip != 0) { /* Valid */
/* ... */
}
}
```
The assignment `ipp = &ip` is not safe because it allows the valid code that follows to reference the value of the volatile object `i` through the non-volatile-qualified reference `ip`. In this example, the compiler may optimize out the entire `if` block because `*ip != 0` must be false if the object to which `ip` points is not volatile.

**Implementation Details**

This example compiles without warning on Microsoft Visual Studio 2013 when compiled in C mode (`/TC`) but causes errors when compiled in C++ mode (`/TP`).

GCC 4.8.1 generates a warning but compiles successfully.

## Compliant Solution

In this compliant solution, `ip` is declared `volatile`:

```cpp
#include <stdio.h>

void func(void) {
static volatile int **ipp;
static volatile int *ip;
static volatile int i = 0;

printf("i = %d.\n", i);

ipp = &ip;
*ipp = &i;
if (*ip != 0) {
/* ... */
}

}
```

## Risk Assessment

Accessing an object with a volatile-qualified type through a reference with a non-volatile-qualified type is [undefined behavior](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-undefinedbehavior).

<table> <tbody> <tr> <th> Rule </th> <th> Severity </th> <th> Likelihood </th> <th> Remediation Cost </th> <th> Priority </th> <th> Level </th> </tr> <tr> <td> EXP32-C </td> <td> Low </td> <td> Likely </td> <td> Medium </td> <td> <strong>P6</strong> </td> <td> <strong>L2</strong> </td> </tr> </tbody> </table>


## Automated Detection

<table> <tbody> <tr> <th> Tool </th> <th> Version </th> <th> Checker </th> <th> Description </th> </tr> <tr> <td> <a> Astrée </a> </td> <td> 22.04 </td> <td> <strong>pointer-qualifier-cast-volatile</strong> <strong>pointer-qualifier-cast-volatile-implicit</strong> </td> <td> Supported indirectly via MISRA C 2012 Rule 11.8 </td> </tr> <tr> <td> <a> Axivion Bauhaus Suite </a> </td> <td> 7.2.0 </td> <td> <strong>CertC-EXP32</strong> </td> <td> Fully implemented </td> </tr> <tr> <td> <a> Clang </a> </td> <td> 3.9 </td> <td> <code>-Wincompatible-pointer-types-discards-qualifiers</code> </td> <td> </td> </tr> <tr> <td> <a> Compass/ROSE </a> </td> <td> </td> <td> </td> <td> </td> </tr> <tr> <td> <a> Coverity </a> </td> <td> 2017.07 </td> <td> <strong>MISRA C 2012 Rule 11.8</strong> </td> <td> Implemented </td> </tr> <tr> <td> <a> GCC </a> </td> <td> 4.3.5 </td> <td> </td> <td> Can detect violations of this rule when the <code>-Wcast-qual</code> flag is used </td> </tr> <tr> <td> <a> Helix QAC </a> </td> <td> 2022.4 </td> <td> <strong>C0312, C0562, C0563, C0673, C0674</strong> </td> <td> </td> </tr> <tr> <td> <a> Klocwork </a> </td> <td> 2022.4 </td> <td> <strong>CERT.EXPR.VOLATILE.ADDR</strong> <strong>CERT.EXPR.VOLATILE.ADDR.PARAM</strong> <strong>CERT.EXPR.VOLATILE.PTRPTR</strong> </td> <td> </td> </tr> <tr> <td> <a> LDRA tool suite </a> </td> <td> 9.7.1 </td> <td> <strong>344 S</strong> </td> <td> Partially implemented </td> </tr> <tr> <td> <a> Parasoft C/C++test </a> </td> <td> 2022.2 </td> <td> <strong>CERT_C-EXP32-a</strong> </td> <td> A cast shall not remove any 'const' or 'volatile' qualification from the type of a pointer or reference </td> </tr> <tr> <td> <a> Polyspace Bug Finder </a> </td> <td> </td> <td> <a> CERT C: Rule EXP32-C </a> </td> <td> Checks for cast to pointer that removes const or volatile qualification (rule fully covered) </td> </tr> <tr> <td> <a> PRQA QA-C </a> </td> <td> 9.7 </td> <td> <strong>0312,562,563,673,674</strong> </td> <td> Fully implemented </td> </tr> <tr> <td> <a> RuleChecker </a> </td> <td> 22.04 </td> <td> <strong>pointer-qualifier-cast-volatile</strong> <strong>pointer-qualifier-cast-volatile-implicit</strong> </td> <td> Supported indirectly via MISRA C 2012 Rule 11.8 </td> </tr> </tbody> </table>


## Related Vulnerabilities

Search for [vulnerabilities](https://wiki.sei.cmu.edu/confluence/display/c/BB.+Definitions#BB.Definitions-vulnerability) resulting from the violation of this rule on the [CERT website](https://www.kb.cert.org/vulnotes/bymetric?searchview&query=FIELD+KEYWORDS+contains+EXP32-C).

## Related Guidelines

[Key here](https://wiki.sei.cmu.edu/confluence/display/c/How+this+Coding+Standard+is+Organized#HowthisCodingStandardisOrganized-RelatedGuidelines) (explains table format and definitions)

<table> <tbody> <tr> <th> Taxonomy </th> <th> Taxonomy item </th> <th> Relationship </th> </tr> <tr> <td> <a> ISO/IEC TR 24772:2013 </a> </td> <td> Pointer Casting and Pointer Type Changes \[HFC\] </td> <td> Prior to 2018-01-12: CERT: Unspecified Relationship </td> </tr> <tr> <td> <a> ISO/IEC TR 24772:2013 </a> </td> <td> Type System \[IHN\] </td> <td> Prior to 2018-01-12: CERT: Unspecified Relationship </td> </tr> <tr> <td> <a> MISRA C:2012 </a> </td> <td> Rule 11.8 (required) </td> <td> Prior to 2018-01-12: CERT: Unspecified Relationship </td> </tr> <tr> <td> <a> CERT C </a> </td> <td> <a> EXP55-CPP. Do not access a cv-qualified object through a cv-unqualified type </a> </td> <td> Prior to 2018-01-12: CERT: Unspecified Relationship </td> </tr> </tbody> </table>


## Bibliography

<table> <tbody> <tr> <td> \[ <a> ISO/IEC 9899:2011 </a> \] </td> <td> 6.7.3, "Type Qualifiers" </td> </tr> </tbody> </table>


## Implementation notes

In limited cases, this query can raise false-positives for assignment of volatile objects and subsequent accesses of those objects via non-volatile pointers.

## References

* CERT-C: [EXP32-C: Do not access a volatile object through a nonvolatile reference](https://wiki.sei.cmu.edu/confluence/display/c)
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/**
* @id c/cert/do-not-access-volatile-object-with-non-volatile-reference
* @name EXP32-C: Do not access a volatile object through a nonvolatile reference
* @description If an an object defined with a volatile-qualified type is referred to with an lvalue
* of a non-volatile-qualified type, the behavior is undefined.
* @kind problem
* @precision high
* @problem.severity error
* @tags external/cert/id/exp32-c
* correctness
* external/cert/obligation/rule
*/

import cpp
import codingstandards.c.cert
import semmle.code.cpp.controlflow.Dereferenced

/**
* An expression involving volatile-qualified types that results in undefined behavior.
*/
abstract class UndefinedVolatilePointerExpr extends Expr {
/**
* Gets a descriptive string describing the type of expression and undefined behavior.
*/
abstract string getMessage();
}

/**
* Gets the depth of a pointer's base type's volatile qualifier
*/
int getAVolatileDepth(Type type) {
type.isVolatile() and result = 1
or
result = getAVolatileDepth(type.(DerivedType).getBaseType()) + 1
}

/**
* A `Cast` which converts from a pointer to a volatile-qualified type
* to a pointer to a non-volatile-qualified type.
*/
class CastFromVolatileToNonVolatileBaseType extends Cast, UndefinedVolatilePointerExpr {
CastFromVolatileToNonVolatileBaseType() {
exists(int i |
i = getAVolatileDepth(this.getExpr().getType()) and
not i = getAVolatileDepth(this.getActualType())
)
}

override string getMessage() {
result = "Cast of object with a volatile-qualified type to a non-volatile-qualified type."
}
}

/**
* Holds if `va` has a subsequent `VariableAccess` which is dereferenced after access
*/
bindingset[va]
predicate hasSubsequentDereference(VariableAccess va) {
dereferenced(pragma[only_bind_out](va).getASuccessor+())
}

/**
* An `AssignExpr` with an *lvalue* that is a pointer to a volatile base type and
* and *rvalue* that is not also a pointer to a volatile base type.
*/
class NonVolatileObjectAssignedToVolatilePointer extends AssignExpr, UndefinedVolatilePointerExpr {
NonVolatileObjectAssignedToVolatilePointer() {
exists(int i |
not i = getAVolatileDepth(this.getRValue().getType()) and
i = getAVolatileDepth(this.getLValue().(VariableAccess).getTarget().getType())
) and
// Checks for subsequent accesses to the underlying object via the original non-volatile
// pointer assigned to the volatile pointer. This heuristic can cause false-positives
// in certain instances which require more advanced reachability analysis, e.g. loops and scope
// considerations that this simple forward traversal of the control-flow graph does not account for.
exists(VariableAccess va |
va = this.getRValue().getAChild*().(VariableAccess).getTarget().getAnAccess() and
hasSubsequentDereference(va)
)
}

override string getMessage() {
result =
"Assignment indicates a volatile object, but a later access of the object occurs via a non-volatile pointer."
}
}

from UndefinedVolatilePointerExpr e
where not isExcluded(e, Pointers3Package::doNotAccessVolatileObjectWithNonVolatileReferenceQuery())
select e, e.getMessage()
Loading