Merge pull request #425 from github/lcartey/m6-5-5-loop-modification

`M6-5-5`: Improve detection of modification by reference

Author: rvermeulen

change_notes/2023-11-05-m6-5-5-const-refs.md

@@ -0,0 +1,3 @@
+ - `M6-5-5`:
+   - Reduce false positives by no longer considering the taking of a const reference as a modification.
+   - Improve detection of non-local modification of loop iteration variables to reduce false positives.

cpp/autosar/test/rules/M6-5-5/LoopControlVariableModifiedInLoopExpression.expected

@@ -1 +1,2 @@
 | test.cpp:24:8:24:15 | testFlag | Loop control variable testFlag is modified in the loop update expression. |
+| test.cpp:47:12:47:12 | y | Loop control variable y is modified in the loop update expression. |

cpp/autosar/test/rules/M6-5-5/test.cpp

@@ -24,3 +24,27 @@ void test_loop_control_variable_modified_in_expression() {
        testFlag = updateFlagWithIncrement(++x)) { // NON_COMPLIANT
   }
 }
+
+#include <vector>
+
+void test_const_refs(std::vector<int> v) {
+  std::vector<int>::iterator first = v.begin();
+  std::vector<int>::iterator last = v.end();
+  // call to operator!= passes a const reference to first
+  for (; first != last; first++) { // COMPLIANT
+  }
+}
+
+void update(std::vector<int>::iterator &f, const int &x, int &y) {}
+
+void test_const_refs_update(std::vector<int> v) {
+  std::vector<int>::iterator last = v.end();
+  int x = 0;
+  int y = 0;
+  // call to operator!= passes a const reference to first
+  for (std::vector<int>::iterator first = v.begin(); first != last; update(
+           first, x, // COMPLIANT - first is a loop counter, so can be modified
+           y)) {     // NON_COMPLIANT - y is modified and is not a loop counter
+    first + 1;
+  }
+}

cpp/common/src/codingstandards/cpp/Loops.qll

@@ -70,6 +70,13 @@ class MemberAssignmentOperation extends FunctionCall {
  */
 pragma[noopt]
 Variable getALoopCounter(ForStmt fs) {
+  // ------------------------------------------------------------------------------------------------
+  // NOTE: This is an updated version of ForStmt.getAnIterationVariable(), handling additional cases.
+  //       The use of pragma[noopt] is retained from the original code, as we haven't determined
+  //       whether it's still necessary across a broad range of databases. As a noopt predicate, it
+  //       includes a degree of duplication as the join order is defined based on the order of the
+  //       conditions.
+  // ------------------------------------------------------------------------------------------------
   // check that it is assigned to, incremented or decremented in the update
   exists(Expr updateOpRoot, Expr updateOp |
     updateOpRoot = fs.getUpdate() and
@@ -106,6 +113,15 @@ Variable getALoopCounter(ForStmt fs) {
     )
     or
     updateOp = result.getAnAssignedValue()
+    or
+    // updateOp is an access whose address is taken in a non-const way
+    exists(FunctionCall fc, VariableAccess va |
+      fc = updateOp and
+      fc instanceof FunctionCall and
+      fc.getAnArgument() = va and
+      va = result.getAnAccess() and
+      va.isAddressOfAccessNonConst()
+    )
   ) and
   result instanceof Variable and
   // checked or used in the condition
@@ -260,7 +276,7 @@ predicate isLoopControlVarModifiedInLoopCondition(
   loopControlVariableAccess = forLoop.getCondition().getAChild+() and
   (
     loopControlVariableAccess.isModified() or
-    loopControlVariableAccess.isAddressOfAccess()
+    loopControlVariableAccess.isAddressOfAccessNonConst()
   )
 }
 
@@ -277,7 +293,7 @@ predicate isLoopControlVarModifiedInLoopExpr(
   loopControlVariableAccess = forLoop.getUpdate().getAChild() and
   (
     loopControlVariableAccess.isModified() or
-    loopControlVariableAccess.isAddressOfAccess()
+    loopControlVariableAccess.isAddressOfAccessNonConst()
   )
 }