C++: Respond to review comments. Relax the escaping requirements on the local variable being used in memset.

Author: MathiasVP

cpp/ql/src/Security/CWE/CWE-014/MemsetMayBeDeleted.ql

@@ -1,6 +1,6 @@
 /**
  * @name Call to `memset` may be deleted
- * @description Using <code>memset</code> the function to clear private data in a variable that has no subsequent use
+ * @description Using `memset` the function to clear private data in a variable that has no subsequent use
  *              can make information-leak vulnerabilities easier to exploit because the compiler can remove the call.
  * @kind problem
  * @id cpp/memset-may-be-deleted
@@ -13,6 +13,7 @@
 import cpp
 import semmle.code.cpp.dataflow.EscapesTree
 import semmle.code.cpp.commons.Exclusions
+import semmle.code.cpp.models.interfaces.Alias
 
 class MemsetFunction extends Function {
   MemsetFunction() {
@@ -24,22 +25,33 @@ class MemsetFunction extends Function {
   }
 }
 
+predicate isNonEscapingArgument(Expr escaped) {
+  exists(Call call, AliasFunction aliasFunction, int i |
+    aliasFunction = call.getTarget() and
+    call.getArgument(i) = escaped.getUnconverted() and
+    (
+      aliasFunction.parameterNeverEscapes(i)
+      or
+      aliasFunction.parameterEscapesOnlyViaReturn(i) and
+      (call instanceof ExprInVoidContext or call.getConversion*() instanceof BoolConversion)
+    )
+  )
+}
+
 from FunctionCall call, LocalVariable v, MemsetFunction memset
 where
   call.getTarget() = memset and
   not isFromMacroDefinition(call) and
-  // `v` only escapes as the argument to `memset`.
+  // `v` escapes as the argument to `memset`
+  variableAddressEscapesTree(v.getAnAccess(), call.getArgument(0).getFullyConverted()) and
+  // ... and `v` doesn't escape anywhere else.
   forall(Expr escape | variableAddressEscapesTree(v.getAnAccess(), escape) |
-    call.getArgument(0) = escape.getUnconverted()
+    isNonEscapingArgument(escape)
   ) and
-  // `v` is a stack-allocated array or a struct, and `v` is not static.
   not v.isStatic() and
-  (
-    v.getUnspecifiedType() instanceof ArrayType and call.getArgument(0) = v.getAnAccess()
-    or
-    v.getUnspecifiedType() instanceof Struct and
-    call.getArgument(0).(AddressOfExpr).getAddressable() = v
-  ) and
+  // Reference-typed variables get special treatment in `variableAddressEscapesTree` so we leave them
+  // out of this query.
+  not v.getUnspecifiedType() instanceof ReferenceType and
   // There is no later use of `v`.
   not v.getAnAccess() = call.getASuccessor*() and
   // Not using the `-fno-builtin-memset` flag