Merge branch 'main' into lcartey/remove-old-is-excluded

Author: lcartey

c/misra/src/rules/RULE-20-8/ControllingExpressionIfDirective.ql

@@ -14,40 +14,35 @@
 
 import cpp
 import codingstandards.c.misra
+import codingstandards.cpp.PreprocessorDirective
 
 /*  A controlling expression is evaluated if it is not excluded (guarded by another controlling expression that is not taken). This translates to it either being taken or not taken.  */
 predicate isEvaluated(PreprocessorBranch b) { b.wasTaken() or b.wasNotTaken() }
 
-class IfOrElifPreprocessorBranch extends PreprocessorBranch {
-  IfOrElifPreprocessorBranch() {
-    this instanceof PreprocessorIf or this instanceof PreprocessorElif
-  }
-}
-
 /**
  * Looks like it contains a single macro, which may be undefined
  */
-class SimpleMacroPreprocessorBranch extends IfOrElifPreprocessorBranch {
+class SimpleMacroPreprocessorBranch extends PreprocessorIfOrElif {
   SimpleMacroPreprocessorBranch() { this.getHead().regexpMatch("[a-zA-Z_][a-zA-Z0-9_]+") }
 }
 
-class SimpleNumericPreprocessorBranch extends IfOrElifPreprocessorBranch {
+class SimpleNumericPreprocessorBranch extends PreprocessorIfOrElif {
   SimpleNumericPreprocessorBranch() { this.getHead().regexpMatch("[0-9]+") }
 }
 
 class ZeroOrOnePreprocessorBranch extends SimpleNumericPreprocessorBranch {
   ZeroOrOnePreprocessorBranch() { this.getHead().regexpMatch("[0|1]") }
 }
 
-predicate containsOnlySafeOperators(IfOrElifPreprocessorBranch b) {
+predicate containsOnlySafeOperators(PreprocessorIfOrElif b) {
   containsOnlyDefinedOperator(b)
   or
   //logic: comparison operators eval last, so they make it safe?
   b.getHead().regexpMatch(".*[\\&\\&|\\|\\||>|<|==].*")
 }
 
 //all defined operators is definitely safe
-predicate containsOnlyDefinedOperator(IfOrElifPreprocessorBranch b) {
+predicate containsOnlyDefinedOperator(PreprocessorIfOrElif b) {
   forall(string portion |
     portion =
       b.getHead()
@@ -65,7 +60,7 @@ class BinaryValuedMacro extends Macro {
   BinaryValuedMacro() { this.getBody().regexpMatch("\\(?(0|1)\\)?") }
 }
 
-from IfOrElifPreprocessorBranch b, string msg
+from PreprocessorIfOrElif b, string msg
 where
   not isExcluded(b, Preprocessor3Package::controllingExpressionIfDirectiveQuery()) and
   isEvaluated(b) and

change_notes/2023-12-06-m16-1-1-perf.md

@@ -1,4 +1,4 @@
- - `M16-1-1`:
+ - `M16-1-1` - `DefinedPreProcessorOperatorGeneratedFromExpansionFound.ql`:
    - Optimize query to improve performance
    - Improve detection of macros whose body contains the `defined` operator after the start of the macro (e.g. `#define X Y || defined(Z)`).
    - Enable exclusions to be applied for this rule.

change_notes/2024-01-30-fix-fp-for-a8-4-8.md

@@ -0,0 +1,7 @@
+- `A8-4-8` - `OutParametersUsed.ql`
+  - Fixes #370 - Non-member user-defined assignment operator and stream insertion/extraction parameters that are required to be out parameters are excluded.
+  - Broadens the definition of out parameter by considering assignment and crement operators as modifications to an out parameter candidate.
+- `FIO51-CPP` - `CloseFilesWhenTheyAreNoLongerNeeded.ql`:
+  - Broadened definition of `IStream` and `OStream` types may result in reduced false negatives.
+- `A5-1-1` - `LiteralValueUsedOutsideTypeInit.ql`:
+  - Broadened definition of `IStream` types may result in reduced false positives because more file stream function calls may be detected as logging operations that will be excluded from the results.

cpp/autosar/src/rules/A8-4-8/OutputParametersUsed.ql

@@ -23,31 +23,60 @@ import codingstandards.cpp.ConstHelpers
 import codingstandards.cpp.Operator
 
 /**
- * Non-const T& and T* `Parameter`s to `Function`s
+ * Holds if p is passed as a non-const reference or pointer and is modified.
+ * This holds for in-out or out-only parameters.
  */
-class NonConstReferenceOrPointerParameterCandidate extends FunctionParameter {
-  NonConstReferenceOrPointerParameterCandidate() {
-    this instanceof NonConstReferenceParameter
-    or
-    this instanceof NonConstPointerParameter
-  }
+predicate isOutParameter(NonConstPointerorReferenceParameter p) {
+  any(VariableEffect ve).getTarget() = p
+}
+
+/**
+ * Holds if parameter `p` is a parameter to a user defined assignment operator that
+ * is defined outside of a class body.
+ * These require an in-out parameter as the first argument.
+ */
+predicate isNonMemberUserAssignmentParameter(NonConstPointerorReferenceParameter p) {
+  p.getFunction() instanceof UserAssignmentOperator and
+  not p.isMember()
+}
+
+/**
+ * Holds if parameter `p` is a parameter to a stream insertion operator that
+ * is defined outside of a class body.
+ * These require an in-out parameter as the first argument.
+ *
+ * e.g., `std::ostream& operator<<(std::ostream& os, const T& obj)`
+ */
+predicate isStreamInsertionStreamParameter(NonConstPointerorReferenceParameter p) {
+  exists(StreamInsertionOperator op | not op.isMember() | op.getParameter(0) = p)
 }
 
-pragma[inline]
-predicate isFirstAccess(VariableAccess va) {
-  not exists(VariableAccess otherVa |
-    otherVa.getTarget() = va.getTarget() or
-    otherVa.getQualifier().(VariableAccess).getTarget() = va.getTarget()
-  |
-    otherVa.getASuccessor() = va
+/**
+ * Holds if parameter `p` is a parameter to a stream insertion operator that
+ * is defined outside of a class body.
+ * These require an in-out parameter as the first argument and an out parameter for the second.
+ *
+ * e.g., `std::istream& operator>>(std::istream& is, T& obj)`
+ */
+predicate isStreamExtractionParameter(NonConstPointerorReferenceParameter p) {
+  exists(StreamExtractionOperator op | not op.isMember() |
+    op.getParameter(0) = p
+    or
+    op.getParameter(1) = p
   )
 }
 
-from NonConstReferenceOrPointerParameterCandidate p, VariableEffect ve
+predicate isException(NonConstPointerorReferenceParameter p) {
+  isNonMemberUserAssignmentParameter(p) and p.getIndex() = 0
+  or
+  isStreamInsertionStreamParameter(p)
+  or
+  isStreamExtractionParameter(p)
+}
+
+from NonConstPointerorReferenceParameter p
 where
   not isExcluded(p, ConstPackage::outputParametersUsedQuery()) and
-  ve.getTarget() = p and
-  isFirstAccess(ve.getAnAccess()) and
-  not ve instanceof AnyAssignOperation and
-  not ve instanceof CrementOperation
-select p, "Out parameter " + p.getName() + " that is modified before being read."
+  isOutParameter(p) and
+  not isException(p)
+select p, "Out parameter '" + p.getName() + "' used."

cpp/autosar/src/rules/M16-1-1/DefinedPreProcessorOperatorGeneratedFromExpansionFound.ql

@@ -14,18 +14,9 @@
 
 import cpp
 import codingstandards.cpp.autosar
+import codingstandards.cpp.PreprocessorDirective
 import DefinedMacro
 
-/**
- * An `if` or `elif` preprocessor branch.
- */
-class PreprocessorIfOrElif extends PreprocessorBranch {
-  PreprocessorIfOrElif() {
-    this instanceof PreprocessorIf or
-    this instanceof PreprocessorElif
-  }
-}
-
 from PreprocessorIfOrElif e, MacroUsesDefined m
 where
   not isExcluded(e, MacrosPackage::definedPreProcessorOperatorInOneOfTheTwoStandardFormsQuery()) and

cpp/autosar/src/rules/M16-1-1/DefinedPreProcessorOperatorInOneOfTheTwoStandardForms.ql

@@ -15,6 +15,7 @@
 
 import cpp
 import codingstandards.cpp.autosar
+import codingstandards.cpp.PreprocessorDirective
 
 //get what comes after each 'defined' used with or without parenth
 string matchesDefinedOperator(PreprocessorBranch e) {
@@ -34,12 +35,8 @@ string matchesDefinedOperator(PreprocessorBranch e) {
   )
 }
 
-from PreprocessorBranch e, string arg
+from PreprocessorIfOrElif e, string arg
 where
-  (
-    e instanceof PreprocessorIf or
-    e instanceof PreprocessorElif
-  ) and
   arg = matchesDefinedOperator(e) and
   not arg.regexpMatch("^\\w*$") and
   not isExcluded(e, MacrosPackage::definedPreProcessorOperatorInOneOfTheTwoStandardFormsQuery())

cpp/autosar/test/rules/A8-4-8/OutputParametersUsed.expected

@@ -1,5 +1,6 @@
-| test.cpp:5:22:5:24 | str | Out parameter str that is modified before being read. |
-| test.cpp:16:14:16:14 | i | Out parameter i that is modified before being read. |
-| test.cpp:21:14:21:14 | i | Out parameter i that is modified before being read. |
-| test.cpp:29:12:29:12 | a | Out parameter a that is modified before being read. |
-| test.cpp:33:12:33:12 | a | Out parameter a that is modified before being read. |
+| test.cpp:5:22:5:24 | str | Out parameter 'str' used. |
+| test.cpp:8:22:8:24 | str | Out parameter 'str' used. |
+| test.cpp:16:14:16:14 | i | Out parameter 'i' used. |
+| test.cpp:21:14:21:14 | i | Out parameter 'i' used. |
+| test.cpp:29:12:29:12 | a | Out parameter 'a' used. |
+| test.cpp:33:12:33:12 | a | Out parameter 'a' used. |

cpp/autosar/test/rules/A8-4-8/test.cpp

@@ -5,7 +5,7 @@ void f(int &i) { // COMPLIANT
 void f1(std::string &str) { // NON_COMPLIANT
   str = "replacement";
 }
-void f2(std::string &str) { // COMPLIANT
+void f2(std::string &str) { // NON_COMPLIANT
   str += "suffix";
 }
 
@@ -37,3 +37,41 @@ void f7(A &a) { // NON_COMPLIANT
 void f8(int i) { // COMPLIANT
   i += 1;
 }
+
+constexpr A &operator|=(
+    A &lhs,
+    const A &rhs) noexcept { // COMPLIANT - non-member user defined assignment
+                             // operators are considered an exception.
+  return lhs;
+}
+
+enum class byte : unsigned char {};
+constexpr byte &
+operator|(const byte &lhs,
+          const byte &rhs) noexcept { // COMPLIANT - parameters are const
+                                      // qualified references
+  return lhs | rhs;
+}
+constexpr byte &operator|=(
+    byte &lhs,
+    const byte rhs) noexcept { // COMPLIANT - non-member user defined assignment
+                               // operators are considered an exception.
+  lhs = (lhs | rhs);
+  return lhs;
+}
+
+#include <iostream>
+std::ostream &operator<<(std::ostream &os,
+                         const byte &obj) { // COMPLIANT - insertion operators
+                                            // are considered an exception.
+  std::ostream other;
+  os = other; // simulate modification
+  return os;
+}
+
+std::istream &operator>>(std::istream &is,
+                         byte &obj) { // COMPLIANT - extraction operators are
+                                      // considered an exception.
+  obj = static_cast<byte>('a');       // simulate modification
+  return is;
+}

cpp/cert/src/rules/FIO51-CPP/CloseFilesWhenTheyAreNoLongerNeeded.ql

@@ -38,8 +38,10 @@ predicate filebufAccess(ControlFlowNode node, FileStreamSource fss) {
   //insertion or extraction operator calls
   node.(InsertionOperatorCall).getFStream() = fss.getAUse() or
   node.(ExtractionOperatorCall).getFStream() = fss.getAUse() or
-  //methods inherited from istream or ostream
-  node.(IOStreamFunctionCall).getFStream() = fss.getAUse()
+  // Methods inherited from istream or ostream that access the file stream.
+  // Exclude is_open as it is not a filebuf access
+  any(IOStreamFunctionCall call | node = call and not call.getTarget().hasName("is_open"))
+      .getFStream() = fss.getAUse()
 }
 
 /**

cpp/common/src/codingstandards/cpp/Operator.qll

@@ -1,5 +1,7 @@
 import cpp
 import Expr
+private import semmle.code.cpp.security.FileWrite
+private import codingstandards.cpp.standardlibrary.FileStreams
 
 /**
  * any assignment operator that also reads from the access
@@ -119,14 +121,16 @@ class UserAssignmentOperator extends AssignmentOperator {
 }
 
 /** An assignment operator of any sort */
-class AssignmentOperator extends MemberFunction {
+class AssignmentOperator extends Function {
   AssignmentOperator() {
     // operator op, where op is =, +=, -=, *=, /=, %=, ^=, &=, |=, >>=, <<=
     exists(string op |
       "operator" + op = this.getName() and
       op in ["=", "+=", "-=", "*=", "/=", "%=", "^=", "&=", "|=", ">>=", "<<="]
     )
   }
+
+  predicate isLValueRefQualified() { this.(MemberFunction).isLValueRefQualified() }
 }
 
 class UserComparisonOperator extends Function {
@@ -265,6 +269,38 @@ class UserOverloadedOperator extends Function {
   }
 }
 
+/** An implementation of a stream insertion operator. */
+class StreamInsertionOperator extends Function {
+  StreamInsertionOperator() {
+    this.hasName("operator<<") and
+    (
+      if this.isMember()
+      then this.getNumberOfParameters() = 1
+      else (
+        this.getNumberOfParameters() = 2 and
+        this.getParameter(0).getType() instanceof BasicOStreamClass
+      )
+    ) and
+    this.getType() instanceof BasicOStreamClass
+  }
+}
+
+/** An implementation of a stream extraction operator. */
+class StreamExtractionOperator extends Function {
+  StreamExtractionOperator() {
+    this.hasName("operator>>") and
+    (
+      if this.isMember()
+      then this.getNumberOfParameters() = 1
+      else (
+        this.getNumberOfParameters() = 2 and
+        this.getParameter(0).getType() instanceof IStream
+      )
+    ) and
+    this.getType() instanceof IStream
+  }
+}
+
 /** A user defined operator address of operator (`&`). */
 class UnaryAddressOfOperator extends Operator {
   UnaryAddressOfOperator() {

cpp/common/src/codingstandards/cpp/PreprocessorDirective.qll

@@ -30,3 +30,13 @@ PreprocessorDirective isLocatedInAMacroInvocation(MacroInvocation m) {
     result = p
   )
 }
+
+/**
+ * An `if` or `elif` preprocessor branch.
+ */
+class PreprocessorIfOrElif extends PreprocessorBranch {
+  PreprocessorIfOrElif() {
+    this instanceof PreprocessorIf or
+    this instanceof PreprocessorElif
+  }
+}

cpp/common/src/codingstandards/cpp/rules/undefinedmacroidentifiers/UndefinedMacroIdentifiers.qll

@@ -1,5 +1,6 @@
 import cpp
 import codingstandards.cpp.Exclusions
+import codingstandards.cpp.PreprocessorDirective
 
 abstract class UndefinedMacroIdentifiersSharedQuery extends Query { }
 
@@ -76,17 +77,10 @@ string getAnIfDefdMacroIdentifier(PreprocessorBranch pb) {
   )
 }
 
-class IfAndElifs extends PreprocessorBranch {
-  IfAndElifs() {
-    this instanceof PreprocessorIf or
-    this instanceof PreprocessorElif
-  }
-}
-
-class BadIfAndElifs extends IfAndElifs {
+class UndefinedIdIfOrElif extends PreprocessorIfOrElif {
   string undefinedMacroIdentifier;
 
-  BadIfAndElifs() {
+  UndefinedIdIfOrElif() {
     exists(string defRM |
       defRM =
         this.getHead()
@@ -113,7 +107,7 @@ class BadIfAndElifs extends IfAndElifs {
   string getAnUndefinedMacroIdentifier() { result = undefinedMacroIdentifier }
 }
 
-query predicate problems(BadIfAndElifs b, string message) {
+query predicate problems(UndefinedIdIfOrElif b, string message) {
   not isExcluded(b, getQuery()) and
   message =
     "#if/#elif that uses a macro identifier " + b.getAnUndefinedMacroIdentifier() +