Merge pull request #338 from github/lcartey/final-compiler-compat-issues

Fixing the final compatibility issues

Author: jsinglet

change_notes/2023-08-02-a8-4-13-false-positives.md

@@ -0,0 +1,2 @@
+ - `A8-4-13`
+  - Address false positives caused by missing modelling of modifying operations for smart pointers for some standard libraries (such as libstdc++).

change_notes/2023-08-02-smart-pointers.md

@@ -0,0 +1,5 @@
+ - `A20-8-1`/`MEM56-CPP`
+    - Address false negatives caused by lack of modelling of flow through smart pointers.
+    - Reduce flow paths through standard library headers to simplify results.
+ - `A18-1-4`
+    - Address false positives caused by missing modelling of modifying operations for smart pointers for some standard libraries (such as libstdc++).

change_notes/2023-08-03-string-replace.md

@@ -0,0 +1,2 @@
+ - `STR51-CPP`
+   - Address false negatives caused by incomplete modelling of the `std::string::replace()` function.

change_notes/2023-08-04-a15-5-1-noexcept.md

@@ -0,0 +1,3 @@
+ - `A15-5-1`
+   - Rephrase alert message for `noalert(false)` special functions to clarify that this permits exceptions.
+   - Additional results for implicit `noexcept(true)` special functions highlighting that the specification should be made explicit.

cpp/autosar/src/rules/A15-2-2/ConstructorErrorLeavesObjectInInvalidState.ql

@@ -78,11 +78,18 @@ class DeleteWrapperFunction extends Function {
 class ExceptionThrownInConstructor extends ExceptionThrowingExpr {
   Constructor c;
 
-  ExceptionThrownInConstructor() { exists(getAFunctionThrownType(c, this)) }
+  ExceptionThrownInConstructor() {
+    exists(getAFunctionThrownType(c, this)) and
+    // The constructor is within the users source code
+    exists(c.getFile().getRelativePath())
+  }
 
   Constructor getConstructor() { result = c }
 }
 
+/**
+ * Add the `nodes` predicate to ensure results with an empty path are still reported.
+ */
 query predicate nodes(ExceptionFlowNode node) { any() }
 
 from

cpp/autosar/src/rules/A15-5-1/SpecialFunctionMissingNoExceptSpecification.ql

@@ -22,15 +22,25 @@ import codingstandards.cpp.exceptions.ExceptionSpecifications
 from SpecialFunction f, string message
 where
   not isExcluded(f, Exceptions2Package::specialFunctionMissingNoExceptSpecificationQuery()) and
-  not isNoExceptTrue(f) and
+  not isFDENoExceptTrue(f.getDefinition()) and
   not f.isCompilerGenerated() and
   not f.isDeleted() and
   not f.isDefaulted() and
   (
     isNoExceptExplicitlyFalse(f) and
-    message = f.getQualifiedName() + " should not be noexcept(false)."
+    message =
+      "Special function " + f.getQualifiedName() +
+        " has a noexcept(false) specification that permits exceptions."
     or
+    isNoExceptTrue(f) and
+    message =
+      f.getQualifiedName() +
+        " has an implicit noexcept(true) specification but should make that explicit."
+    or
+    not isNoExceptTrue(f) and
     not isNoExceptExplicitlyFalse(f) and
-    message = f.getQualifiedName() + " is implicitly noexcept(false) and might throw."
+    message =
+      "Special function " + f.getQualifiedName() +
+        " has an implicit noexcept(false) specification that permits exceptions."
   )
 select f, message

cpp/autosar/src/rules/A18-1-4/PointerToAnElementOfAnArrayPassedToASmartPointer.ql

@@ -46,17 +46,28 @@ class SingleObjectSmartPointerArrayConstructionConfig extends TaintTracking::Con
       (
         sp.getAConstructorCallWithExternalObjectConstruction().getAnArgument() = sink.asExpr()
         or
-        sink.asExpr() =
-          any(FunctionCall fc, MemberFunction mf |
-            mf = fc.getTarget() and
-            mf.getDeclaringType() = sp and
-            mf.getName() = "reset"
-          |
-            fc.getArgument(0)
-          )
+        sink.asExpr() = sp.getAResetCall().getArgument(0)
       )
     )
   }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node source, DataFlow::Node sink) {
+    exists(AutosarUniquePointer sp, FunctionCall fc |
+      fc = sp.getAReleaseCall() and
+      source.asExpr() = fc.getQualifier() and
+      sink.asExpr() = fc
+    )
+  }
+
+  override predicate isSanitizerIn(DataFlow::Node node) {
+    // Exclude flow into header files outside the source archive which are summarized by the
+    // additional taint steps above.
+    exists(AutosarUniquePointer sp |
+      sp.getAReleaseCall().getTarget() = node.asExpr().(ThisExpr).getEnclosingFunction()
+    |
+      not exists(node.getLocation().getFile().getRelativePath())
+    )
+  }
 }
 
 from

