From 16f2bacf4dd4aeab4425fd42fc2abb846f2e8aab Mon Sep 17 00:00:00 2001
From: Raul Garcia <raulga@microsoft.com>
Date: Fri, 14 Dec 2018 15:42:49 -0800
Subject: [PATCH 1/7] cpp - Using the return value of a strcpy or related
 string copy function in an if statement

---
 .gitignore                                    |   3 +
 .../Likely Typos/UsingStrcpyInConditional.cpp |   4 +
 .../UsingStrcpyInConditional.qhelp            |  42 +++++
 .../Likely Typos/UsingStrcpyInConditional.ql  |  45 ++++++
 .../UsingStrcpyInConditional.expected         |  18 +++
 .../UsingStrcpyInConditional.qlref            |   1 +
 .../UsingStrcpyInConditional/test.c           |  70 ++++++++
 .../UsingStrcpyInConditional/test.cpp         | 149 ++++++++++++++++++
 8 files changed, 332 insertions(+)
 create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.cpp
 create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.qhelp
 create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql
 create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.expected
 create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.qlref
 create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.c
 create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.cpp

diff --git a/.gitignore b/.gitignore
index 7e82b2f488ca..606e844f8d3d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -13,3 +13,6 @@
 /.vs/ql/v15/Browse.VC.db
 /.vs/ProjectSettings.json
 
