From cb0785575cf7fad87a01d045d37a25cc98c56794 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Sun, 20 Oct 2024 22:50:47 +0100 Subject: [PATCH 01/10] Rule 8.13: Handle multiple copies If the same code is compiled multiple times, variables may be const in one case but non-const in another. Handle this by requiring all such copies to be const-able before flagging. --- ...interShouldPointToConstTypeWhenPossible.ql | 66 ++++++++++++------- 1 file changed, 42 insertions(+), 24 deletions(-) diff --git a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql index 8b405d138c..a9fd7155a4 100644 --- a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql +++ b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql @@ -18,29 +18,47 @@ 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() { + // Avoid elements in macro expansions, as they cannot be equated across copies + not this.isInMacroExpansion() 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() = 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() = 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() From 4aacef197f7ecb0fb149151031997ff2fe3422c5 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Sun, 20 Oct 2024 23:03:19 +0100 Subject: [PATCH 02/10] Rule 8.13: Exclude results in ASM functions --- .../PointerShouldPointToConstTypeWhenPossible.ql | 2 ++ c/misra/test/rules/RULE-8-13/test.c | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql index a9fd7155a4..312662fe30 100644 --- a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql +++ b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql @@ -22,6 +22,8 @@ import codingstandards.cpp.alertreporting.HoldsForAllCopies class NonConstPointerVariableCandidate extends Variable { NonConstPointerVariableCandidate() { + // Ignore variables in functions that use ASM commands + not exists(AsmStmt a | a.getEnclosingFunction() = this.(LocalScopeVariable).getFunction()) and // Avoid elements in macro expansions, as they cannot be equated across copies not this.isInMacroExpansion() and exists(PointerOrArrayType type | diff --git a/c/misra/test/rules/RULE-8-13/test.c b/c/misra/test/rules/RULE-8-13/test.c index 1ac9e5028c..8c469e290c 100644 --- a/c/misra/test/rules/RULE-8-13/test.c +++ b/c/misra/test/rules/RULE-8-13/test.c @@ -75,4 +75,13 @@ 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; + __asm__("movb %bh (%eax)"); + return result; } \ No newline at end of file From f4f11605c5643d531b8c605a7574ffd160988939 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Sun, 20 Oct 2024 23:28:35 +0100 Subject: [PATCH 03/10] Rule 8.13: Correctly handle assignment into structs --- .../PointerShouldPointToConstTypeWhenPossible.ql | 4 ++-- ...ointerShouldPointToConstTypeWhenPossible.expected | 1 + c/misra/test/rules/RULE-8-13/test.c | 12 ++++++++++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql index 312662fe30..0150e39cb3 100644 --- a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql +++ b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql @@ -41,8 +41,8 @@ class NonConstPointerVariableCandidate extends Variable { not exists(VariableEffect effect | effect.getTarget() = this and // but not pointers that are only themselves modified - not effect.(AssignExpr).getLValue() = effect.getAnAccess() and - not effect.(CrementOperation).getOperand() = effect.getAnAccess() + 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 | diff --git a/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected b/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected index 39dbf04763..23ab0828b2 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:97:30:97:30 | s | $@ points to a non-const-qualified type. | test.c:97:30:97: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 8c469e290c..7bd42a1d98 100644 --- a/c/misra/test/rules/RULE-8-13/test.c +++ b/c/misra/test/rules/RULE-8-13/test.c @@ -84,4 +84,16 @@ test_r(int16_t *value) { // COMPLIANT - ignored because of the use of ASM int16_t result; __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; } \ No newline at end of file From 2976916569a0716880d28a47e59f1050027c0819 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Sun, 20 Oct 2024 23:34:45 +0100 Subject: [PATCH 04/10] Rule 8.13: Remove results in functions without bodies --- .../RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql | 2 ++ c/misra/test/rules/RULE-8-13/test.c | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql index 0150e39cb3..23579965a8 100644 --- a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql +++ b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql @@ -22,6 +22,8 @@ import codingstandards.cpp.alertreporting.HoldsForAllCopies 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()) and // Avoid elements in macro expansions, as they cannot be equated across copies diff --git a/c/misra/test/rules/RULE-8-13/test.c b/c/misra/test/rules/RULE-8-13/test.c index 7bd42a1d98..75ec2febc3 100644 --- a/c/misra/test/rules/RULE-8-13/test.c +++ b/c/misra/test/rules/RULE-8-13/test.c @@ -96,4 +96,7 @@ void test_struct(struct S *s) { // COMPLIANT void test_struct_2(struct S *s) { // NON_COMPLIANT - could be const s = 0; -} \ No newline at end of file +} + +void test_no_body(int *p); // COMPLIANT - no body, so cannot evaluate whether it + // should be const \ No newline at end of file From 808554e093df1054ff8e521d4dede66a702525f6 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Sun, 20 Oct 2024 23:42:48 +0100 Subject: [PATCH 05/10] Rule 8.13: Expand ASM test --- .../RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql | 7 ++++++- .../PointerShouldPointToConstTypeWhenPossible.expected | 2 +- c/misra/test/rules/RULE-8-13/test.c | 6 ++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql index 23579965a8..f5cf07f8fc 100644 --- a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql +++ b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql @@ -25,7 +25,12 @@ class NonConstPointerVariableCandidate extends Variable { // 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()) and + not exists(AsmStmt a | + a.getEnclosingFunction() = this.(LocalScopeVariable).getFunction() + or + // In a type declared locally + this.(Field).getDeclaringType+().getEnclosingFunction() = a.getEnclosingFunction() + ) and // Avoid elements in macro expansions, as they cannot be equated across copies not this.isInMacroExpansion() and exists(PointerOrArrayType type | diff --git a/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected b/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected index 23ab0828b2..e3e0963087 100644 --- a/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected +++ b/c/misra/test/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.expected @@ -12,4 +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:97:30:97:30 | s | $@ points to a non-const-qualified type. | test.c:97:30:97:30 | s | s | +| 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 75ec2febc3..7739f3aa77 100644 --- a/c/misra/test/rules/RULE-8-13/test.c +++ b/c/misra/test/rules/RULE-8-13/test.c @@ -82,6 +82,12 @@ int f17(char *p1) { // NON_COMPLIANT 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; } From ccb20d5210c59be23a3f56ce37656fb0160b0f69 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 00:00:58 +0100 Subject: [PATCH 06/10] Rule 8.13: Support crement operatios. --- c/misra/test/rules/RULE-8-13/test.c | 6 +++++- cpp/common/src/codingstandards/cpp/SideEffect.qll | 2 ++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/c/misra/test/rules/RULE-8-13/test.c b/c/misra/test/rules/RULE-8-13/test.c index 7739f3aa77..a2333d2a3d 100644 --- a/c/misra/test/rules/RULE-8-13/test.c +++ b/c/misra/test/rules/RULE-8-13/test.c @@ -105,4 +105,8 @@ void test_struct_2(struct S *s) { // NON_COMPLIANT - could be const } void test_no_body(int *p); // COMPLIANT - no body, so cannot evaluate whether it - // should be const \ No newline at end of file + // should be const + +void increment(int *p) { // COMPLIANT + *p++ = 1; +} \ 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 | From e90a13397b5c79817dfd2ac293174abc4ba6822e Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 00:01:13 +0100 Subject: [PATCH 07/10] Add change note. --- change_notes/2024-10-20-8-13-fixes.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 change_notes/2024-10-20-8-13-fixes.md 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..8b58ca821f --- /dev/null +++ b/change_notes/2024-10-20-8-13-fixes.md @@ -0,0 +1,5 @@ + - `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++ = ...`. \ No newline at end of file From 3dbdc9cb144b6f0b569b7e36117b79ece2844d82 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 09:43:45 +0100 Subject: [PATCH 08/10] Update release note --- change_notes/2024-10-20-8-13-fixes.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/change_notes/2024-10-20-8-13-fixes.md b/change_notes/2024-10-20-8-13-fixes.md index 8b58ca821f..d771023456 100644 --- a/change_notes/2024-10-20-8-13-fixes.md +++ b/change_notes/2024-10-20-8-13-fixes.md @@ -2,4 +2,5 @@ - 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++ = ...`. \ No newline at end of file + - Exclude false positives where the object pointed to by the variable is modified using `*p++ = ...`. + - Exclude false positives for functions without bodies. \ No newline at end of file From a02baeb18d5343d10570f2b8ca78a0b60c116e2d Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Mon, 21 Oct 2024 09:52:50 +0100 Subject: [PATCH 09/10] Update release note. --- change_notes/2024-10-20-8-13-fixes.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/change_notes/2024-10-20-8-13-fixes.md b/change_notes/2024-10-20-8-13-fixes.md index d771023456..6ee8e3a32c 100644 --- a/change_notes/2024-10-20-8-13-fixes.md +++ b/change_notes/2024-10-20-8-13-fixes.md @@ -3,4 +3,5 @@ - 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. \ No newline at end of file + - 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 From 460fb26a4b4971bf3014c8a3081a809105322f63 Mon Sep 17 00:00:00 2001 From: Luke Cartey Date: Tue, 22 Oct 2024 22:22:15 +0100 Subject: [PATCH 10/10] Reinstate macro instantiation results --- .../RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql | 2 -- .../codingstandards/cpp/alertreporting/HoldsForAllCopies.qll | 4 +--- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql index f5cf07f8fc..ddb8cbcdcc 100644 --- a/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql +++ b/c/misra/src/rules/RULE-8-13/PointerShouldPointToConstTypeWhenPossible.ql @@ -31,8 +31,6 @@ class NonConstPointerVariableCandidate extends Variable { // In a type declared locally this.(Field).getDeclaringType+().getEnclosingFunction() = a.getEnclosingFunction() ) and - // Avoid elements in macro expansions, as they cannot be equated across copies - not this.isInMacroExpansion() and exists(PointerOrArrayType type | // include only pointers which point to a const-qualified type this.getType() = type and 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