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.

Author: jbj

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
   }
 }
 

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 |

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
 	}
 }