Merge pull request #2075 from zlaski-semmle/zlaski/cpp434

[CPP-434] Detect signed overflow checks

change-notes/1.23/analysis-cpp.md

@@ -9,6 +9,7 @@ The following changes in version 1.23 affect C/C++ analysis in all applications.
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | reliability, japanese-era | This query is a combination of two old queries that were identical in purpose but separate as an implementation detail.  This new query replaces Hard-coded Japanese era start date in call (`cpp/japanese-era/constructor-or-method-with-exact-era-date`) and Hard-coded Japanese era start date in struct (`cpp/japanese-era/struct-with-exact-era-date`). |
+| Signed overflow check (`cpp/signed-overflow-check`) | correctness, reliability | Finds overflow checks that rely on signed integer addition to overflow, which has undefined behavior. Example: `a + b < a`. |
 
 ## Changes to existing queries
 

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-bad1.cpp

@@ -0,0 +1,3 @@
+bool foo(int n1, unsigned short delta) {
+    return n1 + delta < n1; // BAD
+}

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-bad2.cpp

@@ -0,0 +1,4 @@
+bool bar(unsigned short n1, unsigned short delta) {
+    // NB: Comparison is always false
+    return n1 + delta < n1; // GOOD (but misleading)
+}

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good1.cpp

@@ -0,0 +1,4 @@
+#include <limits.h>
+bool foo(int n1, unsigned short delta) {
+    return n1 > INT_MAX - delta; // GOOD
+}

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good2.cpp

@@ -0,0 +1,3 @@
+bool bar(unsigned short n1, unsigned short delta) {
+    return (unsigned short)(n1 + delta) < n1; // GOOD
+}

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.qhelp

@@ -0,0 +1,115 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+When checking for integer overflow, you may often write tests like
+<code>a + b &lt; a</code>.  This works fine if <code>a</code> or
+<code>b</code> are unsigned integers, since any overflow in the addition
+will cause the value to simply "wrap around."  However, using
+<i>signed</i> integers is problematic because signed overflow has undefined
+behavior according to the C and C++ standards.  If the addition overflows
+and has an undefined result, the comparison will likewise be undefined;
+it may produce an unintended result, or may be deleted entirely by an
+optimizing compiler.
+</p>
+</overview>
+<recommendation>
+<p>
+Solutions to this problem can be thought of as falling into one of two
+categories: (1) rewrite the signed expression so that overflow cannot occur
+but the signedness remains, or (2) rewrite (or cast) the signed expression
+into unsigned form.
+</p>
+
+<p>
+Below we list examples of expressions where signed overflow may
+occur, along with proposed solutions.  The list should not be
+considered exhaustive.
+</p>
+
+<p>
+Given <code>unsigned short i, delta</code> and <code>i + delta &lt; i</code>, 
+it is possible to rewrite it as <code>(unsigned short)(i + delta)&nbsp;&lt;&nbsp;i</code>.
+Note that <code>i + delta</code>does not actually overflow, due to <code>int</code> promotion
+</p>
+
+<p>
+Given <code>unsigned short i, delta</code> and <code>i + delta &lt; i</code>,
+it is also possible to rewrite it as <code>USHORT_MAX - delta</code>. It must be true
+that <code>delta &gt; 0</code> and the <code>limits.h</code> or <code>climits</code>
+header has been included.
+</p>
+
+<p>
+Given <code>int i, delta</code> and <code>i + delta &lt; i</code>,
+it is possible to rewrite it as <code>INT_MAX - delta</code>. It must be true
+that <code>delta &gt; 0</code> and the <code>limits.h</code> or <code>climits</code>
+header has been included.
+</p>
+
+<p>
+Given <code>int i, delta</code> and <code>i + delta &lt; i</code>,
+it is also possible to rewrite it as <code>(unsigned)i + delta &lt; i</code>.
+Note that program semantics are affected by this change.
+</p>
+
+<p>
+Given <code>int i, delta</code> and <code>i + delta &lt; i</code>,
+it is also possible to rewrite it as <code>unsigned int i, delta</code> and
+<code>i + delta &lt; i</code>.  Note that program semantics are 
+affected by this change.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the following example, even though <code>delta</code> has been declared
+<code>unsigned short</code>, C/C++ type promotion rules require that its
+type is promoted to the larger type used in the addition and comparison,
+namely a <code>signed int</code>.  Addition is performed on 
+signed integers, and may have undefined behavior if an overflow occurs.
+As a result, the entire (comparison) expression may also have an undefined
+result.
+</p>
+<sample src="SignedOverflowCheck-bad1.cpp" />
+<p>
+The following example builds upon the previous one.  Instead of
+performing an addition (which could overflow), we have re-framed the
+solution so that a subtraction is used instead.  Since <code>delta</code>
+is promoted to a <code>signed int</code> and <code>INT_MAX</code> denotes
+the largest possible positive value for an <code>signed int</code>,
+the expression <code>INT_MAX - delta</code> can never be less than zero
+or more than <code>INT_MAX</code>.  Hence, any overflow and underflow
+are avoided.
+</p>
+<sample src="SignedOverflowCheck-good1.cpp" />
+<p>
+In the following example, even though both <code>n</code> and <code>delta</code>
+have been declared <code>unsigned short</code>, both are promoted to 
+<code>signed int</code> prior to addition.  Because we started out with the
+narrower <code>short</code> type, the addition is guaranteed not to overflow
+and is therefore defined.  But the fact that <code>n1 + delta</code> never 
+overflows means that the condition <code>n1 + delta &lt; n1</code> will never
+hold true, which likely is not what the programmer intended. (see also the
+<code>cpp/bad-addition-overflow-check</code> query).
+</p>
+<sample src="SignedOverflowCheck-bad2.cpp" />
+<p>
+The next example provides a solution to the previous one.  Even though
+<code>i + delta</code> does not overflow, casting it to an
+<code>unsigned short</code> truncates the addition modulo 2^16,
+so that <code>unsigned short</code> "wrap around" may now be observed.
+Furthermore, since the left-hand side is now of type <code>unsigned short</code>,
+the right-hand side does not need to be promoted to a <code>signed int</code>.
+</p>
+
+<sample src="SignedOverflowCheck-good2.cpp" />
+</example>
+<references>
+<li><a href="http://c-faq.com/expr/preservingrules.html">comp.lang.c FAQ list · Question 3.19 (Preserving rules)</a></li>
+<li><a href="https://wiki.sei.cmu.edu/confluence/display/c/INT31-C.+Ensure+that+integer+conversions+do+not+result+in+lost+or+misinterpreted+data">INT31-C. Ensure that integer conversions do not result in lost or misinterpreted data</a></li>
+<li>W. Dietz, P. Li, J. Regehr, V. Adve. <a href="https://www.cs.utah.edu/~regehr/papers/overflow12.pdf">Understanding Integer Overflow in C/C++</a></li>
+</references>
+</qhelp>

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.ql

