Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 1566129

Browse filesBrowse 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
Copy full SHA for 1566129

File tree

6 files changed

+94
-27
lines changed
Filter options

6 files changed

+94
-27
lines changed

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

Copy file name to clipboardExpand all lines: c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql
+49-24Lines changed: 49 additions & 24 deletions
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

Copy file name to clipboardExpand all lines: c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected
+1Lines changed: 1 addition & 0 deletions
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

Copy file name to clipboardExpand all lines: c/misra/test/rules/RULE-8-13/test.c
+34Lines changed: 34 additions & 0 deletions
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

Copy file name to clipboard
+7Lines changed: 7 additions & 0 deletions
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

Copy file name to clipboardExpand all lines: cpp/common/src/codingstandards/cpp/SideEffect.qll
+2Lines changed: 2 additions & 0 deletions
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

Copy file name to clipboardExpand all lines: cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll
+1-3Lines changed: 1 addition & 3 deletions
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.