cpp/autosar/src/rules/A8-4-13/SharedPtrPassedToFunctionWithImproperSemantics.ql

@@ -19,19 +19,15 @@ import cpp
 import codingstandards.cpp.autosar
 import codingstandards.cpp.SmartPointers
 
-Expr underlyingObjectAffectingSharedPointerExpr(Function f) {
-  result =
-    any(VariableAccess va, FunctionCall fc |
-      va.getEnclosingFunction() = f and
-      // strip the type so as to include reference parameter types
-      va.getType().stripType() instanceof AutosarSharedPointer and
-      fc.getTarget().getDeclaringType().stripType() instanceof AutosarSharedPointer and
-      fc.getQualifier() = va and
-      // include only calls to methods which modify the underlying object
-      fc.getTarget().hasName(["operator=", "reset", "swap"])
-    |
-      va
-    )
+VariableAccess underlyingObjectAffectingSharedPointerExpr(Function f) {
+  exists(FunctionCall fc |
+    // Find a call in the function
+    fc.getEnclosingFunction() = f and
+    // include only calls to methods which modify the underlying object
+    fc = any(AutosarSharedPointer s).getAModifyingCall() and
+    // Report the qualifier
+    fc.getQualifier() = result
+  )
 }
 
 predicate flowsToUnderlyingObjectAffectingExpr(Parameter p) {

cpp/autosar/test/rules/A1-1-2.3/options.clang

@@ -0,0 +1 @@
+-w

cpp/autosar/test/rules/A12-0-2/test.cpp

@@ -1,5 +1,5 @@
 #include <cstdlib>
-#include <string>
+#include <cstring>
 
 class A {
 public:

cpp/autosar/test/rules/A15-5-1/SpecialFunctionMissingNoExceptSpecification.expected

@@ -1,11 +1,11 @@
-| test.cpp:5:3:5:9 | ~ClassA | ClassA::~ClassA should not be noexcept(false). |
-| test.cpp:9:3:9:9 | ~ClassB | ClassB::~ClassB should not be noexcept(false). |
-| test.cpp:38:6:38:20 | operator delete | operator delete is implicitly noexcept(false) and might throw. |
-| test.cpp:43:6:43:20 | operator delete | operator delete is implicitly noexcept(false) and might throw. |
-| test.cpp:53:11:53:19 | operator= | ClassF::operator= should not be noexcept(false). |
-| test.cpp:63:3:63:8 | ClassH | ClassH::ClassH should not be noexcept(false). |
-| test.cpp:68:6:68:9 | swap | swap is implicitly noexcept(false) and might throw. |
-| test.cpp:72:6:72:9 | swap | swap should not be noexcept(false). |
-| test.cpp:77:8:77:11 | swap | ClassI::swap is implicitly noexcept(false) and might throw. |
-| test.cpp:82:8:82:11 | swap | ClassJ::swap is implicitly noexcept(false) and might throw. |
-| test.cpp:88:6:88:6 | swap | swap is implicitly noexcept(false) and might throw. |
+| test.cpp:5:3:5:9 | ~ClassA | Special function ClassA::~ClassA has a noexcept(false) specification that permits exceptions. |
+| test.cpp:9:3:9:9 | ~ClassB | Special function ClassB::~ClassB has a noexcept(false) specification that permits exceptions. |
+| test.cpp:38:6:38:20 | operator delete | operator delete has an implicit noexcept(true) specification but should make that explicit. |
+| test.cpp:43:6:43:20 | operator delete | operator delete has an implicit noexcept(true) specification but should make that explicit. |
+| test.cpp:53:11:53:19 | operator= | Special function ClassF::operator= has a noexcept(false) specification that permits exceptions. |
+| test.cpp:63:3:63:8 | ClassH | Special function ClassH::ClassH has a noexcept(false) specification that permits exceptions. |
+| test.cpp:68:6:68:9 | swap | Special function swap has an implicit noexcept(false) specification that permits exceptions. |
+| test.cpp:72:6:72:9 | swap | Special function swap has a noexcept(false) specification that permits exceptions. |
+| test.cpp:77:8:77:11 | swap | Special function ClassI::swap has an implicit noexcept(false) specification that permits exceptions. |
+| test.cpp:82:8:82:11 | swap | Special function ClassJ::swap has an implicit noexcept(false) specification that permits exceptions. |
+| test.cpp:88:6:88:6 | swap | Special function swap has an implicit noexcept(false) specification that permits exceptions. |

cpp/autosar/test/rules/A15-5-1/test.cpp

@@ -1,6 +1,6 @@
 #include "stddef.h"
+#include <new>
 #include <stdexcept>
-
 class ClassA {
   ~ClassA() noexcept(false) { throw std::exception(); } // NON_COMPLIANT
 };
@@ -36,12 +36,12 @@ class ClassD {
 };
 
 void operator delete(void *ptr) { // NON_COMPLIANT
-  // NOTE: cannot be declared noexcept(false)
+  // NOTE: defaults to noexcept(true)
   throw std::exception();
 }
 
 void operator delete(void *ptr, size_t size) { // NON_COMPLIANT
-  // NOTE: cannot be declared noexcept(false)
+  // NOTE: defaults to noexcept(true)
   throw std::exception();
 }
 

cpp/autosar/test/rules/A16-2-2/UnusedIncludeDirectives.expected.clang

@@ -0,0 +1 @@
+| test.cpp:1:1:1:19 | #include "test.hpp" | Nothing in this file uses anything from "test.hpp" |

cpp/autosar/test/rules/A16-2-2/UnusedIncludeDirectives.expected.gcc

@@ -0,0 +1 @@
+| test.cpp:1:1:1:19 | #include "test.hpp" | Nothing in this file uses anything from "test.hpp" |

cpp/autosar/test/rules/A16-2-2/UnusedIncludeDirectives.expected.qcc

@@ -0,0 +1 @@
+| test.cpp:1:1:1:19 | #include "test.hpp" | Nothing in this file uses anything from "test.hpp" |

cpp/autosar/test/rules/A16-2-2/test.cpp

@@ -1,5 +1,5 @@
-#include "test.hpp"  //NON_COMPLIANT
-#include <algorithm> //NON_COMPLIANT
+#include "test.hpp" //NON_COMPLIANT
+#include <algorithm> //NON_COMPLIANT - redundant but not useless on real compilers
 #include <vector>    //COMPLIANT
 
 std::vector<int> v;

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

@@ -11,7 +11,7 @@ void *malloc1(int b) { // NON_COMPLIANT - recursion
   return malloc1(b - 1);
 }
 
-void *malloc2(int b) __attribute__((no_caller_saved_registers, __malloc__));
+void *malloc2(int b) __attribute__((__malloc__));
 void *malloc2(int b) { // NON_COMPLIANT - execution doesn't depend on b
 
   for (int i = 0; i < 10; i++) {

cpp/autosar/test/rules/A18-5-6/test.cpp

@@ -4,7 +4,7 @@ void *malloc1(int b) __attribute__((malloc));
 
 void *malloc1(int b) { return nullptr; } // NON_COMPLIANT
 
-void *malloc3(int b) __attribute__((no_caller_saved_registers, __malloc__));
+void *malloc3(int b) __attribute__((__malloc__));
 void *malloc3(int b) { return nullptr; } // NON_COMPLIANT
 
 void h1() {} // NON_COMPLIANT

cpp/autosar/test/rules/M18-0-5/test.cpp

@@ -1,4 +1,4 @@
-#include <string>
+#include <cstring>
 
 void test_unbounded_str_funs() {
   char str1[] = "Sample string";

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

@@ -1,4 +1,4 @@
-#include <string>
+#include <cstring>
 
 struct S1 {
   int i, j, k;

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

@@ -1,4 +1,4 @@
-#include <string>
+#include <cstring>
 
 class trivial {};
 

cpp/common/src/codingstandards/cpp/Concurrency.qll

@@ -114,7 +114,10 @@ class CPPMutexFunctionCall extends MutexFunctionCall {
   /**
    * Holds if this `CPPMutexFunctionCall` is a lock.
    */
-  override predicate isLock() { getTarget().getName() = "lock" }
+  override predicate isLock() {
+    not isLockingOperationWithinLockingOperation(this) and
+    getTarget().getName() = "lock"
+  }
 
   /**
    * Holds if this `CPPMutexFunctionCall` is a speculative lock, defined as calling
@@ -172,6 +175,7 @@ class CMutexFunctionCall extends MutexFunctionCall {
    * Holds if this `CMutexFunctionCall` is a lock.
    */
   override predicate isLock() {
+    not isLockingOperationWithinLockingOperation(this) and
     getTarget().getName() = ["mtx_lock", "mtx_timedlock", "mtx_trylock"]
   }
 
@@ -296,6 +300,16 @@ abstract class LockingOperation extends FunctionCall {
    * Holds if this is an unlock operation
    */
   abstract predicate isUnlock();
+
+  /**
+   * Holds if this locking operation is really a locking operation within a
+   * designated locking operation. This library assumes the underlying locking
+   * operations are implemented correctly in that calling a `LockingOperation`
+   * results in the creation of a singular lock.
+   */
+  predicate isLockingOperationWithinLockingOperation(LockingOperation inner) {
+    exists(LockingOperation outer | outer.getTarget() = inner.getEnclosingFunction())
+  }
 }
 
 /**
@@ -317,6 +331,7 @@ class RAIIStyleLock extends LockingOperation {
    * Holds if this is a lock operation
    */
   override predicate isLock() {
+    not isLockingOperationWithinLockingOperation(this) and
     this instanceof ConstructorCall and
     lock = getArgument(0).getAChild*() and
     // defer_locks don't cause a lock

cpp/common/src/codingstandards/cpp/SmartPointers.qll

@@ -29,6 +29,11 @@ abstract class AutosarSmartPointer extends Class {
       )
   }
 
+  FunctionCall getAGetCall() {
+    result.getTarget().hasName("get") and
+    result.getQualifier().getType().stripType() = this
+  }
+
   FunctionCall getAnInitializerExpr() {
     result =
       any(FunctionCall fc |
@@ -51,10 +56,25 @@ abstract class AutosarSmartPointer extends Class {
           AutosarSmartPointer
       )
   }
+
+  FunctionCall getAResetCall() {
+    result.getTarget().hasName("reset") and
+    result.getQualifier().getType().stripType() = this
+  }
+
+  FunctionCall getAModifyingCall() {
+    result.getTarget().hasName(["operator=", "reset", "swap"]) and
+    result.getQualifier().getType().stripType() = this
+  }
 }
 
 class AutosarUniquePointer extends AutosarSmartPointer {
   AutosarUniquePointer() { this.hasQualifiedName("std", "unique_ptr") }
+
+  FunctionCall getAReleaseCall() {
+    result.getTarget().hasName("release") and
+    result.getQualifier().getType().stripType() = this
+  }
 }
 
 class AutosarSharedPointer extends AutosarSmartPointer {

cpp/common/src/codingstandards/cpp/rules/ownedpointervaluestoredinunrelatedsmartpointer/OwnedPointerValueStoredInUnrelatedSmartPointer.qll

@@ -29,6 +29,34 @@ private class PointerToSmartPointerConstructorFlowConfig extends TaintTracking::
       cc.getArgument(0) = sink.asExpr()
     )
   }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    // Summarize flow through constructor calls
+    exists(AutosarSmartPointer sp, ConstructorCall cc |
+      sp.getAConstructorCall() = cc and
+      cc = node2.asExpr() and
+      cc.getArgument(0) = node1.asExpr()
+    )
+    or
+    // Summarize flow through get() calls
+    exists(AutosarSmartPointer sp, FunctionCall fc |
+      sp.getAGetCall() = fc and
+      fc = node2.asExpr() and
+      fc.getQualifier() = node1.asExpr()
+    )
+  }
+
+  override predicate isSanitizerIn(DataFlow::Node node) {
+    // Exclude flow into header files outside the source archive which are summarized by the
+    // additional taint steps above.
+    exists(AutosarSmartPointer sp |
+      sp.getAConstructorCall().getTarget().getAParameter() = node.asParameter()
+      or
+      sp.getAGetCall().getTarget().getAParameter() = node.asParameter()
+    |
+      not exists(node.getLocation().getFile().getRelativePath())
+    )
+  }
 }
 
 query predicate problems(

cpp/common/src/codingstandards/cpp/rules/preventdeadlockbylockinginpredefinedorder/PreventDeadlockByLockingInPredefinedOrder.qll

@@ -24,7 +24,11 @@ predicate getAnOrderedLockPair(
   lock1 = node.coveredByLock() and
   lock2 = node.coveredByLock() and
   not lock1 = lock2 and
-  lock1.getEnclosingFunction() = lock2.getEnclosingFunction() and
+  exists(Function f |
+    lock1.getEnclosingFunction() = f and
+    lock2.getEnclosingFunction() = f and
+    node.getBasicBlock().getEnclosingFunction() = f
+  ) and
   exists(Location l1Loc, Location l2Loc |
     l1Loc = lock1.getLocation() and
     l2Loc = lock2.getLocation()

cpp/common/src/codingstandards/cpp/standardlibrary/String.qll

@@ -45,7 +45,8 @@ class StdBasicString extends ClassTemplateInstantiation {
   Type getConstIteratorType() {
     exists(TypedefType t |
       t.getDeclaringType() = this and
-      t.getName() = "const_iterator" and
+      // Certain compilers user __const_iterator instead of const_iterator.
+      t.getName() = ["const_iterator", "__const_iterator"] and
       result = t
     )
   }

cpp/common/test/includes/standard-library/array

@@ -1,7 +1,7 @@
 #ifndef _GHLIBCPP_ARRAY
 #define _GHLIBCPP_ARRAY
-#include "iterator.h"
-#include "string.h"
+#include <iterator>
+#include <string>
 
 // Note: a few features currently unused by tests are commented out
 namespace std {

cpp/common/test/includes/standard-library/cerrno

@@ -1,2 +1 @@
-int __errno;
-#define errno __errno
+#include <errno.h>