Merge branch 'main' into rvermeulen/fix-rule-6-1

Author: knewbury01

.github/workflows/finalize-release.yml

@@ -109,14 +109,14 @@ jobs:
           git switch main
           git pull --ff-only origin main
 
-          git switch -c release-automation/bump-version
+          git switch -c "release-automation/bump-version-to-$NEXT_VERSION"
 
           # We are running the script in the tooling directory with the release directory as the working directory
           ../tooling/scripts/release/bump-version.sh "$NEXT_VERSION"
 
           git add -u .
           git commit -m "Bump version to $NEXT_VERSION"
-          git push --set-upstream origin release-automation/bump-version
+          git push --set-upstream origin "release-automation/bump-version-to-$NEXT_VERSION"
 
-          gh pr create --repo $GITHUB_REPOSITORY --base main --head release-automation/bump-version --body "Bump the version of main to $NEXT_VERSION" --title "Bump version to $NEXT_VERSION"
+          gh pr create --repo $GITHUB_REPOSITORY --base main --head "release-automation/bump-version-to-$NEXT_VERSION" --body "Bump the version of main to $NEXT_VERSION" --title "Bump version to $NEXT_VERSION"
         working-directory: release 

change_notes/2024-03-19-change-alert-a3-3-1.md

@@ -0,0 +1,2 @@
+- `A3-3-1` - `ExternalLinkageNotDeclaredInHeaderFile.ql`:
+  - Adjust the alert message to comply with the style guide.

change_notes/2024-03-22-fix-fp-ctr55-cpp.md

@@ -0,0 +1,2 @@
+- `CTR55-CPP` - `DoNotUseAnAdditiveOperatorOnAnIterator.ql`:
+  - Address reported FP in #374. Improve logic on valid end checks and size checks on iterators.

cpp/autosar/src/rules/A3-3-1/ExternalLinkageNotDeclaredInHeaderFile.ql

@@ -38,4 +38,4 @@ where
   // Main functions are an exception to the rule
   not de.getDeclaration() instanceof MainFunction and
   if de.getDeclaration() instanceof Function then kind = "function" else kind = "object"
-select de, "Externally linked " + kind + " " + de.getName() + " not declared in header file."
+select de, "Externally linked " + kind + " '" + de.getName() + "' not declared in header file."

cpp/autosar/test/rules/A3-3-1/ExternalLinkageNotDeclaredInHeaderFile.expected

@@ -1,6 +1,6 @@
-| test.cpp:3:5:3:6 | definition of g1 | Externally linked object g1 not declared in header file. |
-| test.cpp:4:12:4:13 | declaration of g2 | Externally linked object g2 not declared in header file. |
-| test.cpp:10:5:10:6 | definition of l1 | Externally linked object l1 not declared in header file. |
-| test.cpp:11:6:11:7 | definition of f1 | Externally linked function f1 not declared in header file. |
-| test.cpp:22:5:22:5 | definition of f | Externally linked function f not declared in header file. |
-| test.cpp:25:5:25:6 | declaration of f1 | Externally linked function f1 not declared in header file. |
+| test.cpp:3:5:3:6 | definition of g1 | Externally linked object 'g1' not declared in header file. |
+| test.cpp:4:12:4:13 | declaration of g2 | Externally linked object 'g2' not declared in header file. |
+| test.cpp:10:5:10:6 | definition of l1 | Externally linked object 'l1' not declared in header file. |
+| test.cpp:11:6:11:7 | definition of f1 | Externally linked function 'f1' not declared in header file. |
+| test.cpp:22:5:22:5 | definition of f | Externally linked function 'f' not declared in header file. |
+| test.cpp:25:5:25:6 | declaration of f1 | Externally linked function 'f1' not declared in header file. |

cpp/autosar/test/rules/A3-3-1/test.cpp

@@ -35,4 +35,8 @@ namespace n {
 void f5() { // COMPLIANT
   int i = 0;
 }
-} // namespace n
+} // namespace n
+
+const int c = 1;                // COMPLIANT - internal linkage
+const char *const str2 = "foo"; // COMPLIANT - internal linkage
+constexpr int k = 1;            // COMPLIANT - internal linkage

cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql

@@ -80,16 +80,7 @@ where
     iteratorCreationCall = outputContainer.getAnIteratorFunctionCall() and
     iteratorCreationCall = c.getOutputIteratorSource()
   |
-    // Guarded by a bounds check that ensures our destination is larger than "some" value
-    exists(
-      GuardCondition guard, ContainerAccessWithoutRangeCheck::ContainerSizeCall sizeCall,
-      boolean branch
-    |
-      globalValueNumber(sizeCall.getQualifier()) =
-        globalValueNumber(iteratorCreationCall.getQualifier()) and
-      guard.controls(c.getBasicBlock(), branch) and
-      relOpWithSwapAndNegate(guard, sizeCall, _, Greater(), _, branch)
-    )
+    sizeCompareBoundsChecked(iteratorCreationCall, c)
     or
     // Container created with sufficient size for the input
     exists(ContainerAccessWithoutRangeCheck::ContainerConstructorCall outputIteratorConstructor |

cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql

@@ -14,24 +14,81 @@
 import cpp
 import codingstandards.cpp.cert
 import codingstandards.cpp.Iterators
+import semmle.code.cpp.controlflow.Dominance
 
-from ContainerIteratorAccess it
+/**
+ * Models a call to an iterator's `operator+`
+ */
+class AdditionOperatorFunctionCall extends AdditiveOperatorFunctionCall {
+  AdditionOperatorFunctionCall() { this.getTarget().hasName("operator+") }
+}
+
+/**
+ *  There exists a calculation for the reference one passed the end of some container
+ *  An example derivation is:
+ *     `end = begin() + size()`
+ */
+Expr getDerivedReferenceToOnePassedTheEndElement(Expr containerReference) {
+  exists(
+    ContainerAccessWithoutRangeCheck::ContainerSizeCall size,
+    ContainerAccessWithoutRangeCheck::ContainerBeginCall begin, AdditionOperatorFunctionCall calc
+  |
+    result = calc
+  |
+    DataFlow::localFlow(DataFlow::exprNode(size), DataFlow::exprNode(calc.getAChild+())) and
+    DataFlow::localFlow(DataFlow::exprNode(begin), DataFlow::exprNode(calc.getAChild+())) and
+    //make sure its the same container providing its size as giving the begin
+    globalValueNumber(begin.getQualifier()) = globalValueNumber(size.getQualifier()) and
+    containerReference = begin.getQualifier()
+  )
+}
+
+/**
+ * a wrapper predicate for a couple of types of permitted end bounds checks
+ */
+Expr getReferenceToOnePassedTheEndElement(Expr containerReference) {
+  //a container end access - v.end()
+  result instanceof ContainerAccessWithoutRangeCheck::ContainerEndCall and
+  containerReference = result.(FunctionCall).getQualifier()
+  or
+  result = getDerivedReferenceToOnePassedTheEndElement(containerReference)
+}
+
+/**
+ * some guard exists like: `iterator != end`
+ * where a relevant`.end()` call flowed into end
+ */
+predicate isUpperBoundEndCheckedIteratorAccess(IteratorSource source, ContainerIteratorAccess it) {
+  exists(
+    Expr referenceToOnePassedTheEndElement, BasicBlock basicBlockOfIteratorAccess,
+    GuardCondition upperBoundCheck, ContainerIteratorAccess checkedIteratorAccess,
+    Expr containerReferenceFromEndGuard
+  |
+    //sufficient end guard
+    referenceToOnePassedTheEndElement =
+      getReferenceToOnePassedTheEndElement(containerReferenceFromEndGuard) and
+    //guard controls the access
+    upperBoundCheck.controls(basicBlockOfIteratorAccess, _) and
+    basicBlockOfIteratorAccess.contains(it) and
+    //guard is comprised of end check and an iterator access
+    DataFlow::localFlow(DataFlow::exprNode(referenceToOnePassedTheEndElement),
+      DataFlow::exprNode(upperBoundCheck.getChild(_))) and
+    upperBoundCheck.getChild(_) = checkedIteratorAccess and
+    //make sure its the same iterator being checked in the guard as accessed
+    checkedIteratorAccess.getOwningContainer() = it.getOwningContainer() and
+    //if its the end call itself (or its parts), make sure its the same container providing its end as giving the iterator
+    globalValueNumber(containerReferenceFromEndGuard) = globalValueNumber(source.getQualifier()) and
+    // and the guard call we match must be after the assignment call (to avoid valid guards protecting new iterator accesses further down)
+    source.getASuccessor*() = upperBoundCheck
+  )
+}
+
+from ContainerIteratorAccess it, IteratorSource source
 where
   not isExcluded(it, IteratorsPackage::doNotUseAnAdditiveOperatorOnAnIteratorQuery()) and
   it.isAdditiveOperation() and
   not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and
-  // we get the neraby assignment
-  not exists(STLContainer c, FunctionCall nearbyAssigningIteratorCall, FunctionCall guardCall |
-    nearbyAssigningIteratorCall = it.getANearbyAssigningIteratorCall() and
-    // we look for calls to size or end
-    (guardCall = c.getACallToSize() or guardCall = c.getAnIteratorEndFunctionCall()) and
-    // such that the call to size is before this
-    // access
-    guardCall = it.getAPredecessor*() and
-    // and it uses the same qualifier as the one we were just assigned
-    nearbyAssigningIteratorCall.getQualifier().(VariableAccess).getTarget() =
-      guardCall.getQualifier().(VariableAccess).getTarget() and
-    // and the size call we match must be after the assignment call
-    nearbyAssigningIteratorCall.getASuccessor*() = guardCall
-  )
+  source = it.getANearbyAssigningIteratorCall() and
+  not isUpperBoundEndCheckedIteratorAccess(source, it) and
+  not sizeCompareBoundsChecked(source, it)
 select it, "Increment of iterator may overflow since its bounds are not checked."

cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected

@@ -1,4 +1,8 @@
 | test.cpp:8:7:8:7 | i | Increment of iterator may overflow since its bounds are not checked. |
 | test.cpp:9:9:9:9 | i | Increment of iterator may overflow since its bounds are not checked. |
 | test.cpp:10:9:10:9 | i | Increment of iterator may overflow since its bounds are not checked. |
-| test.cpp:27:31:27:31 | i | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:22:18:22:18 | i | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:28:31:28:31 | i | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:41:5:41:8 | end2 | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:53:42:53:42 | i | Increment of iterator may overflow since its bounds are not checked. |
+| test.cpp:64:15:64:15 | i | Increment of iterator may overflow since its bounds are not checked. |

cpp/cert/test/rules/CTR55-CPP/test.cpp

@@ -20,10 +20,47 @@ void f1(std::vector<int> &v) {
   }
   for (auto i = v.begin(),
             l = (i + std::min(static_cast<std::vector<int>::size_type>(10),
-                              v.size()));
-       i != l; ++i) { // COMPLIANT
+                              v.size())); // NON_COMPLIANT - technically in the
+                                          // calculation
+       i != l; ++i) {                     // COMPLIANT
   }
 
   for (auto i = v.begin();; ++i) { // NON_COMPLIANT
   }
+}
+
+void test_fp_reported_in_374(std::vector<int> &v) {
+  {
+    auto end = v.end();
+    for (auto i = v.begin(); i != end; ++i) { // COMPLIANT
+    }
+  }
+
+  {
+    auto end2 = v.end();
+    end2++; // NON_COMPLIANT
+    for (auto i = v.begin(); i != end2;
+         ++i) { // NON_COMPLIANT[FALSE_NEGATIVE] - case of invalidations to
+                // check before use expected to be less frequent, can model in
+                // future if need be
+    }
+  }
+}
+
+void test(std::vector<int> &v, std::vector<int> &v2) {
+  {
+    auto end = v2.end();
+    for (auto i = v.begin(); i != end; ++i) { // NON_COMPLIANT - wrong check
+    }
+  }
+}
+
+void test2(std::vector<int> &v) {
+  auto i = v.begin();
+  while (1) {
+    auto i2 = ((i != v.end()) != 0);
+    if (!i2)
+      break;
+    (void)((++i)); // COMPLIANT[FALSE_POSITIVE]
+  }
 }