From d77092c93160112a0aee7b940f3e02c143088405 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 29 May 2020 09:25:32 +0100
Subject: [PATCH 1/7] C++: Add taint tests for strlen.

---
 .../dataflow/taint-tests/format.cpp           | 21 ++++++++++++++++
 .../dataflow/taint-tests/localTaint.expected  | 25 +++++++++++++++++++
 .../dataflow/taint-tests/taint.expected       |  5 ++++
 .../dataflow/taint-tests/test_ir.expected     |  6 +++++
 4 files changed, 57 insertions(+)

diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp
index 974ea6040e8c..45b5d46daa7d 100644
--- a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp
+++ b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp
@@ -136,3 +136,24 @@ void test1()
 		sink(buffer); // tainted [NOT DETECTED]
 	}
 }
+
+// ----------
+
+size_t strlen(const char *s);
+size_t wcslen(const wchar_t *s);
+
+void test2()
+{
+	char *s = string::source();
+	wchar_t *ws = wstring::source();
+	int i;
+
+	sink(strlen(s)); // [FALSE POSITIVE]
+	sink(wcslen(ws)); // [FALSE POSITIVE]
+
+	i = strlen(s) + 1;
+	sink(i); // [FALSE POSITIVE]
+
+	sink(s[strlen(s) - 1]); // tainted
+	sink(ws + (wcslen(ws) / 2)); // tainted
+}
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
index c7d58f99966f..4b3b4b6029d2 100644
--- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
+++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
@@ -111,6 +111,31 @@
 | format.cpp:135:39:135:45 | ref arg & ... | format.cpp:135:40:135:45 | buffer [inner post update] |  |
 | format.cpp:135:39:135:45 | ref arg & ... | format.cpp:136:8:136:13 | buffer |  |
 | format.cpp:135:40:135:45 | buffer | format.cpp:135:39:135:45 | & ... |  |
+| format.cpp:147:12:147:25 | call to source | format.cpp:151:14:151:14 | s |  |
+| format.cpp:147:12:147:25 | call to source | format.cpp:154:13:154:13 | s |  |
+| format.cpp:147:12:147:25 | call to source | format.cpp:157:7:157:7 | s |  |
+| format.cpp:147:12:147:25 | call to source | format.cpp:157:16:157:16 | s |  |
+| format.cpp:148:16:148:30 | call to source | format.cpp:152:14:152:15 | ws |  |
+| format.cpp:148:16:148:30 | call to source | format.cpp:158:7:158:8 | ws |  |
+| format.cpp:148:16:148:30 | call to source | format.cpp:158:20:158:21 | ws |  |
+| format.cpp:151:14:151:14 | s | format.cpp:151:7:151:12 | call to strlen | TAINT |
+| format.cpp:152:14:152:15 | ws | format.cpp:152:7:152:12 | call to wcslen | TAINT |
+| format.cpp:154:6:154:11 | call to strlen | format.cpp:154:6:154:18 | ... + ... | TAINT |
+| format.cpp:154:6:154:18 | ... + ... | format.cpp:154:2:154:18 | ... = ... |  |
+| format.cpp:154:6:154:18 | ... + ... | format.cpp:155:7:155:7 | i |  |
+| format.cpp:154:13:154:13 | s | format.cpp:154:6:154:11 | call to strlen | TAINT |
+| format.cpp:154:18:154:18 | 1 | format.cpp:154:6:154:18 | ... + ... | TAINT |
+| format.cpp:157:7:157:7 | s | format.cpp:157:7:157:22 | access to array | TAINT |
+| format.cpp:157:9:157:14 | call to strlen | format.cpp:157:9:157:21 | ... - ... | TAINT |
+| format.cpp:157:9:157:21 | ... - ... | format.cpp:157:7:157:22 | access to array | TAINT |
+| format.cpp:157:16:157:16 | s | format.cpp:157:9:157:14 | call to strlen | TAINT |
+| format.cpp:157:21:157:21 | 1 | format.cpp:157:9:157:21 | ... - ... | TAINT |
+| format.cpp:158:7:158:8 | ws | format.cpp:158:7:158:27 | ... + ... | TAINT |
+| format.cpp:158:7:158:27 | ref arg ... + ... | format.cpp:158:7:158:8 | ws [inner post update] |  |
+| format.cpp:158:13:158:18 | call to wcslen | format.cpp:158:13:158:26 | ... / ... | TAINT |
+| format.cpp:158:13:158:26 | ... / ... | format.cpp:158:7:158:27 | ... + ... | TAINT |
+| format.cpp:158:20:158:21 | ws | format.cpp:158:13:158:18 | call to wcslen | TAINT |
+| format.cpp:158:26:158:26 | 2 | format.cpp:158:13:158:26 | ... / ... | TAINT |
 | stl.cpp:67:12:67:17 | call to source | stl.cpp:71:7:71:7 | a |  |
 | stl.cpp:68:16:68:20 | 123 | stl.cpp:68:16:68:21 | call to basic_string | TAINT |
 | stl.cpp:68:16:68:21 | call to basic_string | stl.cpp:72:7:72:7 | b |  |
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected
index e873ff919152..2228c5436c0a 100644
--- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected
+++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected
@@ -8,6 +8,11 @@
 | format.cpp:100:8:100:13 | buffer | format.cpp:99:30:99:43 | call to source |
 | format.cpp:105:8:105:13 | buffer | format.cpp:104:31:104:45 | call to source |
 | format.cpp:110:8:110:14 | wbuffer | format.cpp:109:38:109:52 | call to source |