@@ -0,0 +1,31 @@
+/**
+ * @name Undefined result of signed test for overflow
+ * @description Testing for overflow by adding a value to a variable
+ *              to see if it "wraps around" works only for
+ *              unsigned integer values.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id cpp/signed-overflow-check
+ * @tags reliability
+ *       security
+ */
+
+import cpp
+private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+private import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
+
+from RelationalOperation ro, AddExpr add, Expr expr1, Expr expr2
+where
+  ro.getAnOperand() = add and
+  add.getAnOperand() = expr1 and
+  ro.getAnOperand() = expr2 and
+  globalValueNumber(expr1) = globalValueNumber(expr2) and
+  add.getUnspecifiedType().(IntegralType).isSigned() and
+  not exists(MacroInvocation mi | mi.getAnAffectedElement() = add) and
+  exprMightOverflowPositively(add) and
+  exists(Compilation c | c.getAFileCompiled() = ro.getFile() |
+    not c.getAnArgument() = "-fwrapv" and
+    not c.getAnArgument() = "-fno-strict-overflow"
+  )
+select ro, "Testing for signed overflow may produce undefined results."

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/BadAdditionOverflowCheck/BadAdditionOverflowCheck.expected

@@ -1 +1,3 @@
+| SignedOverflowCheck.cpp:35:9:35:23 | ... < ... | Bad overflow check. |
+| SignedOverflowCheck.cpp:113:12:113:66 | ... < ... | Bad overflow check. |
 | test.cpp:3:11:3:19 | ... < ... | Bad overflow check. |

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/BadAdditionOverflowCheck/SignedOverflowCheck.cpp