+/.vs/ql_6324/v15
+/.vs/VSWorkspaceState.json
+/.vs/slnx.sqlite-journal
diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.cpp b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.cpp
new file mode 100644
index 000000000000..fc4c6466505b
--- /dev/null
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.cpp	
@@ -0,0 +1,4 @@
+if(strcpy(szbuf1, "Manager") == 0) // most likely strcmp was intended instead of strcpy
+{
+	// ...
+}
\ No newline at end of file
diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.qhelp b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.qhelp
new file mode 100644
index 000000000000..76513e4b99f0
--- /dev/null
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.qhelp	
@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>This rule finds uses of the string copy function calls that return the <code>destination</code> parameter, 
+and that do not have a return value reserved to indicate an error.</p>
+
+<p>The rule flags occurrences using such string copy functions as the conditional of an <code>if</code> statement, either directly, as part of an equality operator or a logical operator.</p>
+  
+<p>The string copy functions that the rule takes into consideration are: 
+<li>strcpy</li>
+<li>wcscpy</li>
+<li>_mbscpy</li>
+<li>strncpy</li>
+<li>_strncpy_l</li>
+<li>wcsncpy</li>
+<li>_wcsncpy_l</li>
+<li>_mbsncpy</li>
+<li>_mbsncpy_l</li>
+</p>
+
+<p>NOTE: It is highly recommended to consider using a more secure version of string manipulation functions suchas as <code>strcpy_s</code>.</p>
+
+</overview>
+<recommendation>
+  <p>Check to ensure that the flagged expressions are not typos.</p>
+  <p>If a string comparison is intended, change the function to the appropriate string comparison function.</p>
+  <p>If a string copy is really intended, very likely a secure version of the string copy function such as <code>strcpy_s</code> was intended instead of the insecure version of the string copy function.</p>
+
+</recommendation>
+<example><sample src="UsingStrcpyInConditional.cpp" />
+</example>
+
+<references>
+<li>Microsoft Books on Line: <a href="https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2012/ccf4h9w8(v=vs.110)">C6324</a></li>
+<li>Microsoft Books on Line: <a href="https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/strcpy-wcscpy-mbscpy?view=vs-2017">strcpy, wcscpy, _mbscpy</a></li>
+<li>US-CERT: <a href="https://www.us-cert.gov/bsi/articles/knowledge/coding-practices/strcpy_s-and-strcat_s">strncpy_s() and strncat_s()</a></li>
+
+</references>
+</qhelp>
diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql
new file mode 100644
index 000000000000..8a862a5129f0
--- /dev/null
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql	
@@ -0,0 +1,45 @@
+/**
+ * @name Using the return value of a strcpy or related string copy function in an if statement
+ * @description The return value for strcpy or related string copy functions have no reserved return value to indicate an error.
+ *              Using these functions as part of an if statement condition indicates a logic error.
+ *              Either the intent was to use a more secure version of a string copy function (such as strcpy_s), 
+ *              or a string compare function (such as strcmp).
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/string-copy-function-in-if-condition
+ * @tags external/microsoft/C6324
+ */
+
+import cpp
+
+predicate isStringComparisonFunction(string functionName) {
+  functionName = "strcpy"
+  or functionName = "wcscpy"
+  or functionName = "_mbscpy"
+  or functionName = "strncpy"
+  or functionName = "_strncpy_l"
+  or functionName = "wcsncpy"
+  or functionName = "_wcsncpy_l"
+  or functionName = "_mbsncpy"
+  or functionName = "_mbsncpy_l"
+}
+
+from IfStmt ifs,  
+FunctionCall func
+where isStringComparisonFunction( func.getTarget().getQualifiedName() )
+  and ( func = ifs.getCondition() 
+    or exists( UnaryLogicalOperation ule |
+      ule = ifs.getCondition()
+      and func = ule.getAChild()
+    )
+    or exists( BinaryLogicalOperation ble |
+      ble = ifs.getCondition()
+      and func = ble.getAChild()
+    )
+    or exists( EqualityOperation eop |
+      eop = ifs.getCondition()
+      and func = eop.getAChild()
+    )
+  )
+select func, "Incorrect use of function " + func.getTarget().getQualifiedName() + ". Verify the logic and replace with a secure string copy function, or a string comparison function."
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.expected
new file mode 100644
index 000000000000..5e9c36e80751
--- /dev/null
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.expected	
@@ -0,0 +1,18 @@
+| test.c:33:9:33:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.c:37:10:37:15 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.c:41:9:41:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.c:45:27:45:32 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.c:49:9:49:15 | call to strncpy | Incorrect use of function strncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:75:9:75:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:79:10:79:15 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:83:9:83:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:87:27:87:32 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:91:9:91:37 | call to wcscpy | Incorrect use of function wcscpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:95:9:95:14 | call to wcscpy | Incorrect use of function wcscpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:99:9:99:15 | call to _mbscpy | Incorrect use of function _mbscpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:103:9:103:15 | call to strncpy | Incorrect use of function strncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:107:9:107:15 | call to wcsncpy | Incorrect use of function wcsncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:111:9:111:16 | call to _mbsncpy | Incorrect use of function _mbsncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:115:9:115:18 | call to _strncpy_l | Incorrect use of function _strncpy_l. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:119:9:119:18 | call to _wcsncpy_l | Incorrect use of function _wcsncpy_l. Verify the logic and replace with a secure string copy function, or a string comparison function. |
+| test.cpp:123:9:123:18 | call to _mbsncpy_l | Incorrect use of function _mbsncpy_l. Verify the logic and replace with a secure string copy function, or a string comparison function. |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.qlref b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.qlref
new file mode 100644
index 000000000000..f606fe35837c
--- /dev/null
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.qlref	
@@ -0,0 +1 @@
+Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.c
new file mode 100644
index 000000000000..49ec774e8a59
--- /dev/null
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.c	
@@ -0,0 +1,70 @@
+typedef unsigned int size_t; 
+typedef int* locale_t;
+
+char* strcpy(char* destination, const char* source)
+{
+    return destination;
+}
+char* strncpy(char* destination, const char* source, size_t count)
+{
+    return destination;
+}
+
+int SomeFunction()
+{
+    return 1;
+}
+
+int SomeFunctionThatTakesString(char* destination)
+{
+    return 1;
+}
+
+int strcmp(char* destination, const char* source)
+{
+    return 1;
+}
+
+void PositiveCases()
+{
+    char szbuf1[100];
+    char szbuf2[100];
+
+    if (strcpy(szbuf1, "test")) // Bug, direct usage
+    {
+    }
+
+    if (!strcpy(szbuf1, "test")) // Bug, unary binary operator
+    {
+    }
+
+    if (strcpy(szbuf1, "test") == 0) // Bug, equality operator
+    {
+    }
+
+    if (SomeFunction() && strcpy(szbuf1, "test")) // Bug, binary logical operator
+    {
+    }
+
+    if (strncpy(szbuf1, "test", 100))  // Bug
+    {
+    }
+}
+
+void NegativeCases()
+{
+    char szbuf1[100];
+
+    if (SomeFunction())
+    {
+    }
+
+    if (SomeFunctionThatTakesString(strcpy(szbuf1, "test")))
+    {
+    }
+
+    if (strcmp(szbuf1, "test"))
+    {
+    }
+
+}
\ No newline at end of file
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.cpp
new file mode 100644
index 000000000000..b35c584becfa
--- /dev/null
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.cpp	
@@ -0,0 +1,149 @@
+typedef unsigned long size_t; 
+typedef int* locale_t;
+
+char* strcpy(char* destination, const char* source)
+{
+    return destination;
+}
+wchar_t* wcscpy(wchar_t* destination, const wchar_t* source)
+{
+    return destination;
+}
+unsigned char* _mbscpy(unsigned char* destination, const unsigned char* source)
+{
+    return destination;
+}
+char* strncpy(char* destination, const char* source, size_t count)
+{
+    return destination;
+}
+wchar_t* wcsncpy(wchar_t* destination, const wchar_t* source, size_t count)
+{
+    return destination;
+}
+unsigned char* _mbsncpy(unsigned char* destination, const unsigned char* source, size_t count)
+{
+    return destination;
+}
+char* _strncpy_l(char* destination, const char* source, size_t count, locale_t locale)
+{
+    return destination;
+}
+wchar_t* _wcsncpy_l(wchar_t* destination, const wchar_t* source, size_t count, locale_t locale)
+{
+    return destination;
+}
+unsigned char* _mbsncpy_l(unsigned char* destination, const unsigned char* source, size_t count, locale_t locale)
+{
+    return destination;
+}
+
+int SomeFunction()
+{
+    return 1;
+}
+
+int SomeFunctionThatTakesString(char* destination)
+{
+    return 1;
+}
+
+int strcmp(char* destination, const char* source)
+{
+    return 1;
+}
+
+int strcpy_s(char* destination, size_t dest_size, const char* source)
+{
+    return 1;
+}
+
+#define WCSCPY_6324(x,y) wcscpy(x,y)
+
+void PositiveCases()
+{
+    char szbuf1[100];
+    char szbuf2[100];
+    wchar_t wscbuf1[100];
+    wchar_t wscbuf2[100];
+    unsigned char mbcbuf1[100];
+    unsigned char mbcbuf2[100];
+
+    locale_t x;
+    *x = 0;
+
+    if (strcpy(szbuf1, "test")) // Bug, direct usage
+    {
+    }
+
+    if (!strcpy(szbuf1, "test")) // Bug, unary binary operator
+    {
+    }
+
+    if (strcpy(szbuf1, "test") == 0) // Bug, equality operator
+    {
+    }
+
+    if (SomeFunction() && strcpy(szbuf1, "test")) // Bug, binary logical operator
+    {
+    }
+
+    if (WCSCPY_6324(wscbuf1, wscbuf2)) // Bug, using a macro
+    {
+    }
+
+    if (wcscpy(wscbuf1, wscbuf2)) // Bug
+    {
+    }
+
+    if (_mbscpy(mbcbuf1, mbcbuf2)) // Bug
+    {
+    }
+
+    if (strncpy(szbuf1, "test", 100))  // Bug
+    {
+    }
+
+    if (wcsncpy(wscbuf1, wscbuf2, 100)) // Bug
+    {
+    }
+
+    if (_mbsncpy(mbcbuf1, (const unsigned char*)"test", 100)) // Bug
+    {
+    }
+
+    if (_strncpy_l(szbuf1, "test", 100, x))  // Bug
+    {
+    }
+
+    if (_wcsncpy_l(wscbuf1, wscbuf2, 100, x)) // Bug
+    {
+    }
+
+    if (_mbsncpy_l(mbcbuf1, (const unsigned char*)"test", 100, x)) //Bug
+    {
+    }
+
+}
+
+void NegativeCases()
+{
+    char szbuf1[100];
+
+    if (SomeFunction())
+    {
+    }
+
+    if (SomeFunctionThatTakesString(strcpy(szbuf1, "test")))
+    {
+    }
+
+    if (strcmp(szbuf1, "test"))
+    {
+    }
+
+    if (strcpy_s(szbuf1, 100, "test"))
+    {
+    }
+
+}
\ No newline at end of file

