C++: Fix false positive in PointlessComparison

dave-bartolomeo · dave-bartolomeo · commit 878cdf7cb67e · 2019-04-11T01:56:22.000-07:00
We avoid putting a variable into SSA if its address is ever taken in a way that could allow mutation of the variable via indirection. We currently just look to see if the address is either "pointer to non-const" or "reference to non-const". However, if the address was cast to an integral type (e.g. `uintptr_t n = (uintptr_t)&x;`), we were treating it as unescaped. This change makes the conservative assumption that casting a pointer to an integer may result in the pointed-to value being modified later. This fixes a customer-reported false positive (#2 from https://discuss.lgtm.com/t/2-false-positives-in-c-for-comparison-is-always-same/1943)
diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll b/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll
@@ -198,6 +198,10 @@ private predicate valueMayEscapeMutablyAt(Expr e) {
     or
     t instanceof ReferenceType and
     not t.(ReferenceType).getBaseType().isConst()
+    or
+    // If the address has been cast to an integral type, conservatively assume that it may eventually be cast back to a
+    // pointer to non-const type.
+    t instanceof IntegralType
   )
 }
 
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.c b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.c
@@ -342,3 +342,25 @@ int nan2(double x) {
     }
   }
 }
+
+struct info_t {
+  int id;
+  unsigned long long value;
+};
+
+int command(void* p, unsigned int s);
+
+int callCommand(void)
+{
+  struct info_t info;
+  unsigned int tmp = 0;
+
+  info.id = 1;
+  info.value = (unsigned long long)& tmp;
+  if (command(&info, sizeof(info))) {
+    return 0;
+  }
+  if (tmp == 1)  // tmp could have been modified by the call.
+    return 1;
+  return 0;
+}

Original file line number	Diff line number	Diff line change
`@@ -198,6 +198,10 @@ private predicate valueMayEscapeMutablyAt(Expr e) {`
`198`	`198`	`or`
`199`	`199`	`t instanceof ReferenceType and`
`200`	`200`	`not t.(ReferenceType).getBaseType().isConst()`
	`201`	`+ or`
	`202`	`+ // If the address has been cast to an integral type, conservatively assume that it may eventually be cast back to a`
	`203`	`+ // pointer to non-const type.`
	`204`	`+ t instanceof IntegralType`
`201`	`205`	`)`
`202`	`206`	`}`
`203`	`207`