Skip to content

Commit e655ed8

Browse files
MichaelRFairhurstMichaelRFairhurst
committed
Exclude if (f) f() from EXP-16-C
1 parent e4121ee commit e655ed8

File tree

5 files changed

+158
-42
lines changed

5 files changed

+158
-42
lines changed

‎c/cert/src/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.ql

Lines changed: 5 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -17,33 +17,11 @@
1717
*/
1818

1919
import cpp
20+
import semmle.code.cpp.controlflow.IRGuards
2021
import codingstandards.c.cert
2122
import codingstandards.cpp.types.FunctionType
22-
import semmle.code.cpp.controlflow.IRGuards
23-
24-
class FunctionExpr extends Expr {
25-
Element function;
26-
string funcName;
27-
28-
FunctionExpr() {
29-
function = this.(FunctionAccess).getTarget() and
30-
funcName = "Function " + function.(Function).getName()
31-
or
32-
this.(VariableAccess).getUnderlyingType() instanceof FunctionType and
33-
function = this and
34-
funcName = "Function pointer variable " + this.(VariableAccess).getTarget().getName()
35-
or
36-
this.getUnderlyingType() instanceof FunctionType and
37-
not this instanceof FunctionAccess and
38-
not this instanceof VariableAccess and
39-
function = this and
40-
funcName = "Expression with function pointer type"
41-
}
42-
43-
Element getFunction() { result = function }
44-
45-
string getFuncName() { result = funcName }
46-
}
23+
import codingstandards.cpp.exprs.FunctionExprs
24+
import codingstandards.cpp.exprs.Guards
4725