From 05316024543e99410d534f3e6bf679e3e91a0eba Mon Sep 17 00:00:00 2001
From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com>
Date: Fri, 14 Dec 2018 15:47:04 -0800
Subject: [PATCH 2/7] Update .gitignore

---
 .gitignore | 1 -
 1 file changed, 1 deletion(-)

diff --git a/.gitignore b/.gitignore
index 3f5f0e49f9cc..b5c88ce04abf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -9,7 +9,6 @@
 */ql/test/**/*.testproj
 */ql/test/**/*.actual
 
-
 # Visual studio temporaries, except a file used by QL4VS
 .vs/*
 !.vs/VSWorkspaceSettings.json

From 28932e85d9d4797ed8ecc71f4579cad5966f9d61 Mon Sep 17 00:00:00 2001
From: Raul Garcia <raulga@microsoft.com>
Date: Wed, 2 Jan 2019 16:23:19 -0800
Subject: [PATCH 3/7] Fixing the code based on PR feedback.

---
 ...nditional.cpp => UsingStrcpyAsBoolean.cpp} |  0
 ...ional.qhelp => UsingStrcpyAsBoolean.qhelp} | 27 +++---
 .../Likely Typos/UsingStrcpyAsBoolean.ql      | 92 +++++++++++++++++++
 .../Likely Typos/UsingStrcpyInConditional.ql  | 45 ---------
 .../UsingStrcpyAsBoolean.expected             | 34 +++++++
 .../UsingStrcpyAsBoolean.qlref                |  1 +
 .../test.c                                    |  4 +
 .../test.cpp                                  |  5 +
 .../UsingStrcpyInConditional.expected         | 18 ----
 .../UsingStrcpyInConditional.qlref            |  1 -
 10 files changed, 150 insertions(+), 77 deletions(-)
 rename cpp/ql/src/Likely Bugs/Likely Typos/{UsingStrcpyInConditional.cpp => UsingStrcpyAsBoolean.cpp} (100%)
 rename cpp/ql/src/Likely Bugs/Likely Typos/{UsingStrcpyInConditional.qhelp => UsingStrcpyAsBoolean.qhelp} (86%)
 create mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql
 delete mode 100644 cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql
 create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected
 create mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.qlref
 rename cpp/ql/test/query-tests/Likely Bugs/Likely Typos/{UsingStrcpyInConditional => UsingStrcpyAsBoolean}/test.c (95%)
 rename cpp/ql/test/query-tests/Likely Bugs/Likely Typos/{UsingStrcpyInConditional => UsingStrcpyAsBoolean}/test.cpp (96%)
 delete mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.expected
 delete mode 100644 cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.qlref

diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.cpp b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp
similarity index 100%
rename from cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.cpp
rename to cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp
diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.qhelp b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.qhelp
similarity index 86%
rename from cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.qhelp
rename to cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.qhelp
index 76513e4b99f0..ccf0fa55d088 100644
--- a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.qhelp	
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.qhelp	
@@ -9,18 +9,19 @@ and that do not have a return value reserved to indicate an error.</p>
 
 <p>The rule flags occurrences using such string copy functions as the conditional of an <code>if</code> statement, either directly, as part of an equality operator or a logical operator.</p>
   
-<p>The string copy functions that the rule takes into consideration are: 
-<li>strcpy</li>
-<li>wcscpy</li>
-<li>_mbscpy</li>
-<li>strncpy</li>
-<li>_strncpy_l</li>
-<li>wcsncpy</li>
-<li>_wcsncpy_l</li>
-<li>_mbsncpy</li>
-<li>_mbsncpy_l</li>
-</p>
-
+<p>The string copy functions that the rule takes into consideration are: </p>
+<ul>
+  <li>strcpy</li>
+  <li>wcscpy</li>
+  <li>_mbscpy</li>
+  <li>strncpy</li>
+  <li>_strncpy_l</li>
+  <li>wcsncpy</li>
+  <li>_wcsncpy_l</li>
+  <li>_mbsncpy</li>
+  <li>_mbsncpy_l</li>
+</ul>
+  
 <p>NOTE: It is highly recommended to consider using a more secure version of string manipulation functions suchas as <code>strcpy_s</code>.</p>
 
 </overview>
@@ -30,7 +31,7 @@ and that do not have a return value reserved to indicate an error.</p>
   <p>If a string copy is really intended, very likely a secure version of the string copy function such as <code>strcpy_s</code> was intended instead of the insecure version of the string copy function.</p>
 
 </recommendation>
-<example><sample src="UsingStrcpyInConditional.cpp" />
+<example><sample src="UsingStrcpyAsBoolean.cpp" />
 </example>
 
 <references>
diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql
new file mode 100644
index 000000000000..182362f74a73
--- /dev/null
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql	
@@ -0,0 +1,92 @@
+/**
+ * @name Using the return value of a strcpy or related string copy function as a boolean operator
+ * @description The return value for strcpy, strncpy, or related string copy functions have no reserved return value to indicate an error.
+ *              Using the return values of these functions as boolean function .
+ *              Either the intent was to use a more secure version of a string copy function (such as strcpy_s), or a string compare function (such as strcmp).
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/string-copy-return-value-as-boolean
+ * @tags external/microsoft/C6324
+ */
+
+import cpp
+import semmle.code.cpp.dataflow.DataFlow
+
+predicate isStringComparisonFunction(string functionName) {
+  functionName = "strcpy"
+  or functionName = "wcscpy"
+  or functionName = "_mbscpy"
+  or functionName = "strncpy"
+  or functionName = "_strncpy_l"
+  or functionName = "wcsncpy"
+  or functionName = "_wcsncpy_l"
+  or functionName = "_mbsncpy"
+  or functionName = "_mbsncpy_l"
+}
+
+predicate isBoolean( Expr e1 )
+{
+  exists ( Type t1 |
+    t1 = e1.getType() and
+    (t1.hasName("bool") or t1.hasName("BOOL") or t1.hasName("_Bool"))
+  )
+}
+
+class StringCopyToBooleanConfiguration extends DataFlow::Configuration {
+  StringCopyToBooleanConfiguration() {
+    this = "StringCopyToBooleanConfiguration"
+  }
+
+  override predicate isSource(DataFlow::Node source) {
+    exists( FunctionCall func |
+      func = source.asExpr()
+      and isStringComparisonFunction( func.getTarget().getQualifiedName())
+    )
+  }
+ 
+  override predicate isSink(DataFlow::Node sink) {
+    exists( Expr expr1 |
+      expr1 = sink.asExpr()
+      and isBoolean( expr1.getConversion*())
+    )
+  }
+}
+
+predicate isStringCopyCastedAsBoolean( FunctionCall func, Expr expr1, string msg ) {
+  exists( StringCopyToBooleanConfiguration modeConfig |
+    modeConfig.hasFlow(DataFlow::exprNode(func), DataFlow::exprNode(expr1))
+    and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used as boolean."
+  )
+}
+
+predicate isStringCopyUsedInCondition( FunctionCall func, Expr expr1, string msg ) {
+  exists( ConditionalStmt condstmt |
+    condstmt.getAChild() = expr1 |
+    isStringComparisonFunction( func.getTarget().getQualifiedName() )
+    and ( 
+      // The string copy function is used directly as the conditional expression
+      func = condstmt.getChild(0)
+      // ... or it is being used in an equality or logical operation 
+      or exists( EqualityOperation eop |
+        eop = expr1
+        and func = eop.getAChild()
+      )
+      or exists( UnaryLogicalOperation ule |
+        expr1 = ule
+        and func = ule.getAChild()
+      )
+      or exists( BinaryLogicalOperation ble |
+        expr1 = ble
+        and func = ble.getAChild()
+      )
+    )
+    and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used in a conditional."
+  )
+}
+
+from FunctionCall func, Expr expr1, string msg
+where 
+  isStringCopyCastedAsBoolean(func, expr1, msg)
+  or isStringCopyUsedInCondition(func, expr1, msg)
+select expr1, msg
diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql
deleted file mode 100644
index 8a862a5129f0..000000000000
--- a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql	
+++ /dev/null
@@ -1,45 +0,0 @@
-/**
- * @name Using the return value of a strcpy or related string copy function in an if statement
- * @description The return value for strcpy or related string copy functions have no reserved return value to indicate an error.
- *              Using these functions as part of an if statement condition indicates a logic error.
- *              Either the intent was to use a more secure version of a string copy function (such as strcpy_s), 
- *              or a string compare function (such as strcmp).
- * @kind problem
- * @problem.severity error
- * @precision high
- * @id cpp/string-copy-function-in-if-condition
- * @tags external/microsoft/C6324
- */
-
-import cpp
-
-predicate isStringComparisonFunction(string functionName) {
-  functionName = "strcpy"
-  or functionName = "wcscpy"
-  or functionName = "_mbscpy"
-  or functionName = "strncpy"
-  or functionName = "_strncpy_l"
-  or functionName = "wcsncpy"
-  or functionName = "_wcsncpy_l"
-  or functionName = "_mbsncpy"
-  or functionName = "_mbsncpy_l"
-}
-
-from IfStmt ifs,  
-FunctionCall func
-where isStringComparisonFunction( func.getTarget().getQualifiedName() )
-  and ( func = ifs.getCondition() 
-    or exists( UnaryLogicalOperation ule |
-      ule = ifs.getCondition()
-      and func = ule.getAChild()
-    )
-    or exists( BinaryLogicalOperation ble |
-      ble = ifs.getCondition()
-      and func = ble.getAChild()
-    )
-    or exists( EqualityOperation eop |
-      eop = ifs.getCondition()
-      and func = eop.getAChild()
-    )
-  )
-select func, "Incorrect use of function " + func.getTarget().getQualifiedName() + ". Verify the logic and replace with a secure string copy function, or a string comparison function."
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected
new file mode 100644
index 000000000000..66578ada002c
--- /dev/null
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected	
@@ -0,0 +1,34 @@
+| test.c:33:9:33:14 | call to strcpy | Return Value of strcpy used in a conditional. |
+| test.c:37:9:37:31 | ! ... | Return Value of strcpy used in a conditional. |
+| test.c:41:9:41:35 | ... == ... | Return Value of strcpy used in a conditional. |
+| test.c:45:9:45:48 | ... && ... | Return Value of strcpy used in a conditional. |
+| test.c:49:9:49:15 | call to strncpy | Return Value of strncpy used in a conditional. |
+| test.c:53:6:53:34 | ! ... | Return Value of strncpy used in a conditional. |
+| test.cpp:75:9:75:14 | call to strcpy | Return Value of strcpy used as boolean. |
+| test.cpp:75:9:75:14 | call to strcpy | Return Value of strcpy used in a conditional. |
+| test.cpp:79:9:79:31 | ! ... | Return Value of strcpy used in a conditional. |
+| test.cpp:79:10:79:15 | call to strcpy | Return Value of strcpy used as boolean. |
+| test.cpp:83:9:83:35 | ... == ... | Return Value of strcpy used in a conditional. |
+| test.cpp:87:9:87:48 | ... && ... | Return Value of strcpy used in a conditional. |
+| test.cpp:87:27:87:32 | call to strcpy | Return Value of strcpy used as boolean. |
+| test.cpp:91:9:91:37 | call to wcscpy | Return Value of wcscpy used as boolean. |
+| test.cpp:91:9:91:37 | call to wcscpy | Return Value of wcscpy used in a conditional. |
+| test.cpp:95:9:95:14 | call to wcscpy | Return Value of wcscpy used as boolean. |
+| test.cpp:95:9:95:14 | call to wcscpy | Return Value of wcscpy used in a conditional. |
+| test.cpp:99:9:99:15 | call to _mbscpy | Return Value of _mbscpy used as boolean. |
+| test.cpp:99:9:99:15 | call to _mbscpy | Return Value of _mbscpy used in a conditional. |
+| test.cpp:103:9:103:15 | call to strncpy | Return Value of strncpy used as boolean. |
+| test.cpp:103:9:103:15 | call to strncpy | Return Value of strncpy used in a conditional. |
+| test.cpp:107:9:107:15 | call to wcsncpy | Return Value of wcsncpy used as boolean. |
+| test.cpp:107:9:107:15 | call to wcsncpy | Return Value of wcsncpy used in a conditional. |
+| test.cpp:111:9:111:16 | call to _mbsncpy | Return Value of _mbsncpy used as boolean. |
+| test.cpp:111:9:111:16 | call to _mbsncpy | Return Value of _mbsncpy used in a conditional. |
+| test.cpp:115:9:115:18 | call to _strncpy_l | Return Value of _strncpy_l used as boolean. |
+| test.cpp:115:9:115:18 | call to _strncpy_l | Return Value of _strncpy_l used in a conditional. |
+| test.cpp:119:9:119:18 | call to _wcsncpy_l | Return Value of _wcsncpy_l used as boolean. |
+| test.cpp:119:9:119:18 | call to _wcsncpy_l | Return Value of _wcsncpy_l used in a conditional. |
+| test.cpp:123:9:123:18 | call to _mbsncpy_l | Return Value of _mbsncpy_l used as boolean. |
+| test.cpp:123:9:123:18 | call to _mbsncpy_l | Return Value of _mbsncpy_l used in a conditional. |
+| test.cpp:127:6:127:34 | ! ... | Return Value of strncpy used in a conditional. |
+| test.cpp:127:7:127:13 | call to strncpy | Return Value of strncpy used as boolean. |
+| test.cpp:131:11:131:17 | call to strncpy | Return Value of strncpy used as boolean. |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.qlref b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.qlref
new file mode 100644
index 000000000000..6ae254cc9747
--- /dev/null
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.qlref	
@@ -0,0 +1 @@
+Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c
similarity index 95%
rename from cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.c
rename to cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c
index 49ec774e8a59..8c5046d90b4b 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.c	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c	
@@ -49,6 +49,10 @@ void PositiveCases()
     if (strncpy(szbuf1, "test", 100))  // Bug
     {
     }
