Fix #370

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

@@ -0,0 +1,3 @@
+- `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.

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,38 @@ 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 {
+  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/common/src/codingstandards/cpp/Operator.qll

@@ -1,5 +1,6 @@
 import cpp
 import Expr
+private import semmle.code.cpp.security.FileWrite
 
 /**
  * any assignment operator that also reads from the access
@@ -119,14 +120,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 {
@@ -264,3 +267,51 @@ class UserOverloadedOperator extends Function {
     not this.isCompilerGenerated()
   }
 }
+
+/**
+ * A `std::basic_istream` class, or something that can be used
+ * as one. Based on the BasicOStreamClass.
+ */
+private class BasicIStreamClass extends Type {
+  BasicIStreamClass() {
+    this.(Class).getName().matches("basic\\_istream%")
+    or
+    this.getUnspecifiedType() instanceof BasicIStreamClass
+    or
+    this.(Class).getABaseClass() instanceof BasicIStreamClass
+    or
+    this.(ReferenceType).getBaseType() instanceof BasicIStreamClass
+  }
+}
+
+/** 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 BasicIStreamClass
+      )
+    ) and
+    this.getType() instanceof BasicIStreamClass
+  }
+}