Merge pull request #2599 from MathiasVP/assign-where-compare-meant-false-positives

Assign where compare meant false positives

cpp/ql/src/Likely Bugs/Likely Typos/AssignWhereCompareMeant.ql

@@ -38,6 +38,12 @@ abstract class BooleanControllingAssignment extends AssignExpr {
   abstract predicate isWhitelisted();
 }
 
+/**
+ * Gets an operand of a logical operation expression (we need the restriction
+ * to BinaryLogicalOperation expressions to get the correct transitive closure).
+ */
+Expr getComparisonOperand(BinaryLogicalOperation op) { result = op.getAnOperand() }
+
 class BooleanControllingAssignmentInExpr extends BooleanControllingAssignment {
   BooleanControllingAssignmentInExpr() {
     this.getParent() instanceof UnaryLogicalOperation or
@@ -45,7 +51,18 @@ class BooleanControllingAssignmentInExpr extends BooleanControllingAssignment {
     exists(ConditionalExpr c | c.getCondition() = this)
   }
 
-  override predicate isWhitelisted() { this.getConversion().(ParenthesisExpr).isParenthesised() }
+  override predicate isWhitelisted() {
+    this.getConversion().(ParenthesisExpr).isParenthesised()
+    or
+    // whitelist this assignment if all comparison operations in the expression that this
+    // assignment is part of, are not parenthesized. In that case it seems like programmer
+    // is fine with unparenthesized comparison operands to binary logical operators, and
+    // the parenthesis around this assignment was used to call it out as an assignment.
+    this.isParenthesised() and
+    forex(ComparisonOperation op | op = getComparisonOperand*(this.getParent+()) |
+      not op.isParenthesised()
+    )
+  }
 }
 
 class BooleanControllingAssignmentInStmt extends BooleanControllingAssignment {
@@ -65,7 +82,8 @@ class BooleanControllingAssignmentInStmt extends BooleanControllingAssignment {
  */
 predicate candidateResult(BooleanControllingAssignment ae) {
   ae.getRValue().isConstant() and
-  not ae.isWhitelisted()
+  not ae.isWhitelisted() and
+  not ae.getRValue() instanceof StringLiteral
 }
 
 /**
@@ -81,5 +99,6 @@ predicate candidateVariable(Variable v) {
 from BooleanControllingAssignment ae, UndefReachability undef
 where
   candidateResult(ae) and
+  not ae.isFromUninstantiatedTemplate(_) and
   not undef.reaches(_, ae.getLValue().(VariableAccess).getTarget(), ae.getLValue())
 select ae, "Use of '=' where '==' may have been intended."

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AssignWhereCompareMeant/AssignWhereCompareMeant.expected

@@ -13,3 +13,9 @@
 | test.cpp:82:7:82:11 | ... = ... | Use of '=' where '==' may have been intended. |
 | test.cpp:84:7:84:11 | ... = ... | Use of '=' where '==' may have been intended. |
 | test.cpp:92:17:92:22 | ... = ... | Use of '=' where '==' may have been intended. |
+| test.cpp:113:6:113:10 | ... = ... | Use of '=' where '==' may have been intended. |
+| test.cpp:134:19:134:23 | ... = ... | Use of '=' where '==' may have been intended. |
+| test.cpp:138:21:138:25 | ... = ... | Use of '=' where '==' may have been intended. |
+| test.cpp:144:32:144:36 | ... = ... | Use of '=' where '==' may have been intended. |
+| test.cpp:150:32:150:36 | ... = ... | Use of '=' where '==' may have been intended. |
+| test.cpp:153:46:153:50 | ... = ... | Use of '=' where '==' may have been intended. |

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/AssignWhereCompareMeant/test.cpp

@@ -98,3 +98,58 @@ void g(int *i_p, int cond) {
   if (y = 1) { // GOOD: y might not be initialized so it is probably intentional.
   }
 }
+
+template<typename>
+void h() {
+  int x;
+  if(x = 0) { // GOOD: x is not initialized so this is probably intentional
+  }
+
+  int y = 0;
+  if((y = 1)) { // GOOD
+  }
+
+  int z = 0;
+  if(z = 1) { // BAD
+  }
+}
+
+void f() {
+  h<int>();
+}
+
+void f2() {
+  const char* sz = "abc";
+
+  if(sz = "def") { // GOOD: a == comparison with a string literal is probably not the intent here
+  }
+}
+
+void f3(int x, int y) {
+  if(x == 1 && (y = 2)) { // GOOD: the programmer seems to be okay with unparenthesized
+                          // comparison operands, so the parenthesis was used to mark this
+                          // as an assignment
+  }
+
+  if((x == 1) && (y = 2)) { // BAD
+  }
+
+  long z = x;
+  if(((z == 42) || (y = 2)) && (x == 1)) { // BAD
+  }
+
+  if((y = 2) && (x == z || x == 1)) { // GOOD
+  }
+
+  if(((x == 42) || x == 1) && (y = 2)) { // BAD
+  }
+
+  if(x == 10 || (x == 42 && x == 1) && (y = 2)) { // GOOD
+  }
+
+  if(x == 10 || ((x == 42) && (y = 2)) && (z == 1)) { // BAD
+  }
+
+  if((x == 10) || ((z == z) && (x == 1)) && (y = 2)) { // BAD
+  }
+}