+| format.cpp:151:7:151:12 | call to strlen | format.cpp:147:12:147:25 | call to source |
+| format.cpp:152:7:152:12 | call to wcslen | format.cpp:148:16:148:30 | call to source |
+| format.cpp:155:7:155:7 | i | format.cpp:147:12:147:25 | call to source |
+| format.cpp:157:7:157:22 | access to array | format.cpp:147:12:147:25 | call to source |
+| format.cpp:158:7:158:27 | ... + ... | format.cpp:148:16:148:30 | call to source |
 | stl.cpp:71:7:71:7 | a | stl.cpp:67:12:67:17 | call to source |
 | stl.cpp:73:7:73:7 | c | stl.cpp:69:16:69:21 | call to source |
 | stl.cpp:75:9:75:13 | call to c_str | stl.cpp:69:16:69:21 | call to source |
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected
index 63ab09a46823..7e3f17322dd9 100644
--- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected
+++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected
@@ -1,3 +1,9 @@
+| format.cpp:151:7:151:12 | call to strlen | format.cpp:147:12:147:25 | call to source |
+| format.cpp:152:7:152:12 | call to wcslen | format.cpp:148:16:148:30 | call to source |
+| format.cpp:155:7:155:7 | i | format.cpp:147:12:147:25 | call to source |
+| format.cpp:157:7:157:22 | (int)... | format.cpp:147:12:147:25 | call to source |
+| format.cpp:157:7:157:22 | access to array | format.cpp:147:12:147:25 | call to source |
+| format.cpp:158:7:158:27 | ... + ... | format.cpp:148:16:148:30 | call to source |
 | stl.cpp:71:7:71:7 | (const char *)... | stl.cpp:67:12:67:17 | call to source |
 | stl.cpp:71:7:71:7 | a | stl.cpp:67:12:67:17 | call to source |
 | taint.cpp:8:8:8:13 | clean1 | taint.cpp:4:27:4:33 | source1 |

From 408e38a4d457c6975a79bb7dd3031ff56dd871ba Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 May 2020 15:55:23 +0100
Subject: [PATCH 2/7] C++: Clarify which taint tracking libraries should be
 used somewhat.

---
 cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll     | 2 ++
 cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll b/cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll
index e20dfd83efde..65836d285adc 100644
--- a/cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll
@@ -1,5 +1,7 @@
 /*
  * Support for tracking tainted data through the program.
+ *
+ * Prefer to use `semmle.code.cpp.dataflow.TaintTracking` when designing new queries.
  */
 
 import semmle.code.cpp.ir.dataflow.DefaultTaintTracking
