From 8ffa124bf96ada8fa410a901d6f0b9af351d3152 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 28 Apr 2020 16:32:31 +0200 Subject: [PATCH 1/3] C++: Addresses may escape through call qualifiers Also clarify the docs on `Call` to decrease the likelyhood of such an omission happening again. The updated test reflects that `f1.operator()` lets the address of `f1` escape from the caller. --- .../semmle/code/cpp/dataflow/EscapesTree.qll | 35 ++++++++++++++++--- cpp/ql/src/semmle/code/cpp/exprs/Call.qll | 16 +++++---- .../defuse/isAddressOfAccess.expected | 4 +-- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll b/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll index 8a99a71f6afe..3896011a2755 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll @@ -166,10 +166,13 @@ private predicate referenceFromVariableAccess(VariableAccess va, Expr reference) ) } -private predicate valueMayEscapeAt(Expr e) { +private predicate addressMayEscapeAt(Expr e) { exists(Call call | e = call.getAnArgument().getFullyConverted() and not stdIdentityFunction(call.getTarget()) + or + e = call.getQualifier().getFullyConverted() and + e.getUnderlyingType() instanceof PointerType ) or exists(AssignExpr assign | e = assign.getRValue().getFullyConverted()) @@ -187,8 +190,8 @@ private predicate valueMayEscapeAt(Expr e) { exists(AsmStmt asm | e = asm.getAChild().(Expr).getFullyConverted()) } -private predicate valueMayEscapeMutablyAt(Expr e) { - valueMayEscapeAt(e) and +private predicate addressMayEscapeMutablyAt(Expr e) { + addressMayEscapeAt(e) and exists(Type t | t = e.getType().getUnderlyingType() | exists(PointerType pt | pt = t @@ -207,6 +210,22 @@ private predicate valueMayEscapeMutablyAt(Expr e) { ) } +private predicate lvalueMayEscapeAt(Expr e) { + // A call qualifier, like `q` in `q.f()`, is special in that the address of + // `q` escapes even though `q` is not a pointer or a reference. + exists(Call call | + e = call.getQualifier().getFullyConverted() and + e.getType().getUnspecifiedType() instanceof Class + ) +} + +private predicate lvalueMayEscapeMutablyAt(Expr e) { + lvalueMayEscapeAt(e) and + // A qualifier of a call to a const member function is converted to a const + // class type. + not e.isConstant() +} + private predicate addressFromVariableAccess(VariableAccess va, Expr e) { pointerFromVariableAccess(va, e) or @@ -253,8 +272,11 @@ private module EscapesTree_Cached { */ cached predicate variableAddressEscapesTree(VariableAccess va, Expr e) { - valueMayEscapeAt(e) and + addressMayEscapeAt(e) and addressFromVariableAccess(va, e) + or + lvalueMayEscapeAt(e) and + lvalueFromVariableAccess(va, e) } /** @@ -283,8 +305,11 @@ private module EscapesTree_Cached { */ cached predicate variableAddressEscapesTreeNonConst(VariableAccess va, Expr e) { - valueMayEscapeMutablyAt(e) and + addressMayEscapeMutablyAt(e) and addressFromVariableAccess(va, e) + or + lvalueMayEscapeMutablyAt(e) and + lvalueFromVariableAccess(va, e) } /** diff --git a/cpp/ql/src/semmle/code/cpp/exprs/Call.qll b/cpp/ql/src/semmle/code/cpp/exprs/Call.qll index 6eb4ed523195..d22e090a9e62 100644 --- a/cpp/ql/src/semmle/code/cpp/exprs/Call.qll +++ b/cpp/ql/src/semmle/code/cpp/exprs/Call.qll @@ -9,9 +9,8 @@ private import semmle.code.cpp.dataflow.EscapesTree */ abstract class Call extends Expr, NameQualifiableElement { /** - * Gets the number of actual parameters in this call; use - * `getArgument(i)` with `i` between `0` and `result - 1` to - * retrieve actuals. + * Gets the number of arguments (actual parameters) of this call. The count + * does _not_ include the qualifier of the call, if any. */ int getNumberOfArguments() { result = count(this.getAnArgument()) } @@ -32,21 +31,24 @@ abstract class Call extends Expr, NameQualifiableElement { Expr getQualifier() { result = this.getChild(-1) } /** - * Gets an argument for this call. + * Gets an argument for this call. To get the qualifier of this call, if + * any, use `getQualifier()`. */ Expr getAnArgument() { exists(int i | result = this.getChild(i) and i >= 0) } /** * Gets the nth argument for this call. * - * The range of `n` is from `0` to `getNumberOfArguments() - 1`. + * The range of `n` is from `0` to `getNumberOfArguments() - 1`. To get the + * qualifier of this call, if any, use `getQualifier()`. */ Expr getArgument(int n) { result = this.getChild(n) and n >= 0 } /** - * Gets a sub expression of the argument at position `index`. If the + * Gets a subexpression of the argument at position `index`. If the * argument itself contains calls, such calls will be considered - * leafs in the expression tree. + * leaves in the expression tree. The qualifier of the call, if any, is not + * considered to be an argument. * * Example: the call `f(2, 3 + 4, g(4 + 5))` has sub expression(s) * `2` at index 0; `3`, `4`, and `3 + 4` at index 1; and `g(4 + 5)` diff --git a/cpp/ql/test/library-tests/defuse/isAddressOfAccess.expected b/cpp/ql/test/library-tests/defuse/isAddressOfAccess.expected index ff7ee26843d5..67b611b868cf 100644 --- a/cpp/ql/test/library-tests/defuse/isAddressOfAccess.expected +++ b/cpp/ql/test/library-tests/defuse/isAddressOfAccess.expected @@ -11,9 +11,9 @@ | addressOf.cpp:38:20:38:20 | i | non-const address | | addressOf.cpp:40:15:40:15 | i | non-const address | | addressOf.cpp:42:19:42:22 | iref | non-const address | -| addressOf.cpp:48:3:48:4 | f1 | | +| addressOf.cpp:48:3:48:4 | f1 | non-const address | | addressOf.cpp:49:15:49:22 | captured | non-const address | -| addressOf.cpp:50:3:50:4 | f2 | | +| addressOf.cpp:50:3:50:4 | f2 | non-const address | | addressOf.cpp:51:10:51:17 | captured | | | addressOf.cpp:56:16:56:16 | i | | | addressOf.cpp:56:19:56:19 | i | | From 9fc27e913028418ab47e5c23449e450443665023 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 1 May 2020 09:04:31 +0200 Subject: [PATCH 2/3] C++: Fix "is constant" check The check was supposed to check for constant type, not constant value. This fixes a false negative that appeared in `LargeParameter/test.cpp:106`. --- cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll b/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll index 3896011a2755..22e39f0b9075 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll @@ -223,7 +223,7 @@ private predicate lvalueMayEscapeMutablyAt(Expr e) { lvalueMayEscapeAt(e) and // A qualifier of a call to a const member function is converted to a const // class type. - not e.isConstant() + not e.getType().isConst() } private predicate addressFromVariableAccess(VariableAccess va, Expr e) { From 9b9f5248af1613b4e4f9f622566eadc59baa8c6b Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 1 May 2020 15:23:00 +0200 Subject: [PATCH 3/3] C++: Accept test changes Lambda invocations are apparently const. This was exposed by the fix in the previous commit. --- cpp/ql/test/library-tests/defuse/isAddressOfAccess.expected | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/test/library-tests/defuse/isAddressOfAccess.expected b/cpp/ql/test/library-tests/defuse/isAddressOfAccess.expected index 67b611b868cf..8a57ff7206e0 100644 --- a/cpp/ql/test/library-tests/defuse/isAddressOfAccess.expected +++ b/cpp/ql/test/library-tests/defuse/isAddressOfAccess.expected @@ -11,9 +11,9 @@ | addressOf.cpp:38:20:38:20 | i | non-const address | | addressOf.cpp:40:15:40:15 | i | non-const address | | addressOf.cpp:42:19:42:22 | iref | non-const address | -| addressOf.cpp:48:3:48:4 | f1 | non-const address | +| addressOf.cpp:48:3:48:4 | f1 | const address | | addressOf.cpp:49:15:49:22 | captured | non-const address | -| addressOf.cpp:50:3:50:4 | f2 | non-const address | +| addressOf.cpp:50:3:50:4 | f2 | const address | | addressOf.cpp:51:10:51:17 | captured | | | addressOf.cpp:56:16:56:16 | i | | | addressOf.cpp:56:19:56:19 | i | |