Skip to content

Commit 1566129

Browse files
nicolaswillnicolaswill
authored
Merge pull request #765 from github/lcartey/rule-8-13-copies
`RULE-8-13`: Address false positive issues
2 parents 5d247be + 460fb26 commit 1566129

File tree

6 files changed

+94
-27
lines changed

6 files changed

+94
-27
lines changed

‎c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql

+49-24
Original file line numberDiff line numberDiff line change
@@ -18,29 +18,54 @@ import cpp
1818
import codingstandards.c.misra
1919
import codingstandards.cpp.Pointers
2020
import codingstandards.cpp.SideEffect
21+
import codingstandards.cpp.alertreporting.HoldsForAllCopies
2122

22-
from Variable ptr, PointerOrArrayType type
23+
class NonConstPointerVariableCandidate extends Variable {
24+
NonConstPointerVariableCandidate() {
25+
// Ignore parameters in functions without bodies
26+
(this instanceof Parameter implies exists(this.(Parameter).getFunction().getBlock())) and
27+
// Ignore variables in functions that use ASM commands
28+
not exists(AsmStmt a |
29+
a.getEnclosingFunction() = this.(LocalScopeVariable).getFunction()
30+
or
31+
// In a type declared locally
32+
this.(Field).getDeclaringType+().getEnclosingFunction() = a.getEnclosingFunction()
33+
) and
34+
exists(PointerOrArrayType type |
35+
// include only pointers which point to a const-qualified type
36+
this.getType() = type and
37+
not type.isDeeplyConstBelow()
38+
) and
39+
// exclude pointers passed as arguments to functions which take a
40+
// parameter that points to a non-const-qualified type
41+
not exists(FunctionCall fc, int i |
42+
fc.getArgument(i) = this.getAnAccess() and
43+
not fc.getTarget().getParameter(i).getType().isDeeplyConstBelow()
44+
) and
45+
// exclude any pointers which have their underlying data modified
46+
not exists(VariableEffect effect |
47+
effect.getTarget() = this and
48+
// but not pointers that are only themselves modified
49+
not effect.(AssignExpr).getLValue() = this.getAnAccess() and
50+
not effect.(CrementOperation).getOperand() = this.getAnAccess()
51+
) and
52+
// exclude pointers assigned to another pointer to a non-const-qualified type
53+
not exists(Variable a |
54+
a.getAnAssignedValue() = this.getAnAccess() and
55+
not a.getType().(PointerOrArrayType).isDeeplyConstBelow()
56+
)
57+
}
58+
}
59+
60+
/**
61+
* Ensure that all copies of a variable are considered to be missing const qualification to avoid
62+
* false positives where a variable is only used/modified in a single copy.
63+
*/
64+
class NonConstPointerVariable =
65+
HoldsForAllCopies<NonConstPointerVariableCandidate, Variable>::LogicalResultElement;
66+
67+
from NonConstPointerVariable ptr
2368
where
24-
not isExcluded(ptr, Pointers1Package::pointerShouldPointToConstTypeWhenPossibleQuery()) and
25-
// include only pointers which point to a const-qualified type
26-
ptr.getType() = type and
27-
not type.isDeeplyConstBelow() and
28-
// exclude pointers passed as arguments to functions which take a
29-
// parameter that points to a non-const-qualified type
30-
not exists(FunctionCall fc, int i |
31-
fc.getArgument(i) = ptr.getAnAccess() and
32-
not fc.getTarget().getParameter(i).getType().isDeeplyConstBelow()
33-
) and
34-
// exclude any pointers which have their underlying data modified
35-
not exists(VariableEffect effect |
36-
effect.getTarget() = ptr and
37-
// but not pointers that are only themselves modified
38-
not effect.(AssignExpr).getLValue() = effect.getAnAccess() and
39-
not effect.(CrementOperation).getOperand() = effect.getAnAccess()
40-
) and
41-
// exclude pointers assigned to another pointer to a non-const-qualified type
42-
not exists(Variable a |
43-
a.getAnAssignedValue() = ptr.getAnAccess() and
44-
not a.getType().(PointerOrArrayType).isDeeplyConstBelow()
45-
)
46-
select ptr, "$@ points to a non-const-qualified type.", ptr, ptr.getName()
69+
not isExcluded(ptr.getAnElementInstance(),
70+
Pointers1Package::pointerShouldPointToConstTypeWhenPossibleQuery())
71+
select ptr, "$@ points to a non-const-qualified type.", ptr, ptr.getAnElementInstance().getName()