4826
abstract class EffectivelyComparison extends Element {
4927
abstract string getExplanation();
@@ -85,6 +63,7 @@ where
8563
not isExcluded(comparison,
8664
Expressions2Package::doNotCompareFunctionPointersToConstantValuesQuery()) and
8765
funcExpr = comparison.getFunctionExpr() and
66+
not exists(NullFunctionCallGuard nullGuard | nullGuard.getFunctionExpr() = funcExpr) and
8867
function = funcExpr.getFunction() and
89-
funcName = funcExpr.getFuncName()
68+
funcName = funcExpr.describe()
9069
select comparison, comparison.getExplanation(), function, funcName

‎c/cert/test/rules/EXP16-C/DoNotCompareFunctionPointersToConstantValues.expected

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,20 @@
1-
| test.c:17:7:17:13 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
2-
| test.c:20:7:20:12 | ... > ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
3-
| test.c:29:7:29:13 | ... == ... | $@ compared to constant value. | test.c:29:7:29:8 | g1 | Function pointer variable g1 |
4-
| test.c:32:7:32:16 | ... == ... | $@ compared to constant value. | test.c:32:7:32:8 | g2 | Function pointer variable g2 |
5-
| test.c:35:7:35:15 | ... != ... | $@ compared to constant value. | test.c:35:7:35:8 | g1 | Function pointer variable g1 |
6-
| test.c:38:7:38:8 | f1 | $@ undergoes implicit constant comparison. | test.c:3:5:3:6 | f1 | Function f1 |
7-
| test.c:41:7:41:8 | g1 | $@ undergoes implicit constant comparison. | test.c:41:7:41:8 | g1 | Function pointer variable g1 |
8-
| test.c:68:7:68:27 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
9-
| test.c:71:7:71:18 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
10-
| test.c:74:7:76:14 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Function f1 |
11-
| test.c:83:3:83:9 | ... == ... | $@ compared to constant value. | test.c:83:3:83:4 | l1 | Function pointer variable l1 |
12-
| test.c:84:3:84:12 | ... == ... | $@ compared to constant value. | test.c:84:3:84:4 | l1 | Function pointer variable l1 |
13-
| test.c:91:3:91:4 | g1 | $@ undergoes implicit constant comparison. | test.c:91:3:91:4 | g1 | Function pointer variable g1 |
14-
| test.c:96:7:96:18 | ... == ... | $@ compared to constant value. | test.c:96:9:96:10 | fp | Function pointer variable fp |
15-
| test.c:102:7:102:22 | ... == ... | $@ compared to constant value. | test.c:14:11:14:21 | get_handler | Function get_handler |
1+
| test.c:17:7:17:13 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Address of function f1 |
2+
| test.c:20:7:20:12 | ... > ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Address of function f1 |
3+
| test.c:29:7:29:13 | ... == ... | $@ compared to constant value. | test.c:4:8:4:9 | g1 | Function pointer variable g1 |
4+
| test.c:32:7:32:16 | ... == ... | $@ compared to constant value. | test.c:5:7:5:8 | g2 | Function pointer variable g2 |
5+
| test.c:35:7:35:15 | ... != ... | $@ compared to constant value. | test.c:4:8:4:9 | g1 | Function pointer variable g1 |
6+
| test.c:38:7:38:8 | f1 | $@ undergoes implicit constant comparison. | test.c:3:5:3:6 | f1 | Address of function f1 |
7+
| test.c:41:7:41:8 | g1 | $@ undergoes implicit constant comparison. | test.c:4:8:4:9 | g1 | Function pointer variable g1 |
8+
| test.c:68:7:68:27 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Address of function f1 |
9+
| test.c:71:7:71:18 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Address of function f1 |
10+
| test.c:74:7:76:14 | ... == ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Address of function f1 |
11+
| test.c:83:3:83:9 | ... == ... | $@ compared to constant value. | test.c:82:10:82:11 | l1 | Function pointer variable l1 |
12+
| test.c:84:3:84:12 | ... == ... | $@ compared to constant value. | test.c:82:10:82:11 | l1 | Function pointer variable l1 |
13+
| test.c:91:3:91:4 | g1 | $@ undergoes implicit constant comparison. | test.c:4:8:4:9 | g1 | Function pointer variable g1 |
14+
| test.c:96:7:96:18 | ... == ... | $@ compared to constant value. | test.c:9:9:9:10 | fp | Function pointer variable fp |
15+
| test.c:102:7:102:22 | ... == ... | $@ compared to constant value. | test.c:14:11:14:21 | get_handler | Address of function get_handler |
1616
| test.c:105:7:105:24 | ... == ... | $@ compared to constant value. | test.c:105:7:105:17 | call to get_handler | Expression with function pointer type |
17+
| test.c:121:7:121:13 | ... != ... | $@ compared to constant value. | test.c:3:5:3:6 | f1 | Address of function f1 |
18+
| test.c:133:7:133:13 | ... != ... | $@ compared to constant value. | test.c:4:8:4:9 | g1 | Function pointer variable g1 |
19+
| test.c:139:7:139:13 | ... == ... | $@ compared to constant value. | test.c:4:8:4:9 | g1 | Function pointer variable g1 |
20+
| test.c:149:8:149:9 | g1 | $@ undergoes implicit constant comparison. | test.c:4:8:4:9 | g1 | Function pointer variable g1 |

‎c/cert/test/rules/EXP16-C/test.c

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ void f6(void) {
9494
void f7(void) {
9595
struct S s;
9696
if (s.fp == NULL) // NON-COMPLIANT
97-
return;
97+
f1();
9898

9999
if (s.fp() == NULL) // COMPLIANT
100100
return;
@@ -110,4 +110,44 @@ void f7(void) {
110110

111111
if (get_handler()() == 0) // COMPLIANT
112112
return;
113+
}
114+
115+
void f8(void) {
116+
// Test instances of where the function pointer check is used to guard calls
117+
// to that function.
118+
119+
// Technically, this function may perhaps be set to NULL by the linker. But
120+
// it is not a variable that should need to be null-checked at runtime.
121+
if (f1 != 0) // NON-COMPLIANT
122+
{
123+
f1();
124+
}
125+
126+
// Check guards a call, so it is compliant.
127+
if (g1 != 0) // COMPLIANT
128+
{
129+
g1();
130+
}
131+
132+
// Incorrect check, not compliant.
133+
if (g1 != 0) // NON-COMPLIANT
134+
{
135+
f1();
136+
}
137+
138+
// Incorrect check, not compliant.
139+
if (g1 == 0) // NON-COMPLIANT
140+
{
141+
g1();
142+
}
143+
144+
if (g1) // COMPLIANT
145+
{
146+
g1();
147+
}
148+
149+
if (!g1) // NON-COMPLIANT
150+
{
151+
g1();
152+
}
113153
}

‎cpp/common/src/codingstandards/cpp/exprs/FunctionExprs.qll

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
import cpp
2+
import codingstandards.cpp.types.FunctionType
3+
4+
/**
5+
* A class representing an expression that has a function pointer type. This can be a function
6+
* access, a variable access, or any expression that has a function pointer type.
7+
*/
8+
abstract class FunctionExpr extends Expr {
9+
/** Any element that could represent the function, for example, a variable or an expression. */
10+
abstract Element getFunction();
11+
12+
/** A name or string that describes the function. */
13+
abstract string describe();
14+
15+
/** Get calls of this function */
16+
abstract Call getACall();
17+
}
18+
19+
/**
20+
* A function access is an an expression of function type where we know the function.
21+
*/
22+
class SimpleFunctionAccess extends FunctionExpr, FunctionAccess {
23+
override Element getFunction() { result = this.getTarget() }
24+
25+
override string describe() { result = "Address of function " + this.getTarget().getName() }
26+
27+
override FunctionCall getACall() { result.getTarget() = this.getTarget() }
28+
}
29+
30+
/**
31+
* An access of a variable that has a function pointer type is also a function expression, for which
32+
* we can track certain properties of the function.
33+
*/
34+
class FunctionVariableAccess extends FunctionExpr, VariableAccess {
35+
FunctionVariableAccess() { this.getUnderlyingType() instanceof FunctionType }
36+
37+
override Element getFunction() { result = this.getTarget() }
38+
39+
override string describe() { result = "Function pointer variable " + this.getTarget().getName() }
40+
41+
override ExprCall getACall() { result.getExpr().(VariableAccess).getTarget() = this.getTarget() }
42+
}
43+
44+
/**
45+
* A function typed expression that is not a function access or a variable access.
46+
*/
47+
class FunctionTypedExpr extends FunctionExpr {
48+
FunctionTypedExpr() {
49+
this.getUnderlyingType() instanceof FunctionType and
50+
not this instanceof FunctionAccess and
51+
not this instanceof VariableAccess
52+
}
53+
54+
override Element getFunction() { result = this }
55+
56+
override string describe() { result = "Expression with function pointer type" }
57+
58+
override ExprCall getACall() { result.getExpr() = this }
59+
}

‎cpp/common/src/codingstandards/cpp/exprs/Guards.qll

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import cpp
2+
import codeql.util.Boolean
3+
import semmle.code.cpp.controlflow.Guards
4+
import codingstandards.cpp.exprs.FunctionExprs
5+
6+
/**
7+
* A guard of the form: `if (funcPtr) funcPtr();`, e.g., a null check on a function before calling
8+
* that function.
9+
*
10+
* Note this does not consider the above to be a null function call guard if `funcPtr` is a
11+
* function name, as that could only be null via unusual linkage steps, and is not expected to be
12+
* an intentional null check.
13+
*/
14+
class NullFunctionCallGuard extends GuardCondition {
15+
FunctionExpr expr;
16+
17+
NullFunctionCallGuard() {
18+
exists(BasicBlock block, Call guardedCall |
19+
(
20+
// Handles 'if (funcPtr != NULL)`:
21+
this.ensuresEq(expr, 0, block, false)
22+
or
23+
// Handles `if (funcPtr)` in C where no implicit conversion to bool exists:
24+
expr = this and
25+
expr.getFunction() instanceof Variable and
26+
this.controls(block, true)
27+
) and
28+
guardedCall = expr.getACall() and
29+
block.contains(guardedCall)
30+
)
31+
}
32+
33+
FunctionExpr getFunctionExpr() { result = expr }
34+
}

0 commit comments

Comments
 (0)