Merge pull request github#515 from rvermeulen/rvermeulen/fix-issue-#311

Fix github#370

Author: rvermeulen

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/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/standardlibrary/FileStreams.qll

@@ -12,6 +12,7 @@
 import cpp
 import codingstandards.cpp.dataflow.DataFlow
 import codingstandards.cpp.dataflow.TaintTracking
+private import codingstandards.cpp.Operator
 
 /**
  * A `basic_fstream` like `std::fstream`
@@ -23,15 +24,31 @@ class FileStream extends ClassTemplateInstantiation {
 /**
  * A `basic_istream` like `std::istream`
  */
-class IStream extends ClassTemplateInstantiation {
-  IStream() { this.getTemplate().hasQualifiedName("std", "basic_istream") }
+class IStream extends Type {
+  IStream() {
+    this.(Class).getQualifiedName().matches("std::basic\\_istream%")
+    or
+    this.getUnspecifiedType() instanceof IStream
+    or
+    this.(Class).getABaseClass() instanceof IStream
+    or
+    this.(ReferenceType).getBaseType() instanceof IStream
+  }
 }
 
 /**
  * A `basic_ostream` like `std::ostream`
  */
-class OStream extends ClassTemplateInstantiation {
-  OStream() { this.getTemplate().hasQualifiedName("std", "basic_ostream") }
+class OStream extends Type {
+  OStream() {
+    this.(Class).getQualifiedName().matches("std::basic\\_ostream%")
+    or
+    this.getUnspecifiedType() instanceof OStream
+    or
+    this.(Class).getABaseClass() instanceof OStream
+    or
+    this.(ReferenceType).getBaseType() instanceof OStream
+  }
 }
 
 /**
@@ -53,7 +70,7 @@ predicate sameStreamSource(FileStreamFunctionCall a, FileStreamFunctionCall b) {
  * Insertion `operator<<` and Extraction `operator>>` operators.
  */
 class InsertionOperatorCall extends FileStreamFunctionCall {
-  InsertionOperatorCall() { this.getTarget().(Operator).hasQualifiedName("std", "operator<<") }
+  InsertionOperatorCall() { this.getTarget() instanceof StreamInsertionOperator }
 
   override Expr getFStream() {
     result = this.getQualifier()
@@ -63,7 +80,7 @@ class InsertionOperatorCall extends FileStreamFunctionCall {
 }
 
 class ExtractionOperatorCall extends FileStreamFunctionCall {
-  ExtractionOperatorCall() { this.getTarget().(Operator).hasQualifiedName("std", "operator>>") }
+  ExtractionOperatorCall() { this.getTarget() instanceof StreamExtractionOperator }
 
   override Expr getFStream() {
     result = this.getQualifier()