‎c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected

+1
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,4 @@
1212
| test.c:66:23:66:24 | p1 | $@ points to a non-const-qualified type. | test.c:66:23:66:24 | p1 | p1 |
1313
| test.c:71:17:71:18 | p1 | $@ points to a non-const-qualified type. | test.c:71:17:71:18 | p1 | p1 |
1414
| test.c:75:15:75:16 | p1 | $@ points to a non-const-qualified type. | test.c:75:15:75:16 | p1 | p1 |
15+
| test.c:103:30:103:30 | s | $@ points to a non-const-qualified type. | test.c:103:30:103:30 | s | s |

‎c/misra/test/rules/RULE-8-13/test.c

+34
Original file line numberDiff line numberDiff line change
@@ -75,4 +75,38 @@ char *f16(char *p1) { // NON_COMPLIANT
7575
int f17(char *p1) { // NON_COMPLIANT
7676
p1++;
7777
return 0;
78+
}
79+
80+
#include <stdint.h>
81+
82+
int16_t
83+
test_r(int16_t *value) { // COMPLIANT - ignored because of the use of ASM
84+
int16_t result;
85+
struct S {
86+
int *x; // COMPLIANT - ignored because of the use of ASM
87+
struct S2 {
88+
int *y; // COMPLIANT - ignored because of the use of ASM
89+
} s2;
90+
};
91+
__asm__("movb %bh (%eax)");
92+
return result;
93+
}
94+
95+
struct S {
96+
int x;
97+
};
98+
99+
void test_struct(struct S *s) { // COMPLIANT
100+
s->x = 1;
101+
}
102+
103+
void test_struct_2(struct S *s) { // NON_COMPLIANT - could be const
104+
s = 0;
105+
}
106+
107+
void test_no_body(int *p); // COMPLIANT - no body, so cannot evaluate whether it
108+
// should be const
109+
110+
void increment(int *p) { // COMPLIANT
111+
*p++ = 1;
78112
}

‎change_notes/2024-10-20-8-13-fixes.md

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
- `RULE-8-13` - `PointerShouldPointToConstTypeWhenPossible.ql`
2+
- Exclude false positives where a variable occurs in a file compiled multiple times, but where it may only be const in some of those scenarios.
3+
- Exclude results for local scope variables in functions that use assembly code, as CodeQL cannot determine the impact of the assembly.
4+
- Exclude false positives when an assignment is made to a struct field.
5+
- Exclude false positives where the object pointed to by the variable is modified using `*p++ = ...`.
6+
- Exclude false positives for functions without bodies.
7+
- Rules that rely on the determination of side-effects of an expression may change as a result of considering `*p++ = ...` as having a side-effect on `p`.

‎cpp/common/src/codingstandards/cpp/SideEffect.qll

+2
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,8 @@ Expr getAnEffect(Expr base) {
190190
or
191191
exists(PointerDereferenceExpr e | e.getOperand() = base | result = getAnEffect(e))
192192
or
193+
exists(CrementOperation c | c.getOperand() = base | result = getAnEffect(c))
194+
or
193195
// local alias analysis, assume alias when data flows to derived type (pointer/reference)
194196
// auto ptr = &base;
195197
exists(VariableAccess va, AddressOfExpr addressOf |

‎cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll

+1-3
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,9 @@ module HoldsForAllCopies<CandidateElementSig CandidateElement, ElementSetSig Ele
8585
string filepath, int startline, int startcolumn, int endline, int endcolumn
8686
) {
8787
exists(CandidateElement s |
88-
// Only consider candidates where we can match up the location
89-
isNotWithinMacroExpansion(s) and
9088
hasLocation(s, filepath, startline, startcolumn, endline, endcolumn) and
9189
// All relevant elements that occur at the same location are candidates
92-
forex(RelevantElement relevantElement | s = relevantElement.getCandidateElement() |
90+
forall(RelevantElement relevantElement | s = relevantElement.getCandidateElement() |
9391
relevantElement instanceof CandidateElement
9492
)
9593
)

0 commit comments

Comments
 (0)