+
+	if (!strncpy(szbuf1, "test", 100))  // Bug
+	{
+	}
 }
 
 void NegativeCases()
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp
similarity index 96%
rename from cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.cpp
rename to cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp
index b35c584becfa..ff17d67b84a9 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/test.cpp	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp	
@@ -124,6 +124,11 @@ void PositiveCases()
     {
     }
 
+	if (!strncpy(szbuf1, "test", 100))  // Bug
+	{
+	}
+
+	bool b = strncpy(szbuf1, "test", 100);
 }
 
 void NegativeCases()
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.expected
deleted file mode 100644
index 5e9c36e80751..000000000000
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.expected	
+++ /dev/null
@@ -1,18 +0,0 @@
-| test.c:33:9:33:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.c:37:10:37:15 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.c:41:9:41:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.c:45:27:45:32 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.c:49:9:49:15 | call to strncpy | Incorrect use of function strncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:75:9:75:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:79:10:79:15 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:83:9:83:14 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:87:27:87:32 | call to strcpy | Incorrect use of function strcpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:91:9:91:37 | call to wcscpy | Incorrect use of function wcscpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:95:9:95:14 | call to wcscpy | Incorrect use of function wcscpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:99:9:99:15 | call to _mbscpy | Incorrect use of function _mbscpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:103:9:103:15 | call to strncpy | Incorrect use of function strncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:107:9:107:15 | call to wcsncpy | Incorrect use of function wcsncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:111:9:111:16 | call to _mbsncpy | Incorrect use of function _mbsncpy. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:115:9:115:18 | call to _strncpy_l | Incorrect use of function _strncpy_l. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:119:9:119:18 | call to _wcsncpy_l | Incorrect use of function _wcsncpy_l. Verify the logic and replace with a secure string copy function, or a string comparison function. |
-| test.cpp:123:9:123:18 | call to _mbsncpy_l | Incorrect use of function _mbsncpy_l. Verify the logic and replace with a secure string copy function, or a string comparison function. |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.qlref b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.qlref
deleted file mode 100644
index f606fe35837c..000000000000
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyInConditional/UsingStrcpyInConditional.qlref	
+++ /dev/null
@@ -1 +0,0 @@
-Likely Bugs/Likely Typos/UsingStrcpyInConditional.ql