diff --git a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll
index a24820b277f9..06cf4c456ce1 100644
--- a/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll
+++ b/cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll
@@ -1,4 +1,8 @@
 /**
+ * DEPRECATED: we now use `semmle.code.cpp.ir.dataflow.DefaultTaintTracking`,
+ * which is based on the IR but designed to behave similarly to this old
+ * libarary.
+ *
  * Provides the implementation of `semmle.code.cpp.security.TaintTracking`. Do
  * not import this file directly.
  */

From 59cb5f9b1e351d6cea7e3e61ab7eff963ef89b58 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 May 2020 18:38:46 +0100
Subject: [PATCH 3/7] C++: Remove a special case for strlen in
 DefaultTaintTracking.

---
 .../src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll  | 4 +---
 .../dataflow/security-taint/tainted_diff.expected             | 4 ++++
 .../library-tests/dataflow/security-taint/tainted_ir.expected | 4 ++++
 .../CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected     | 3 +++
 .../CWE-190/semmle/tainted/IntegerOverflowTainted.expected    | 3 +++
 5 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
index d13a6b58d830..75b8641d449b 100644
--- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll
@@ -33,8 +33,7 @@ private predicate predictableInstruction(Instruction instr) {
  * Note that the list itself is not very principled; it consists of all the
  * functions listed in the old security library's [default] `isPureFunction`
  * that have more than one argument, but are not in the old taint tracking
- * library's `returnArgument` predicate.  In addition, `strlen` is included
- * because it's also a special case in flow to return values.
+ * library's `returnArgument` predicate.
  */
 predicate predictableOnlyFlow(string name) {
   name = "strcasestr" or
@@ -43,7 +42,6 @@ predicate predictableOnlyFlow(string name) {
   name = "strchrnul" or
   name = "strcmp" or
   name = "strcspn" or
-  name = "strlen" or // special case
   name = "strncmp" or
   name = "strndup" or
   name = "strnlen" or
diff --git a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected b/cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected
index 26441b08ba49..27573d98e768 100644
--- a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected
+++ b/cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected
@@ -3,6 +3,10 @@
 | test.cpp:49:23:49:28 | call to getenv | test.cpp:50:29:50:40 | envStrGlobal | AST only |
 | test.cpp:49:23:49:28 | call to getenv | test.cpp:52:2:52:12 | * ... | AST only |
 | test.cpp:49:23:49:28 | call to getenv | test.cpp:52:3:52:12 | envStr_ptr | AST only |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:10:64:14 | bytes | IR only |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:23 | call to strlen | IR only |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | (int)... | IR only |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | ... + ... | IR only |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:11:20:11:21 | s1 | AST only |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:67:7:67:13 | copying | AST only |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:69:10:69:13 | copy | AST only |
diff --git a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected b/cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected
index 50c90bccb2b4..97e713fbfbef 100644
--- a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected
+++ b/cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected
@@ -29,6 +29,10 @@
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:60:18:60:25 | userName |  |
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:60:29:60:34 | call to getenv |  |
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:60:29:60:47 | (const char *)... |  |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:10:64:14 | bytes |  |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:23 | call to strlen |  |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | (int)... |  |
+| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | ... + ... |  |
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:64:25:64:32 | userName |  |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:11:36:11:37 | s2 |  |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:68:17:68:24 | userName |  |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected
index b74dd08dd76e..131283c1c557 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected
@@ -8,3 +8,6 @@
 | test.c:14:15:14:28 | maxConnections | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:11:29:11:32 | argv | User-provided value |
 | test.c:44:7:44:10 | len2 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:41:17:41:20 | argv | User-provided value |
 | test.c:54:7:54:10 | len3 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:51:17:51:20 | argv | User-provided value |
+| test.c:74:7:74:10 | len5 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:71:19:71:22 | argv | User-provided value |
+| test.c:84:7:84:10 | len6 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:81:19:81:22 | argv | User-provided value |
+| test.c:94:7:94:10 | len7 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:91:19:91:22 | argv | User-provided value |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected
index d2b6499d40e2..926d4130d142 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected
@@ -4,3 +4,6 @@
 | test5.cpp:10:9:10:15 | call to strtoul | $@ flows to here and is used in an expression which might overflow. | test5.cpp:9:7:9:9 | buf | User-provided value |
 | test.c:44:7:44:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:41:17:41:20 | argv | User-provided value |
 | test.c:54:7:54:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:51:17:51:20 | argv | User-provided value |
+| test.c:74:7:74:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:71:19:71:22 | argv | User-provided value |
+| test.c:84:7:84:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:81:19:81:22 | argv | User-provided value |
+| test.c:94:7:94:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:91:19:91:22 | argv | User-provided value |

From 705529cdf71ded1fcef11e84252bd2a148b83653 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 May 2020 17:48:34 +0100
Subject: [PATCH 4/7] C++: Split StrLenFunction from PureStrFunction (without
 changes).

---
 .../code/cpp/models/implementations/Pure.qll  | 61 ++++++++++++++++++-
 1 file changed, 58 insertions(+), 3 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll
index c831a8bf3570..5740916e5955 100644
--- a/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll
+++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll
@@ -20,9 +20,7 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE
         name = "strpbrk" or
         name = "strcmp" or
         name = "strcspn" or
-        name = "strlen" or
         name = "strncmp" or
-        name = "strnlen" or
         name = "strrchr" or
         name = "strspn" or
         name = "strtod" or
@@ -30,7 +28,64 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE
         name = "strtol" or
         name = "strtoll" or
         name = "strtoq" or
-        name = "strtoul" or
+        name = "strtoul"
+      )
+    )
+  }
+
+  override predicate hasArrayInput(int bufParam) {
+    getParameter(bufParam).getUnspecifiedType() instanceof PointerType
+  }
+
+  override predicate hasArrayWithNullTerminator(int bufParam) {
+    getParameter(bufParam).getUnspecifiedType() instanceof PointerType
+  }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    exists(ParameterIndex i |
+      input.isParameter(i) and
+      exists(getParameter(i))
+      or
+      input.isParameterDeref(i) and
+      getParameter(i).getUnspecifiedType() instanceof PointerType
+    ) and
+    (
+      output.isReturnValueDeref() and
+      getUnspecifiedType() instanceof PointerType
+      or
+      output.isReturnValue()
+    )
+  }
+
+  override predicate parameterNeverEscapes(int i) {
+    getParameter(i).getUnspecifiedType() instanceof PointerType and
+    not parameterEscapesOnlyViaReturn(i)
+  }
+
+  override predicate parameterEscapesOnlyViaReturn(int i) {
+    i = 0 and
+    getUnspecifiedType() instanceof PointerType
+  }
+
+  override predicate parameterIsAlwaysReturned(int i) { none() }
+
+  override predicate hasOnlySpecificReadSideEffects() { any() }
+
+  override predicate hasOnlySpecificWriteSideEffects() { any() }
+
+  override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
+    getParameter(i).getUnspecifiedType() instanceof PointerType and
+    buffer = true
+  }
+}
+
+class StrLenFunction extends AliasFunction, ArrayFunction, TaintFunction, SideEffectFunction {
+  StrLenFunction() {
+    exists(string name |
+      hasGlobalOrStdName(name) and
+      (
+        name = "strlen" or
+        name = "strnlen" or
         name = "wcslen"
       )
       or

From 19c33ab41cc299f848c3cca1756e220a0b19b871 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Thu, 28 May 2020 17:55:45 +0100
Subject: [PATCH 5/7] C++: Refine StrLenFunction, including removal of taint
 flow.

---
 .../code/cpp/models/implementations/Pure.qll  | 24 +++----------------
 .../security-taint/tainted_diff.expected      |  4 ----
 .../security-taint/tainted_ir.expected        |  4 ----
 .../dataflow/taint-tests/format.cpp           |  6 ++---
 .../dataflow/taint-tests/localTaint.expected  |  5 ----
 .../dataflow/taint-tests/taint.expected       |  3 ---
 .../dataflow/taint-tests/test_ir.expected     |  3 ---
 .../semmle/tainted/ArithmeticTainted.expected |  3 ---
 .../tainted/IntegerOverflowTainted.expected   |  3 ---
 9 files changed, 6 insertions(+), 49 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll
index 5740916e5955..d4302da42ad9 100644
--- a/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll
+++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll
@@ -79,7 +79,7 @@ class PureStrFunction extends AliasFunction, ArrayFunction, TaintFunction, SideE
   }
 }
 
-class StrLenFunction extends AliasFunction, ArrayFunction, TaintFunction, SideEffectFunction {
+class StrLenFunction extends AliasFunction, ArrayFunction, SideEffectFunction {
   StrLenFunction() {
     exists(string name |
       hasGlobalOrStdName(name) and
@@ -107,30 +107,12 @@ class StrLenFunction extends AliasFunction, ArrayFunction, TaintFunction, SideEf
     getParameter(bufParam).getUnspecifiedType() instanceof PointerType
   }
 
-  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
-    exists(ParameterIndex i |
-      input.isParameter(i) and
-      exists(getParameter(i))
-      or
-      input.isParameterDeref(i) and
-      getParameter(i).getUnspecifiedType() instanceof PointerType
-    ) and
-    (
-      output.isReturnValueDeref() and
-      getUnspecifiedType() instanceof PointerType
-      or
-      output.isReturnValue()
-    )
-  }
-
   override predicate parameterNeverEscapes(int i) {
-    getParameter(i).getUnspecifiedType() instanceof PointerType and
-    not parameterEscapesOnlyViaReturn(i)
+    getParameter(i).getUnspecifiedType() instanceof PointerType
   }
 
   override predicate parameterEscapesOnlyViaReturn(int i) {
-    i = 0 and
-    getUnspecifiedType() instanceof PointerType
+    none()
   }
 
   override predicate parameterIsAlwaysReturned(int i) { none() }
diff --git a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected b/cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected
index 27573d98e768..26441b08ba49 100644
--- a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected
+++ b/cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected
@@ -3,10 +3,6 @@
 | test.cpp:49:23:49:28 | call to getenv | test.cpp:50:29:50:40 | envStrGlobal | AST only |
 | test.cpp:49:23:49:28 | call to getenv | test.cpp:52:2:52:12 | * ... | AST only |
 | test.cpp:49:23:49:28 | call to getenv | test.cpp:52:3:52:12 | envStr_ptr | AST only |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:10:64:14 | bytes | IR only |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:23 | call to strlen | IR only |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | (int)... | IR only |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | ... + ... | IR only |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:11:20:11:21 | s1 | AST only |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:67:7:67:13 | copying | AST only |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:69:10:69:13 | copy | AST only |
diff --git a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected b/cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected
index 97e713fbfbef..50c90bccb2b4 100644
--- a/cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected
+++ b/cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected
@@ -29,10 +29,6 @@
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:60:18:60:25 | userName |  |
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:60:29:60:34 | call to getenv |  |
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:60:29:60:47 | (const char *)... |  |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:10:64:14 | bytes |  |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:23 | call to strlen |  |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | (int)... |  |
-| test.cpp:60:29:60:34 | call to getenv | test.cpp:64:18:64:37 | ... + ... |  |
 | test.cpp:60:29:60:34 | call to getenv | test.cpp:64:25:64:32 | userName |  |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:11:36:11:37 | s2 |  |
 | test.cpp:68:28:68:33 | call to getenv | test.cpp:68:17:68:24 | userName |  |
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp
index 45b5d46daa7d..90bbb1ca0cd3 100644
--- a/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp
+++ b/cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp
@@ -148,11 +148,11 @@ void test2()
 	wchar_t *ws = wstring::source();
 	int i;
 
-	sink(strlen(s)); // [FALSE POSITIVE]
-	sink(wcslen(ws)); // [FALSE POSITIVE]
+	sink(strlen(s));
+	sink(wcslen(ws));
 
 	i = strlen(s) + 1;
-	sink(i); // [FALSE POSITIVE]
+	sink(i);
 
 	sink(s[strlen(s) - 1]); // tainted
 	sink(ws + (wcslen(ws) / 2)); // tainted
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
index 4b3b4b6029d2..2ce7d426eea5 100644
--- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
+++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
@@ -118,23 +118,18 @@
 | format.cpp:148:16:148:30 | call to source | format.cpp:152:14:152:15 | ws |  |
 | format.cpp:148:16:148:30 | call to source | format.cpp:158:7:158:8 | ws |  |
 | format.cpp:148:16:148:30 | call to source | format.cpp:158:20:158:21 | ws |  |
-| format.cpp:151:14:151:14 | s | format.cpp:151:7:151:12 | call to strlen | TAINT |
-| format.cpp:152:14:152:15 | ws | format.cpp:152:7:152:12 | call to wcslen | TAINT |
 | format.cpp:154:6:154:11 | call to strlen | format.cpp:154:6:154:18 | ... + ... | TAINT |
 | format.cpp:154:6:154:18 | ... + ... | format.cpp:154:2:154:18 | ... = ... |  |
 | format.cpp:154:6:154:18 | ... + ... | format.cpp:155:7:155:7 | i |  |
-| format.cpp:154:13:154:13 | s | format.cpp:154:6:154:11 | call to strlen | TAINT |
 | format.cpp:154:18:154:18 | 1 | format.cpp:154:6:154:18 | ... + ... | TAINT |
 | format.cpp:157:7:157:7 | s | format.cpp:157:7:157:22 | access to array | TAINT |
 | format.cpp:157:9:157:14 | call to strlen | format.cpp:157:9:157:21 | ... - ... | TAINT |
 | format.cpp:157:9:157:21 | ... - ... | format.cpp:157:7:157:22 | access to array | TAINT |
-| format.cpp:157:16:157:16 | s | format.cpp:157:9:157:14 | call to strlen | TAINT |
 | format.cpp:157:21:157:21 | 1 | format.cpp:157:9:157:21 | ... - ... | TAINT |
 | format.cpp:158:7:158:8 | ws | format.cpp:158:7:158:27 | ... + ... | TAINT |
 | format.cpp:158:7:158:27 | ref arg ... + ... | format.cpp:158:7:158:8 | ws [inner post update] |  |
 | format.cpp:158:13:158:18 | call to wcslen | format.cpp:158:13:158:26 | ... / ... | TAINT |
 | format.cpp:158:13:158:26 | ... / ... | format.cpp:158:7:158:27 | ... + ... | TAINT |
-| format.cpp:158:20:158:21 | ws | format.cpp:158:13:158:18 | call to wcslen | TAINT |
 | format.cpp:158:26:158:26 | 2 | format.cpp:158:13:158:26 | ... / ... | TAINT |
 | stl.cpp:67:12:67:17 | call to source | stl.cpp:71:7:71:7 | a |  |
 | stl.cpp:68:16:68:20 | 123 | stl.cpp:68:16:68:21 | call to basic_string | TAINT |
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected
index 2228c5436c0a..312a46e979e9 100644
--- a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected
+++ b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected
@@ -8,9 +8,6 @@
 | format.cpp:100:8:100:13 | buffer | format.cpp:99:30:99:43 | call to source |
 | format.cpp:105:8:105:13 | buffer | format.cpp:104:31:104:45 | call to source |
 | format.cpp:110:8:110:14 | wbuffer | format.cpp:109:38:109:52 | call to source |
-| format.cpp:151:7:151:12 | call to strlen | format.cpp:147:12:147:25 | call to source |
-| format.cpp:152:7:152:12 | call to wcslen | format.cpp:148:16:148:30 | call to source |
-| format.cpp:155:7:155:7 | i | format.cpp:147:12:147:25 | call to source |
 | format.cpp:157:7:157:22 | access to array | format.cpp:147:12:147:25 | call to source |
 | format.cpp:158:7:158:27 | ... + ... | format.cpp:148:16:148:30 | call to source |
 | stl.cpp:71:7:71:7 | a | stl.cpp:67:12:67:17 | call to source |
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected
index 7e3f17322dd9..41176c5caa20 100644
--- a/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected
+++ b/cpp/ql/test/library-tests/dataflow/taint-tests/test_ir.expected
@@ -1,6 +1,3 @@
-| format.cpp:151:7:151:12 | call to strlen | format.cpp:147:12:147:25 | call to source |
-| format.cpp:152:7:152:12 | call to wcslen | format.cpp:148:16:148:30 | call to source |
-| format.cpp:155:7:155:7 | i | format.cpp:147:12:147:25 | call to source |
 | format.cpp:157:7:157:22 | (int)... | format.cpp:147:12:147:25 | call to source |
 | format.cpp:157:7:157:22 | access to array | format.cpp:147:12:147:25 | call to source |
 | format.cpp:158:7:158:27 | ... + ... | format.cpp:148:16:148:30 | call to source |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected
index 131283c1c557..b74dd08dd76e 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/ArithmeticTainted.expected
@@ -8,6 +8,3 @@
 | test.c:14:15:14:28 | maxConnections | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:11:29:11:32 | argv | User-provided value |
 | test.c:44:7:44:10 | len2 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:41:17:41:20 | argv | User-provided value |
 | test.c:54:7:54:10 | len3 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:51:17:51:20 | argv | User-provided value |
-| test.c:74:7:74:10 | len5 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:71:19:71:22 | argv | User-provided value |
-| test.c:84:7:84:10 | len6 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:81:19:81:22 | argv | User-provided value |
-| test.c:94:7:94:10 | len7 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:91:19:91:22 | argv | User-provided value |
diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected
index 926d4130d142..d2b6499d40e2 100644
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected
@@ -4,6 +4,3 @@
 | test5.cpp:10:9:10:15 | call to strtoul | $@ flows to here and is used in an expression which might overflow. | test5.cpp:9:7:9:9 | buf | User-provided value |
 | test.c:44:7:44:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:41:17:41:20 | argv | User-provided value |
 | test.c:54:7:54:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:51:17:51:20 | argv | User-provided value |
-| test.c:74:7:74:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:71:19:71:22 | argv | User-provided value |
-| test.c:84:7:84:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:81:19:81:22 | argv | User-provided value |
-| test.c:94:7:94:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:91:19:91:22 | argv | User-provided value |

From f534f09784eabb91ccaff6cf22973a6feeaf03fe Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 29 May 2020 14:05:08 +0100
Subject: [PATCH 6/7] C++: Autoformat.

---
 cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll
index d4302da42ad9..8e1739fe6a74 100644
--- a/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll
+++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Pure.qll
@@ -111,9 +111,7 @@ class StrLenFunction extends AliasFunction, ArrayFunction, SideEffectFunction {
     getParameter(i).getUnspecifiedType() instanceof PointerType
   }
 
-  override predicate parameterEscapesOnlyViaReturn(int i) {
-    none()
-  }
+  override predicate parameterEscapesOnlyViaReturn(int i) { none() }
 
   override predicate parameterIsAlwaysReturned(int i) { none() }
 

From 9ee75aaca1cf0e68a3361307e68215b89c74ea56 Mon Sep 17 00:00:00 2001
From: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
Date: Fri, 29 May 2020 16:22:04 +0100
Subject: [PATCH 7/7] C++: Change note.

---
 change-notes/1.25/analysis-cpp.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/change-notes/1.25/analysis-cpp.md b/change-notes/1.25/analysis-cpp.md
index 43a5d9e9f8d1..7ab98ffe859a 100644
--- a/change-notes/1.25/analysis-cpp.md
+++ b/change-notes/1.25/analysis-cpp.md
@@ -41,4 +41,4 @@ The following changes in version 1.25 affect C/C++ analysis in all applications.
   };
   ```
 * The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) now considers that equality checks may block the flow of taint.  This results in fewer false positive results from queries that use this library.
-
+* The length of a tainted string (such as the return value of a call to `strlen` or `strftime` with tainted parameters) is no longer itself considered tainted by the `models` library.  This leads to fewer false positive results in queries that use any of our taint libraries.