diff --git a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql index 8b405d138c..ddb8cbcdcc 100644 --- a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql +++ b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql @@ -18,29 +18,54 @@ import cpp import codingstandards.c.misra import codingstandards.cpp.Pointers import codingstandards.cpp.SideEffect +import codingstandards.cpp.alertreporting.HoldsForAllCopies -from Variable ptr, PointerOrArrayType type +class NonConstPointerVariableCandidate extends Variable { + NonConstPointerVariableCandidate() { + // Ignore parameters in functions without bodies + (this instanceof Parameter implies exists(this.(Parameter).getFunction().getBlock())) and + // Ignore variables in functions that use ASM commands + not exists(AsmStmt a | + a.getEnclosingFunction() = this.(LocalScopeVariable).getFunction() + or + // In a type declared locally + this.(Field).getDeclaringType+().getEnclosingFunction() = a.getEnclosingFunction() + ) and + exists(PointerOrArrayType type | + // include only pointers which point to a const-qualified type + this.getType() = type and + not type.isDeeplyConstBelow() + ) and + // exclude pointers passed as arguments to functions which take a + // parameter that points to a non-const-qualified type + not exists(FunctionCall fc, int i | + fc.getArgument(i) = this.getAnAccess() and + not fc.getTarget().getParameter(i).getType().isDeeplyConstBelow() + ) and + // exclude any pointers which have their underlying data modified + not exists(VariableEffect effect | + effect.getTarget() = this and + // but not pointers that are only themselves modified + not effect.(AssignExpr).getLValue() = this.getAnAccess() and + not effect.(CrementOperation).getOperand() = this.getAnAccess() + ) and + // exclude pointers assigned to another pointer to a non-const-qualified type + not exists(Variable a | + a.getAnAssignedValue() = this.getAnAccess() and + not a.getType().(PointerOrArrayType).isDeeplyConstBelow() + ) + } +} + +/** + * Ensure that all copies of a variable are considered to be missing const qualification to avoid + * false positives where a variable is only used/modified in a single copy. + */ +class NonConstPointerVariable = + HoldsForAllCopies::LogicalResultElement; + +from NonConstPointerVariable ptr where - not isExcluded(ptr, Pointers1Package::pointerShouldPointToConstTypeWhenPossibleQuery()) and - // include only pointers which point to a const-qualified type - ptr.getType() = type and - not type.isDeeplyConstBelow() and - // exclude pointers passed as arguments to functions which take a - // parameter that points to a non-const-qualified type - not exists(FunctionCall fc, int i | - fc.getArgument(i) = ptr.getAnAccess() and - not fc.getTarget().getParameter(i).getType().isDeeplyConstBelow() - ) and - // exclude any pointers which have their underlying data modified - not exists(VariableEffect effect | - effect.getTarget() = ptr and - // but not pointers that are only themselves modified - not effect.(AssignExpr).getLValue() = effect.getAnAccess() and - not effect.(CrementOperation).getOperand() = effect.getAnAccess() - ) and - // exclude pointers assigned to another pointer to a non-const-qualified type - not exists(Variable a | - a.getAnAssignedValue() = ptr.getAnAccess() and - not a.getType().(PointerOrArrayType).isDeeplyConstBelow() - ) -select ptr, "$@ points to a non-const-qualified type.", ptr, ptr.getName() + not isExcluded(ptr.getAnElementInstance(), + Pointers1Package::pointerShouldPointToConstTypeWhenPossibleQuery()) +select ptr, "$@ points to a non-const-qualified type.", ptr, ptr.getAnElementInstance().getName() diff --git a/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected b/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected index 39dbf04763..e3e0963087 100644 --- a/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected +++ b/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected @@ -12,3 +12,4 @@ | test.c:66:23:66:24 | p1 | $@ points to a non-const-qualified type. | test.c:66:23:66:24 | p1 | p1 | | test.c:71:17:71:18 | p1 | $@ points to a non-const-qualified type. | test.c:71:17:71:18 | p1 | p1 | | test.c:75:15:75:16 | p1 | $@ points to a non-const-qualified type. | test.c:75:15:75:16 | p1 | p1 | +| test.c:103:30:103:30 | s | $@ points to a non-const-qualified type. | test.c:103:30:103:30 | s | s | diff --git a/c/misra/test/rules/RULE-8-13/test.c b/c/misra/test/rules/RULE-8-13/test.c index 1ac9e5028c..a2333d2a3d 100644 --- a/c/misra/test/rules/RULE-8-13/test.c +++ b/c/misra/test/rules/RULE-8-13/test.c @@ -75,4 +75,38 @@ char *f16(char *p1) { // NON_COMPLIANT int f17(char *p1) { // NON_COMPLIANT p1++; return 0; +} + +#include + +int16_t +test_r(int16_t *value) { // COMPLIANT - ignored because of the use of ASM + int16_t result; + struct S { + int *x; // COMPLIANT - ignored because of the use of ASM + struct S2 { + int *y; // COMPLIANT - ignored because of the use of ASM + } s2; + }; + __asm__("movb %bh (%eax)"); + return result; +} + +struct S { + int x; +}; + +void test_struct(struct S *s) { // COMPLIANT + s->x = 1; +} + +void test_struct_2(struct S *s) { // NON_COMPLIANT - could be const + s = 0; +} + +void test_no_body(int *p); // COMPLIANT - no body, so cannot evaluate whether it + // should be const + +void increment(int *p) { // COMPLIANT + *p++ = 1; } \ No newline at end of file diff --git a/change_notes/2024-10-20-8-13-fixes.md b/change_notes/2024-10-20-8-13-fixes.md new file mode 100644 index 0000000000..6ee8e3a32c --- /dev/null +++ b/change_notes/2024-10-20-8-13-fixes.md @@ -0,0 +1,7 @@ + - `RULE-8-13` - `PointerShouldPointToConstTypeWhenPossible.ql` + - 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. + - Exclude results for local scope variables in functions that use assembly code, as CodeQL cannot determine the impact of the assembly. + - Exclude false positives when an assignment is made to a struct field. + - Exclude false positives where the object pointed to by the variable is modified using `*p++ = ...`. + - Exclude false positives for functions without bodies. + - 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`. \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/SideEffect.qll b/cpp/common/src/codingstandards/cpp/SideEffect.qll index 08cd9394d3..68fe2cd0cd 100644 --- a/cpp/common/src/codingstandards/cpp/SideEffect.qll +++ b/cpp/common/src/codingstandards/cpp/SideEffect.qll @@ -190,6 +190,8 @@ Expr getAnEffect(Expr base) { or exists(PointerDereferenceExpr e | e.getOperand() = base | result = getAnEffect(e)) or + exists(CrementOperation c | c.getOperand() = base | result = getAnEffect(c)) + or // local alias analysis, assume alias when data flows to derived type (pointer/reference) // auto ptr = &base; exists(VariableAccess va, AddressOfExpr addressOf | diff --git a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll index 634c1bf610..1d47e833dc 100644 --- a/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll +++ b/cpp/common/src/codingstandards/cpp/alertreporting/HoldsForAllCopies.qll @@ -85,11 +85,9 @@ module HoldsForAllCopies