From e7bc3e6c0df335e63a33de89ec02cc60ea842009 Mon Sep 17 00:00:00 2001
From: Raul Garcia <42392023+raulgarciamsft@users.noreply.github.com>
Date: Wed, 2 Jan 2019 17:33:28 -0800
Subject: [PATCH 4/7] Update UsingStrcpyAsBoolean.cpp

---
 cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp
index fc4c6466505b..3e7e4b797d76 100644
--- a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp	
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.cpp	
@@ -1,4 +1,4 @@
 if(strcpy(szbuf1, "Manager") == 0) // most likely strcmp was intended instead of strcpy
 {
 	// ...
-}
\ No newline at end of file
+}

From 2c1d7bbc415f247a82124e7a23f6c1be9709faa6 Mon Sep 17 00:00:00 2001
From: Raul Garcia <raulga@microsoft.com>
Date: Thu, 3 Jan 2019 10:06:49 -0800
Subject: [PATCH 5/7] Switched to DataFlow::localFlow to avoid false positives.

---
 .../Likely Typos/UsingStrcpyAsBoolean.ql      | 28 +++----------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql
index 182362f74a73..fdadc8b807b1 100644
--- a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql	
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql	
@@ -33,31 +33,11 @@ predicate isBoolean( Expr e1 )
   )
 }
 
