Merge pull request #765 from github/lcartey/rule-8-13-copies

`RULE-8-13`: Address false positive issues

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<NonConstPointerVariableCandidate, Variable>::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()

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 |

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 <stdint.h>
+
+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;
 }

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`.

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 |

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

@@ -85,11 +85,9 @@ module HoldsForAllCopies<CandidateElementSig CandidateElement, ElementSetSig Ele
       string filepath, int startline, int startcolumn, int endline, int endcolumn
     ) {
       exists(CandidateElement s |
-        // Only consider candidates where we can match up the location
-        isNotWithinMacroExpansion(s) and
         hasLocation(s, filepath, startline, startcolumn, endline, endcolumn) and
         // All relevant elements that occur at the same location are candidates
-        forex(RelevantElement relevantElement | s = relevantElement.getCandidateElement() |
+        forall(RelevantElement relevantElement | s = relevantElement.getCandidateElement() |
           relevantElement instanceof CandidateElement
         )
       )