Merge branch 'main' into rvermeulen/fix-424

Author: knewbury01

change_notes/2023-10-04-aggregate-literals-from-variadic-templates.md

@@ -0,0 +1,2 @@
+ `M8-5-2` - `AggregateLiteralEnhancements.qll`:
+   - recognise aggregate literals initialized with parameters from variadic templates.

change_notes/2023-11-03-identifier-hiding-improvements.md

@@ -0,0 +1,3 @@
+ - `A2-10-1`, `RULE-5-3`:
+   - Reduce false positives by considering point of declaration for local variables.
+   - Reduce false negatives by considering catch block parameters to be in scope in the catch block.

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.

change_notes/2024-01-16-m5-2-10-arith-only.md

@@ -0,0 +1,2 @@
+ `M5-2-10` - `IncrementAndDecrementOperatorsMixedWithOtherOperatorsInExpression.ql`:
+   - only report use of the increment and decrement operations in conjunction with arithmetic operators, as specified by the rule. Notably we no longer report the expressions of the form `*p++`, which combine increment and dereferencing operations.

cpp/autosar/src/rules/M5-2-10/IncrementAndDecrementOperatorsMixedWithOtherOperatorsInExpression.ql

@@ -16,8 +16,9 @@
 
 import cpp
 import codingstandards.cpp.autosar
+import codingstandards.cpp.Expr
 
-from CrementOperation cop, Operation op, string name
+from CrementOperation cop, ArithmeticOperation op, string name
 where
   not isExcluded(cop) and
   not isExcluded(op,

cpp/autosar/test/rules/M5-2-10/test.cpp

@@ -6,4 +6,6 @@ void f1() {
   ++l1;             // COMPLIANT
   --l2;             // COMPLIANT
   l3 = l1 * l2;
+  int *p;
+  *p++; // COMPLIANT - * is not an arithmetic operator
 }

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/autosar/test/rules/M8-5-2/test.cpp

@@ -55,4 +55,13 @@ void test() {
   Bar b2{{0}};      // NON_COMPLIANT - missing explicit init, nested zero init
   StructNested n{}; // COMPLIANT
   StructNested n1 = {}; // COMPLIANT
-}
+}
+
+#include <initializer_list>
+template <class T> bool all_of(std::initializer_list<T>);
+
+template <typename... Args> constexpr bool all_of(Args... args) noexcept {
+  return all_of({args...}); // COMPLIANT - explicitly initialized via varargs
+}
+
+void test_all_of() { all_of(true, false, false); }

cpp/common/src/codingstandards/cpp/Expr.qll

@@ -2,6 +2,15 @@ import cpp
 private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import codingstandards.cpp.AccessPath
 
+/**
+ * A unary or binary arithmetic operation.
+ */
+class ArithmeticOperation extends Operation {
+  ArithmeticOperation() {
+    this instanceof UnaryArithmeticOperation or this instanceof BinaryArithmeticOperation
+  }
+}
+
 /** A full expression as defined in [intro.execution] of N3797. */
 class FullExpr extends Expr {
   FullExpr() {

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()
   )
 }
 

cpp/common/src/codingstandards/cpp/Scope.qll

@@ -42,7 +42,16 @@ private Element getParentScope(Element e) {
   then result = e.getParentScope()
   else (
     // Statements do no have a parent scope, so return the enclosing block.
-    result = e.(Stmt).getEnclosingBlock() or result = e.(Expr).getEnclosingBlock()
+    result = e.(Stmt).getEnclosingBlock()
+    or
+    result = e.(Expr).getEnclosingBlock()
+    or
+    // Catch block parameters don't have an enclosing scope, so attach them to the
+    // the block itself
+    exists(CatchBlock cb |
+      e = cb.getParameter() and
+      result = cb
+    )
   )
 }
 