@@ -0,0 +1,130 @@
+// Signed-comparison tests
+
+/* 1. Signed-signed comparison.  The semantics are undefined. */
+bool cannotHoldAnother8(int n1) {
+    // clang 8.0.0 -O2: deleted (silently)
+    // gcc 9.2 -O2: deleted (silently)
+    // msvc 19.22 /O2: not deleted
+    return n1 + 8 < n1; // BAD
+}
+
+/* 2. Signed comparison with a narrower unsigned type.  The narrower
+      type gets promoted to the (signed) larger type, and so the
+      semantics are undefined. */
+bool cannotHoldAnotherUShort(int n1, unsigned short delta) {
+    // clang 8.0.0 -O2: deleted (silently)
+    // gcc 9.2 -O2: deleted (silently)
+    // msvc 19.22 /O2: not deleted
+    return n1 + delta < n1; // BAD
+}
+
+/* 3. Signed comparison with a non-narrower unsigned type.  The
+      signed type gets promoted to (a possibly wider) unsigned type,
+      and the resulting comparison is unsigned. */
+bool cannotHoldAnotherUInt(int n1, unsigned int delta) {
+    // clang 8.0.0 -O2: not deleted
+    // gcc 9.2 -O2: not deleted
+    // msvc 19.22 /O2: not deleted
+    return n1 + delta < n1; // GOOD
+}
+
+bool shortShort1(unsigned short n1, unsigned short delta) {
+
+    // BAD [BadAdditionOverflowCheck.ql]
+    // GOOD [SigneOverflowCheck.ql]: Test always fails, but will never overflow.
+	return n1 + delta < n1;
+}
+
+bool shortShort2(unsigned short n1, unsigned short delta) {
+    // clang 8.0.0 -O2: not deleted
+    // gcc 9.2 -O2: not deleted
+    // msvc 19.22 /O2: not deleted
+	return (unsigned short)(n1 + delta) < n1; // GOOD
+}
+
+/* Distinguish `varname` from `ptr->varname` and `obj.varname` */
+struct N {
+	int n1;
+} n, *np;
+
+bool shortStruct1(unsigned short n1, unsigned short delta) {
+	return np->n1 + delta < n1; // GOOD
+}
+
+bool shortStruct1a(unsigned short n1, unsigned short delta) {
+	return n1 + delta < n.n1; // GOOD
+}
+
+bool shortStruct2(unsigned short n1, unsigned short delta) {
+	return (unsigned short)(n1 + delta) < n.n1; // GOOD
+}
+
+struct se {
+        int xPos;
+        short yPos;
+        short xSize;
+        short ySize;
+};
+
+extern se *getSo(void);
+
+bool func1(se *so) {
+	se *o = getSo();
+	if (so->xPos + so->xSize < so->xPos // BAD
+	    || so->xPos > o->xPos + o->xSize) { // GOOD
+    	// clang 8.0.0 -O2: not deleted
+    	// gcc 9.2 -O2: not deleted
+    	// msvc 19.22 /O2: not deleted
+		return false;
+	}
+	return true;
+}
+
+bool checkOverflow3(unsigned int a, unsigned short b) {
+  return (a + b < a);  // GOOD
+}
+
+struct C {
+  unsigned int length;
+};
+
+int checkOverflow4(unsigned int ioff, C c) {
+  // not deleted by gcc or clang
+  if ((int)(ioff + c.length) < (int)ioff) return 0; // GOOD
+  return 1;
+}
+
+int overflow12(int n) {
+    // not deleted by gcc or clang
+	return (n + 32 <= (unsigned)n? -1: 1); // BAD: n + 32 can overflow
+}
+
+bool multipleCasts(char x) {
+
+    // BAD [UNDETECTED - BadAdditionOverflowCheck.ql]
+    // GOOD [SigneOverflowCheck.ql]: Test always fails, but will never overflow.
+    return (int)(unsigned short)x + 2 < (int)(unsigned short)x; // GOOD: cannot overflow
+}
+
+bool multipleCasts2(char x) {
+
+    // BAD [BadAdditionOverflowCheck.ql]
+    // GOOD [SigneOverflowCheck.ql]: Test always fails, but will never overflow.
+    return (int)(unsigned short)(x + '1') < (int)(unsigned short)x;
+}
+
+int does_it_overflow(int n1, unsigned short delta) {
+    return n1 + (unsigned)delta < n1; // GOOD: everything converted to unsigned
+}
+
+int overflow12b(int n) {
+    // not deleted by gcc or clang
+	return ((unsigned)(n + 32) <= (unsigned)n? -1: 1); // BAD: n + 32 may overflow
+}
+
+#define MACRO(E1, E2) (E1) <= (E2)? -1: 1
+
+int overflow12_macro(int n) {
+	return MACRO((unsigned)(n + 32), (unsigned)n); // GOOD: inside a macro expansion
+}
+

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/BadAdditionOverflowCheck/SignedOverflowCheck.expected

@@ -0,0 +1,5 @@
+| SignedOverflowCheck.cpp:8:12:8:22 | ... < ... | Testing for signed overflow may produce undefined results. |
+| SignedOverflowCheck.cpp:18:12:18:26 | ... < ... | Testing for signed overflow may produce undefined results. |
+| SignedOverflowCheck.cpp:73:6:73:36 | ... < ... | Testing for signed overflow may produce undefined results. |
+| SignedOverflowCheck.cpp:99:10:99:30 | ... <= ... | Testing for signed overflow may produce undefined results. |
+| SignedOverflowCheck.cpp:122:10:122:42 | ... <= ... | Testing for signed overflow may produce undefined results. |

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/BadAdditionOverflowCheck/SignedOverflowCheck.qlref

@@ -0,0 +1 @@
+Likely Bugs/Arithmetic/SignedOverflowCheck.ql

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/BadAdditionOverflowCheck/test.cpp

@@ -1,11 +1,11 @@
 // Test for BadAdditionOverflowCheck.
 bool checkOverflow1(unsigned short a, unsigned short b) {
-  return (a + b < a);  // BAD: a + b is automatically promoted to int.
+  return (a + b < a);  // BAD: comparison always false (due to promotion).
 }
 
 // Test for BadAdditionOverflowCheck.
 bool checkOverflow2(unsigned short a, unsigned short b) {
-  return ((unsigned short)(a + b) < a);  // GOOD: explicit cast
+  return ((unsigned short)(a + b) < a);  // GOOD
 }
 
 // Test for PointlessSelfComparison.