-class StringCopyToBooleanConfiguration extends DataFlow::Configuration {
-  StringCopyToBooleanConfiguration() {
-    this = "StringCopyToBooleanConfiguration"
-  }
-
-  override predicate isSource(DataFlow::Node source) {
-    exists( FunctionCall func |
-      func = source.asExpr()
-      and isStringComparisonFunction( func.getTarget().getQualifiedName())
-    )
-  }
- 
-  override predicate isSink(DataFlow::Node sink) {
-    exists( Expr expr1 |
-      expr1 = sink.asExpr()
-      and isBoolean( expr1.getConversion*())
-    )
-  }
-}
-
 predicate isStringCopyCastedAsBoolean( FunctionCall func, Expr expr1, string msg ) {
-  exists( StringCopyToBooleanConfiguration modeConfig |
-    modeConfig.hasFlow(DataFlow::exprNode(func), DataFlow::exprNode(expr1))
-    and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used as boolean."
-  )
+  DataFlow::localFlow(DataFlow::exprNode(func), DataFlow::exprNode(expr1))
+  and isBoolean( expr1.getConversion*())
+  and isStringComparisonFunction( func.getTarget().getQualifiedName())
+  and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used as boolean."
 }
 
 predicate isStringCopyUsedInCondition( FunctionCall func, Expr expr1, string msg ) {

From 880306c6216c9ce5a0794b9b9c2317176a31bce0 Mon Sep 17 00:00:00 2001
From: Raul Garcia <raulga@microsoft.com>
Date: Fri, 4 Jan 2019 10:45:43 -0800
Subject: [PATCH 6/7] Removing duplicated results

---
 .../Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql   |  4 +++-
 .../UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected | 10 ----------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql
index fdadc8b807b1..8fdd30f356b9 100644
--- a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql	
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql	
@@ -67,6 +67,8 @@ predicate isStringCopyUsedInCondition( FunctionCall func, Expr expr1, string msg
 
 from FunctionCall func, Expr expr1, string msg
 where 
-  isStringCopyCastedAsBoolean(func, expr1, msg)
+  ( isStringCopyCastedAsBoolean(func, expr1, msg) and 
+    not isStringCopyUsedInCondition(func, expr1, _)
+  )
   or isStringCopyUsedInCondition(func, expr1, msg)
 select expr1, msg
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected
index 66578ada002c..6e2b5f0c2a08 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected	
@@ -4,30 +4,20 @@
 | test.c:45:9:45:48 | ... && ... | Return Value of strcpy used in a conditional. |
 | test.c:49:9:49:15 | call to strncpy | Return Value of strncpy used in a conditional. |
 | test.c:53:6:53:34 | ! ... | Return Value of strncpy used in a conditional. |
-| test.cpp:75:9:75:14 | call to strcpy | Return Value of strcpy used as boolean. |
 | test.cpp:75:9:75:14 | call to strcpy | Return Value of strcpy used in a conditional. |
 | test.cpp:79:9:79:31 | ! ... | Return Value of strcpy used in a conditional. |
 | test.cpp:79:10:79:15 | call to strcpy | Return Value of strcpy used as boolean. |
 | test.cpp:83:9:83:35 | ... == ... | Return Value of strcpy used in a conditional. |
 | test.cpp:87:9:87:48 | ... && ... | Return Value of strcpy used in a conditional. |
 | test.cpp:87:27:87:32 | call to strcpy | Return Value of strcpy used as boolean. |
-| test.cpp:91:9:91:37 | call to wcscpy | Return Value of wcscpy used as boolean. |
 | test.cpp:91:9:91:37 | call to wcscpy | Return Value of wcscpy used in a conditional. |
-| test.cpp:95:9:95:14 | call to wcscpy | Return Value of wcscpy used as boolean. |
 | test.cpp:95:9:95:14 | call to wcscpy | Return Value of wcscpy used in a conditional. |
-| test.cpp:99:9:99:15 | call to _mbscpy | Return Value of _mbscpy used as boolean. |
 | test.cpp:99:9:99:15 | call to _mbscpy | Return Value of _mbscpy used in a conditional. |
-| test.cpp:103:9:103:15 | call to strncpy | Return Value of strncpy used as boolean. |
 | test.cpp:103:9:103:15 | call to strncpy | Return Value of strncpy used in a conditional. |
-| test.cpp:107:9:107:15 | call to wcsncpy | Return Value of wcsncpy used as boolean. |
 | test.cpp:107:9:107:15 | call to wcsncpy | Return Value of wcsncpy used in a conditional. |
-| test.cpp:111:9:111:16 | call to _mbsncpy | Return Value of _mbsncpy used as boolean. |
 | test.cpp:111:9:111:16 | call to _mbsncpy | Return Value of _mbsncpy used in a conditional. |
-| test.cpp:115:9:115:18 | call to _strncpy_l | Return Value of _strncpy_l used as boolean. |
 | test.cpp:115:9:115:18 | call to _strncpy_l | Return Value of _strncpy_l used in a conditional. |
-| test.cpp:119:9:119:18 | call to _wcsncpy_l | Return Value of _wcsncpy_l used as boolean. |
 | test.cpp:119:9:119:18 | call to _wcsncpy_l | Return Value of _wcsncpy_l used in a conditional. |
-| test.cpp:123:9:123:18 | call to _mbsncpy_l | Return Value of _mbsncpy_l used as boolean. |
 | test.cpp:123:9:123:18 | call to _mbsncpy_l | Return Value of _mbsncpy_l used in a conditional. |
 | test.cpp:127:6:127:34 | ! ... | Return Value of strncpy used in a conditional. |
 | test.cpp:127:7:127:13 | call to strncpy | Return Value of strncpy used as boolean. |

From 18bb6696e0abcabe08416b972127da2916b39a01 Mon Sep 17 00:00:00 2001
From: Raul Garcia <raulga@microsoft.com>
Date: Mon, 7 Jan 2019 10:44:11 -0800
Subject: [PATCH 7/7] Fixing conditional only issue. I changed  to detect any
 logical operation usage (i.e. !, ==), but I kept usage in a conditional
 directly as a separate detection condition. I found no false positives on the
 projects you shared with me previously.

---
 .../Likely Typos/UsingStrcpyAsBoolean.ql      | 27 +++++-----
 .../UsingStrcpyAsBoolean.expected             | 50 +++++++++++--------
 .../Likely Typos/UsingStrcpyAsBoolean/test.c  |  9 ++++
 .../UsingStrcpyAsBoolean/test.cpp             |  9 ++++
 4 files changed, 63 insertions(+), 32 deletions(-)

diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql
index 8fdd30f356b9..55412b9729cf 100644
--- a/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql	
+++ b/cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql	
@@ -40,15 +40,11 @@ predicate isStringCopyCastedAsBoolean( FunctionCall func, Expr expr1, string msg
   and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used as boolean."
 }
 
-predicate isStringCopyUsedInCondition( FunctionCall func, Expr expr1, string msg ) {
-  exists( ConditionalStmt condstmt |
-    condstmt.getAChild() = expr1 |
+predicate isStringCopyUsedInLogicalOperationOrCondition( FunctionCall func, Expr expr1, string msg ) {
     isStringComparisonFunction( func.getTarget().getQualifiedName() )
-    and ( 
-      // The string copy function is used directly as the conditional expression
-      func = condstmt.getChild(0)
-      // ... or it is being used in an equality or logical operation 
-      or exists( EqualityOperation eop |
+    and ((( 
+      // it is being used in an equality or logical operation 
+      exists( EqualityOperation eop |
         eop = expr1
         and func = eop.getAChild()
       )
@@ -61,14 +57,21 @@ predicate isStringCopyUsedInCondition( FunctionCall func, Expr expr1, string msg
         and func = ble.getAChild()
       )
     )
-    and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used in a conditional."
-  )
+    and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used in a logical operation."
+    )
+    or
+      exists( ConditionalStmt condstmt |
+      condstmt.getAChild() = expr1 |
+      // or the string copy function is used directly as the conditional expression
+      func = condstmt.getChild(0)
+      and msg = "Return Value of " + func.getTarget().getQualifiedName() + " used directly in a conditional expression."
+    ))
 }
 
 from FunctionCall func, Expr expr1, string msg
 where 
   ( isStringCopyCastedAsBoolean(func, expr1, msg) and 
-    not isStringCopyUsedInCondition(func, expr1, _)
+    not isStringCopyUsedInLogicalOperationOrCondition(func, expr1, _)
   )
-  or isStringCopyUsedInCondition(func, expr1, msg)
+  or isStringCopyUsedInLogicalOperationOrCondition(func, expr1, msg)
 select expr1, msg
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected
index 6e2b5f0c2a08..3a56eaee4add 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/UsingStrcpyAsBoolean.expected	
@@ -1,24 +1,34 @@
-| test.c:33:9:33:14 | call to strcpy | Return Value of strcpy used in a conditional. |
-| test.c:37:9:37:31 | ! ... | Return Value of strcpy used in a conditional. |
-| test.c:41:9:41:35 | ... == ... | Return Value of strcpy used in a conditional. |
-| test.c:45:9:45:48 | ... && ... | Return Value of strcpy used in a conditional. |
-| test.c:49:9:49:15 | call to strncpy | Return Value of strncpy used in a conditional. |
-| test.c:53:6:53:34 | ! ... | Return Value of strncpy used in a conditional. |
-| test.cpp:75:9:75:14 | call to strcpy | Return Value of strcpy used in a conditional. |
-| test.cpp:79:9:79:31 | ! ... | Return Value of strcpy used in a conditional. |
+| test.c:34:9:34:14 | call to strcpy | Return Value of strcpy used directly in a conditional expression. |
+| test.c:38:9:38:31 | ! ... | Return Value of strcpy used in a logical operation. |
+| test.c:42:9:42:35 | ... == ... | Return Value of strcpy used in a logical operation. |
+| test.c:46:9:46:48 | ... && ... | Return Value of strcpy used in a logical operation. |
+| test.c:50:9:50:15 | call to strncpy | Return Value of strncpy used directly in a conditional expression. |
+| test.c:54:6:54:34 | ! ... | Return Value of strncpy used in a logical operation. |
+| test.c:58:11:58:39 | ! ... | Return Value of strncpy used in a logical operation. |
+| test.c:60:11:60:37 | ... && ... | Return Value of strcpy used in a logical operation. |
+| test.c:62:11:62:37 | ... == ... | Return Value of strcpy used in a logical operation. |
+| test.c:64:11:64:37 | ... != ... | Return Value of strcpy used in a logical operation. |
+| test.cpp:75:9:75:14 | call to strcpy | Return Value of strcpy used directly in a conditional expression. |
+| test.cpp:79:9:79:31 | ! ... | Return Value of strcpy used in a logical operation. |
 | test.cpp:79:10:79:15 | call to strcpy | Return Value of strcpy used as boolean. |
-| test.cpp:83:9:83:35 | ... == ... | Return Value of strcpy used in a conditional. |
-| test.cpp:87:9:87:48 | ... && ... | Return Value of strcpy used in a conditional. |
+| test.cpp:83:9:83:35 | ... == ... | Return Value of strcpy used in a logical operation. |
+| test.cpp:87:9:87:48 | ... && ... | Return Value of strcpy used in a logical operation. |
 | test.cpp:87:27:87:32 | call to strcpy | Return Value of strcpy used as boolean. |
-| test.cpp:91:9:91:37 | call to wcscpy | Return Value of wcscpy used in a conditional. |
-| test.cpp:95:9:95:14 | call to wcscpy | Return Value of wcscpy used in a conditional. |
-| test.cpp:99:9:99:15 | call to _mbscpy | Return Value of _mbscpy used in a conditional. |
-| test.cpp:103:9:103:15 | call to strncpy | Return Value of strncpy used in a conditional. |
-| test.cpp:107:9:107:15 | call to wcsncpy | Return Value of wcsncpy used in a conditional. |
-| test.cpp:111:9:111:16 | call to _mbsncpy | Return Value of _mbsncpy used in a conditional. |
-| test.cpp:115:9:115:18 | call to _strncpy_l | Return Value of _strncpy_l used in a conditional. |
-| test.cpp:119:9:119:18 | call to _wcsncpy_l | Return Value of _wcsncpy_l used in a conditional. |
-| test.cpp:123:9:123:18 | call to _mbsncpy_l | Return Value of _mbsncpy_l used in a conditional. |
-| test.cpp:127:6:127:34 | ! ... | Return Value of strncpy used in a conditional. |
+| test.cpp:91:9:91:37 | call to wcscpy | Return Value of wcscpy used directly in a conditional expression. |
+| test.cpp:95:9:95:14 | call to wcscpy | Return Value of wcscpy used directly in a conditional expression. |
+| test.cpp:99:9:99:15 | call to _mbscpy | Return Value of _mbscpy used directly in a conditional expression. |
+| test.cpp:103:9:103:15 | call to strncpy | Return Value of strncpy used directly in a conditional expression. |
+| test.cpp:107:9:107:15 | call to wcsncpy | Return Value of wcsncpy used directly in a conditional expression. |
+| test.cpp:111:9:111:16 | call to _mbsncpy | Return Value of _mbsncpy used directly in a conditional expression. |
+| test.cpp:115:9:115:18 | call to _strncpy_l | Return Value of _strncpy_l used directly in a conditional expression. |
+| test.cpp:119:9:119:18 | call to _wcsncpy_l | Return Value of _wcsncpy_l used directly in a conditional expression. |
+| test.cpp:123:9:123:18 | call to _mbsncpy_l | Return Value of _mbsncpy_l used directly in a conditional expression. |
+| test.cpp:127:6:127:34 | ! ... | Return Value of strncpy used in a logical operation. |
 | test.cpp:127:7:127:13 | call to strncpy | Return Value of strncpy used as boolean. |
 | test.cpp:131:11:131:17 | call to strncpy | Return Value of strncpy used as boolean. |
+| test.cpp:133:16:133:44 | ! ... | Return Value of strncpy used in a logical operation. |
+| test.cpp:133:17:133:23 | call to strncpy | Return Value of strncpy used as boolean. |
+| test.cpp:135:11:135:16 | call to strcpy | Return Value of strcpy used as boolean. |
+| test.cpp:135:11:135:37 | ... && ... | Return Value of strcpy used in a logical operation. |
+| test.cpp:137:11:137:37 | ... == ... | Return Value of strcpy used in a logical operation. |
+| test.cpp:139:11:139:37 | ... != ... | Return Value of strcpy used in a logical operation. |
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c
index 8c5046d90b4b..7e3753613f8a 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.c	
@@ -29,6 +29,7 @@ void PositiveCases()
 {
     char szbuf1[100];
     char szbuf2[100];
+	int result;
 
     if (strcpy(szbuf1, "test")) // Bug, direct usage
     {
@@ -53,6 +54,14 @@ void PositiveCases()
 	if (!strncpy(szbuf1, "test", 100))  // Bug
 	{
 	}
+
+	result = !strncpy(szbuf1, "test", 100);
+
+	result = strcpy(szbuf1, "test") && 1;
+
+	result = strcpy(szbuf1, "test") == 0;
+
+	result = strcpy(szbuf1, "test") != 0;
 }
 
 void NegativeCases()
diff --git a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp
index ff17d67b84a9..58aa67680582 100644
--- a/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp	
+++ b/cpp/ql/test/query-tests/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean/test.cpp	
@@ -129,6 +129,15 @@ void PositiveCases()
 	}
 
 	bool b = strncpy(szbuf1, "test", 100);
+
+	bool result = !strncpy(szbuf1, "test", 100);
+
+	result = strcpy(szbuf1, "test") && 1;
+
+	result = strcpy(szbuf1, "test") == 0;
+
+	result = strcpy(szbuf1, "test") != 0;
+
 }
 
 void NegativeCases()