@@ -209,7 +218,42 @@ private predicate hides_candidateStrict(UserVariable v1, UserVariable v2) {
   v2 = getPotentialScopeOfVariableStrict(v1) and
   v1.getName() = v2.getName() and
   // Member variables cannot hide other variables nor be hidden because the can be referenced through their qualified name.
-  not (v1.isMember() or v2.isMember())
+  not (v1.isMember() or v2.isMember()) and
+  (
+    // If v1 is a local variable, ensure that v1 is declared before v2
+    (
+      v1 instanceof LocalVariable and
+      // Ignore variables declared in conditional expressions, as they apply to
+      // the nested scope
+      not v1 = any(ConditionDeclExpr cde).getVariable() and
+      // Ignore variables declared in loops
+      not exists(Loop l | l.getADeclaration() = v1)
+    )
+    implies
+    exists(BlockStmt bs, DeclStmt v1Stmt, Stmt v2Stmt |
+      v1 = v1Stmt.getADeclaration() and
+      getEnclosingStmt(v2).getParentStmt*() = v2Stmt
+    |
+      bs.getIndexOfStmt(v1Stmt) <= bs.getIndexOfStmt(v2Stmt)
+    )
+  )
+}
+
+/**
+ * Gets the enclosing statement of the given variable, if any.
+ */
+private Stmt getEnclosingStmt(LocalScopeVariable v) {
+  result.(DeclStmt).getADeclaration() = v
+  or
+  exists(ConditionDeclExpr cde |
+    cde.getVariable() = v and
+    result = cde.getEnclosingStmt()
+  )
+  or
+  exists(CatchBlock cb |
+    cb.getParameter() = v and
+    result = cb.getEnclosingStmt()
+  )
 }
 
 /** Holds if `v2` hides `v1`. */

cpp/common/src/codingstandards/cpp/enhancements/AggregateLiteralEnhancements.qll

@@ -85,6 +85,9 @@ module ArrayAggregateLiterals {
         // The aggregate itself not be compiler generated, or in a macro expansion, otherwise our line numbers will be off
         not cal.isCompilerGenerated() and
         not cal.isInMacroExpansion() and
+        // Ignore cases where the compilerGenerated value is a variable access targeting
+        // a parameter, as these are generated from variadic templates
+        not compilerGeneratedVal.(VariableAccess).getTarget() instanceof Parameter and
         exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |
           compilerGeneratedVal.getLocation().hasLocationInfo(filepath, _, _, endline, endcolumn) and
           previousExpr

cpp/common/test/rules/identifierhidden/IdentifierHidden.expected

@@ -5,3 +5,7 @@
 | test.cpp:23:13:23:15 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
 | test.cpp:26:12:26:14 | id1 | Variable is hiding variable $@. | test.cpp:1:5:1:7 | id1 | id1 |
 | test.cpp:27:14:27:16 | id1 | Variable is hiding variable $@. | test.cpp:26:12:26:14 | id1 | id1 |
+| test.cpp:65:11:65:11 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:67:9:67:9 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:70:12:70:12 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |
+| test.cpp:75:16:75:16 | i | Variable is hiding variable $@. | test.cpp:61:7:61:7 | i | i |

cpp/common/test/rules/identifierhidden/test.cpp

@@ -40,4 +40,38 @@ template <class T> constexpr T foo1 = T(1.1L);
 template <class T, class R> T f(T r) {
   T v = foo1<T> * r * r;  // COMPLIANT
   T v1 = foo1<R> * r * r; // COMPLIANT
+}
+
+void test_scope_order() {
+  {
+    {
+      int i; // COMPLIANT
+    }
+    int i; // COMPLIANT
+  }
+
+  for (int i = 0; i < 10; i++) { // COMPLIANT
+  }
+
+  try {
+
+  } catch (int i) { // COMPLIANT
+  }
+
+  int i; // COMPLIANT
+
+  {
+    {
+      int i; // NON_COMPLIANT
+    }
+    int i; // NON_COMPLIANT
+  }
+
+  for (int i = 0; i < 10; i++) { // NON_COMPLIANT
+  }
+
+  try {
+
+  } catch (int i) { // NON_COMPLIANT
+  }
 }