[CPP-434] Add table of constructs to Qhelp. Rewrite examples section.

change-notes/1.23/analysis-cpp.md

@@ -9,7 +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 is undefined behavior. Example: `a + b < a`. |
+| 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-bad2.cpp

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

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

@@ -1,3 +1,4 @@
-bool baf(unsigned short n1, unsigned short delta) {
-    return (unsigned short)(n1 + delta) < n1; // GOOD
+#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

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

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

@@ -4,11 +4,11 @@
 <qhelp>
 <overview>
 <p>
-When checking for integer overflow, one often writes tests like
+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 overflows have undefined
+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
@@ -17,10 +17,89 @@ optimizing compiler.
 </overview>
 <recommendation>
 <p>
-When checking for overflow by adding two values, first make sure that <code>a</code>
-or <code>b</code> are (converted into) unsigned values, unless it is
-certain that the signed addition cannot overflow.
+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.
+
+The table below lists various expressions where signed overflow may
+occur, along with proposed rewritings.  It should not be
+considered as exhaustive.
 </p>
+<table>
+  <tr>
+    <th>Original Construct</th>
+    <th>Alternate Construct(s)</th>
+    <th>Notes</th>
+  </tr>
+  <tr>
+    <td><tt><table>
+      <tr>
+        <td>unsigned short i, delta;</td>
+      </tr><tr>
+        <td>i + delta &lt; i</td>
+      </tr>
+    </table></tt></td>
+    <td><tt><table>
+      <tr>
+        <td>unsigned short i, delta;</td>
+      </tr><tr>
+        <td>(unsigned short)(i + delta)&nbsp;&lt;&nbsp;i</td>
+      </tr>
+    </table></tt></td>
+    <td><tt>i + delta</tt>does not actually overflow due to <tt>int</tt> promotion</td>
+  </tr>
+  <tr>
+    <td>&nbsp;</td>
+    <td><tt><table>
+      <tr>
+        <td>unsigned short i, delta;</td>
+      </tr><tr>
+        <td>i > USHORT_MAX - delta</td>
+      </tr>
+    </table></tt></td>
+    <td>Must include <tt>limits.h</tt> or <tt>climits</tt></td>
+  </tr>
+  <tr>
+    <td><tt><table>
+      <tr>
+        <td>int i, delta;</td>
+      </tr><tr>
+        <td>i + delta &lt; i</td>
+      </tr>
+    </table></tt></td>
+    <td><tt><table>
+      <tr>
+        <td>int i, delta;</td>
+      </tr><tr>
+        <td>i &gt; INT_MAX - delta</td>
+      </tr>
+    </table></tt></td>
+    <td>Must include <tt>limits.h</tt> or <tt>climits</tt></td>
+  </tr>
+  <tr>
+    <td>&nbsp;</td>
+    <td><tt><table>
+      <tr>
+        <td>int i, delta;</td>
+      </tr><tr>
+        <td>(unsigned)i + delta &lt; i</td>
+      </tr>
+    </table></tt></td>
+    <td>Change in program semantics</td>
+  </tr>
+  <tr>
+    <td>&nbsp;</td>
+    <td><tt><table>
+      <tr>
+        <td>unsigned int i, delta;</td>
+      </tr><tr>
+        <td>i + delta &lt; i</td>
+      </tr>
+    </table></tt></td>
+    <td>Change in program semantics</td>
+  </tr>
+</table>
 </recommendation>
 <example>
 <p>
@@ -34,6 +113,17 @@ 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
@@ -45,26 +135,18 @@ hold true, which likely is not what the programmer intended. (see also the
 </p>
 <sample src="SignedOverflowCheck-bad2.cpp" />
 <p>
-The following example builds upon the previous one.  Again, we have two
-<code>unsigned short</code> values getting promoted to a wider type, resulting
-in a comparison that always succeeds (since there is no overflow).  To
-test whether we have an <code>unsigned short</code> overflow, we cast the
-left-hand side to it, causing the right-hand side to remain an <code>unsigned
-short</code> as well.
-</p>
-<sample src="SignedOverflowCheck-good1.cpp" />
-<p>
-In the next example, we have two <code>signed int</code> values that we
-wish to add together.  Adding them "as-is" opens the possibility of
-a signed integer overflow, the results of which are undefined.
-By casting one of the operands to <code>unsigned</code>, the entire 
-expression is evaluated using <code>unsigned</code>
-values, which is defined behavior per the C/C++ standard.
+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="http://www.cs.utah.edu/~regehr/papers/overflow12.pdf">Understanding Integer Overflow in C/C++</a></li>
 </references>
 </qhelp>