From bebd5ae36beeb2c8158005945affa5930cca998f Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jbj@github.com>
Date: Mon, 11 May 2020 09:35:25 +0200
Subject: [PATCH] C++: Call qualifiers are passed by reference

After #3382 changed the escape analysis to model qualifiers as escaping,
there was an imbalance in the SSA library, where `addressTakenVariable`
excludes variables from SSA analysis if they have their address taken
but are _not_ passed by reference. This showed up as a missing result in
`TOCTOUFilesystemRace.ql`, demonstrated with a test case in #3432.

This commit changes the definition of "pass by reference" to include
call qualifiers, which allows SSA modeling of variables that have member
function calls on them.
---
 cpp/ql/src/semmle/code/cpp/exprs/Call.qll            | 12 ++++++++++--
 .../CWE/CWE-367/semmle/TOCTOUFilesystemRace.expected |  2 ++
 .../query-tests/Security/CWE/CWE-367/semmle/test.cpp |  4 ++--
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/exprs/Call.qll b/cpp/ql/src/semmle/code/cpp/exprs/Call.qll
index d22e090a9e62..abb26002091c 100644
--- a/cpp/ql/src/semmle/code/cpp/exprs/Call.qll
+++ b/cpp/ql/src/semmle/code/cpp/exprs/Call.qll
@@ -82,7 +82,8 @@ abstract class Call extends Expr, NameQualifiableElement {
 
   /**
    * Holds if this call passes the variable accessed by `va` by
-   * reference as the `i`th argument.
+   * reference as the `i`th argument. The qualifier of a call to a member
+   * function is `i = -1`.
    *
    * A variable is passed by reference if the `i`th parameter of the function
    * receives an address that points within the object denoted by `va`. For a
@@ -101,11 +102,15 @@ abstract class Call extends Expr, NameQualifiableElement {
    */
   predicate passesByReference(int i, VariableAccess va) {
     variableAddressEscapesTree(va, this.getArgument(i).getFullyConverted())
+    or
+    variableAddressEscapesTree(va, this.getQualifier().getFullyConverted()) and
+    i = -1
   }
 
   /**
    * Holds if this call passes the variable accessed by `va` by
-   * reference to non-const data as the `i`th argument.
+   * reference to non-const data as the `i`th argument. The qualifier of a
+   * call to a member function is `i = -1`.
    *
    * A variable is passed by reference if the `i`th parameter of the function
    * receives an address that points within the object denoted by `va`. For a
@@ -124,6 +129,9 @@ abstract class Call extends Expr, NameQualifiableElement {
    */
   predicate passesByReferenceNonConst(int i, VariableAccess va) {
     variableAddressEscapesTreeNonConst(va, this.getArgument(i).getFullyConverted())
+    or
+    variableAddressEscapesTreeNonConst(va, this.getQualifier().getFullyConverted()) and
+    i = -1
   }
 }
 
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/TOCTOUFilesystemRace.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/TOCTOUFilesystemRace.expected
index 4794d4744af8..f514742ff0ac 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/TOCTOUFilesystemRace.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/TOCTOUFilesystemRace.expected
@@ -1 +1,3 @@
 | test.cpp:21:3:21:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test.cpp:21:10:21:14 | file1 | filename | test.cpp:19:7:19:12 | call to rename | checked |
+| test.cpp:35:3:35:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test.cpp:35:10:35:14 | file1 | filename | test.cpp:32:7:32:12 | call to rename | checked |
+| test.cpp:49:3:49:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test.cpp:49:10:49:14 | file1 | filename | test.cpp:47:7:47:12 | call to rename | checked |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/test.cpp
index 6433523d69a0..b876146f5716 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/test.cpp
@@ -32,7 +32,7 @@ void test2()
 	if (!rename(file1, file2))
 	{
 		file1.set("d.txt");
-		remove(file1); // GOOD
+		remove(file1); // GOOD [FALSE POSITIVE]
 	}
 }
 
@@ -46,6 +46,6 @@ void test3()
 	create(file1);
 	if (!rename(file1, file2))
 	{
-		remove(file1); // BAD [NOT DETECTED]
+		remove(file1); // BAD
 	}
 }