From 878cdf7cb67e3c51f0f07095aef17980aad36ffc Mon Sep 17 00:00:00 2001 From: Dave Bartolomeo Date: Thu, 11 Apr 2019 01:56:22 -0700 Subject: [PATCH] C++: Fix false positive in PointlessComparison 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) --- .../semmle/code/cpp/dataflow/EscapesTree.qll | 4 ++++ .../PointlessComparison/PointlessComparison.c | 22 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll b/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll index 4f9474e2efc6..911b796077b1 100644 --- 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 index 1ab2983645e8..f1d086482242 100644 --- 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; +} \ No newline at end of file