Merge pull request #441 from felicity-semmle/cpp/SD-2777-cwe-qhelp

C++: Bring qhelp inline with current guidelines, part 1 (SD-2777)

Author: geoffw0

cpp/ql/src/Critical/DeadCodeCondition.qhelp

@@ -5,23 +5,26 @@
 
 
 <overview>
-<p>This rule finds branching statements with conditions that always evaluate to the same value. 
-More likely than not these conditions indicate a defect in the branching condition or are an artifact left behind after debugging.</p>
+<p>This query finds branching statements with conditions that always evaluate to the same value. 
+It is likely that these conditions indicate an error in the branching condition. 
+Alternatively, the conditions may have been left behind after debugging.</p>
 
 <include src="aliasAnalysisWarning.qhelp" />
-
 </overview>
-<recommendation>
-<p>Check the branch condition for defects, and verify that it isn't a remnant from debugging.</p>
 
+<recommendation>
+<p>Check the branch condition for logic errors. Check whether it is still required.</p>
 </recommendation>
-<example><sample src="DeadCodeCondition.cpp" />
-
-
-
-
-
 
+<example>
+<p>This example shows two branch conditions that always evaluate to the same value. 
+The two conditions and their associated branches should be deleted. 
+This will simplify the code and make it easier to maintain.</p>
 
+<sample src="DeadCodeCondition.cpp" />
 </example>
+
+<references>
+<li>SEI CERT C++ Coding Standard <a href="https://wiki.sei.cmu.edu/confluence/display/c/MSC12-C.+Detect+and+remove+code+that+has+no+effect+or+is+never+executed">MSC12-C. Detect and remove code that has no effect or is never executed</a>.</li>
+</references>
 </qhelp>

cpp/ql/src/Critical/DeadCodeFunction.cpp

@@ -2,7 +2,7 @@ class C {
 public:
 	void g() {
 		...
-		//f() was previously used but is now commented, orphaning f()
+		//f() was previously used but is now commented-out, orphaning f()
 		//f();
 		...
 	}

cpp/ql/src/Critical/DeadCodeFunction.qhelp

@@ -3,28 +3,31 @@
   "qhelp.dtd">
 <qhelp>
 
-
 <overview>
-<p>This rule finds functions that are non-public, non-virtual and are never called. Dead functions are often deprecated pieces of code, and should be removed 
-as they may increase object code size, decrease code comprehensibility, and create the possibility of misuse.</p>
+<p>This query highlights functions that are non-public, non-virtual, and are never called. 
+Dead functions are often deprecated pieces of code, and should be removed. 
+If left in the code base they increase object code size, decrease code comprehensibility, and create the possibility of misuse.</p>
 
 <p>
-<code>public</code> and <code>protected</code> functions are not considered by the check, as they could be part of the program's
-API and could be used by external programs.
+<code>public</code> and <code>protected</code> functions are ignored by this query. 
+This type of function may be part of the program's API and could be used by external programs.
 </p>
 
 <include src="callGraphWarning.qhelp" />
-
 </overview>
-<recommendation>
-<p>Consider removing the function.</p>
 
+<recommendation>
+<p>Verify that the function is genuinely unused and consider removing it.</p>
 </recommendation>
-<example><sample src="DeadCodeFunction.cpp" />
-
 
 
+<example>
+<p>The example below includes a function <code>f</code> that is no longer used and should be deleted.</p>
+<sample src="DeadCodeFunction.cpp" />
+</example>
 
+<references>
+<li>SEI CERT C++ Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/MSC12-C.+Detect+and+remove+code+that+has+no+effect+or+is+never+executed">MSC12-C. Detect and remove code that has no effect or is never executed</a>.</li>
+</references>
 
-</example>
 </qhelp>

cpp/ql/src/Critical/DeadCodeFunction.ql

@@ -1,6 +1,6 @@
 /**
  * @name Function is never called
- * @description A function is never called, and should be considered for removal. Unused functions may increase object size, decrease readability and create the possibility of misuse.
+ * @description Unused functions may increase object size, decrease readability, and create the possibility of misuse.
  * @kind problem
  * @id cpp/dead-code-function
  * @problem.severity warning

cpp/ql/src/Critical/GlobalUseBeforeInit.qhelp

@@ -5,26 +5,28 @@
 
 
 <overview>
-<p>This rule finds calls to functions that use a global variable which happen before the variable was initialized. 
+<p>This rule finds calls to functions that use a global variable before the variable has been initialized. 
 Not all compilers generate code that zero-out memory, especially when optimizations are enabled or the compiler 
 is not compliant with the latest language standards. Accessing uninitialized memory will lead to undefined results.
 </p>
 
 <include src="dataFlowWarning.qhelp" />
-
 </overview>
+
 <recommendation>
 <p>
 Initialize the global variable. If no constant can be used for initialization, ensure that all accesses to the variable occur after
 the initialization code is executed.
 </p>
-
 </recommendation>
-<example><sample src="GlobalUseBeforeInit.cpp" />
-
-
-
-
-
+<example>
+<p>
+In the example below, <code>callCtr</code> is wrongly used before it has been initialized.
+</p>
+<sample src="GlobalUseBeforeInit.cpp" />
 </example>
+
+<references>
+<li>SEI CERT C++ Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP53-CPP.+Do+not+read+uninitialized+memory">EXP53-CPP. Do not read uninitialized memory</a>.</li>
+</references>
 </qhelp>

cpp/ql/src/Critical/GlobalUseBeforeInit.ql

@@ -1,6 +1,6 @@
 /**
- * @name Global variable used before initialization
- * @description A function that uses a global variable has been called before the variable has been initialized. Not all compilers zero-out memory for variables, especially when optimizations are enabled, or if the compiler is not compliant with the latest language standards. Using an uninitialized variable leads to undefined results.
+ * @name Global variable may be used before initialization
+ * @description Using an uninitialized variable may lead to undefined results.
  * @kind problem
  * @id cpp/global-use-before-init
  * @problem.severity warning

cpp/ql/src/Critical/InconsistentNullnessTesting.qhelp

@@ -5,23 +5,30 @@
 
 
 <overview>
-<p>This rule finds pointer dereferences that do not check the pointer for nullness, while the same pointer is checked for nullness in other
-places in the code. It is most likely that the nullness check was omitted, and that a NULL pointer dereference can occur.
-Dereferencing a null pointer and attempting to modify its contents can lead to anything from a segfault to corrupting
-important system data (i.e. the interrupt table in some architectures).
+<p>This query finds pointer dereferences that do not first check the pointer for nullness, 
+even though the same pointer is checked for nullness in other 
+parts of the code. It is likely that the nullness check was accidentally omitted, and that a null pointer dereference can occur.
+Dereferencing a null pointer and attempting to modify its contents can lead to anything from a segmentation fault to corrupting
+important system data (including the interrupt table in some architectures).
 </p>
 
 <include src="pointsToWarning.qhelp" />
-
 </overview>
+
 <recommendation>
-<p>Make the nullness check on the pointer consistent across all dereferences.</p>
+<p>Use a nullness check consistently in all cases where a pointer is dereferenced.</p>
 
 </recommendation>
-<example><sample src="InconsistentNullnessTesting.cpp" />
-
-
-
-
+<example>
+<p>
+This code shows two examples where a pointer is dereferenced. 
+The first example checks that the pointer is not null before dereferencing it. 
+The second example fails to perform a nullness check, leading to a potential vulnerability in the code.
+</p>
+<sample src="InconsistentNullnessTesting.cpp" />
 </example>
+
+<references>
+<li>SEI CERT C++ Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/MEM10-C.+Define+and+use+a+pointer+validation+function">MEM10-C. Define and use a pointer validation function</a>.</li>
+</references>
 </qhelp>

cpp/ql/src/Critical/InconsistentNullnessTesting.ql

@@ -1,6 +1,6 @@
 /**
  * @name Inconsistent null check of pointer
- * @description A dereferenced pointer is not checked for nullness in the given location, but is checked in other locations. Dereferencing a NULL pointer leads to undefined results.
+ * @description A dereferenced pointer is not checked for nullness in this location, but it is checked in other locations. Dereferencing a null pointer leads to undefined results.
  * @kind problem
  * @id cpp/inconsistent-nullness-testing
  * @problem.severity warning

cpp/ql/src/Critical/OverflowCalculated.qhelp

@@ -6,36 +6,43 @@
 
 <overview>
 <p>
-This rule finds <code>malloc</code> that use a <code>strlen</code> for the size but to not take the 
-zero terminator into consideration, and <code>strcat/strncat</code> calls that are done on buffers that do
-not have the sufficient size to contain the new string. 
-</p>
+This query finds calls to:</p>
+<ul>
+<li><code>malloc</code> that use a <code>strlen</code> for the buffer size and do not take the 
+zero terminator into consideration.</li>
+<li><code>strcat</code> or <code>strncat</code> that use buffers that are too small to contain the new string.</li>
+</ul>
 
 <p>
-The indicated expression will cause a buffer overflow due to a buffer that is of insufficient size to contain
-the data being copied. Buffer overflows can result to anything from a segfault to a security vulnerability (particularly 
+The highlighted expression will cause a buffer overflow because the buffer is too small to contain
+the data being copied. Buffer overflows can result to anything from a segmentation fault to a security vulnerability (particularly 
 if the array is on stack-allocated memory).
 </p>
 
 <include src="aliasAnalysisWarning.qhelp" />
-
 </overview>
+
 <recommendation>
 <p>
 Increase the size of the buffer being allocated.
 </p>
-
 </recommendation>
-<example><sample src="OverflowCalculated.cpp" />
-
 
+<example>
+<p>This example includes three annotated calls that copy a string into a buffer.
+The first call to <code>malloc</code> creates a buffer that's the 
+same size as the string, leaving no space for the zero terminator 
+and causing an overflow. The second call to <code>malloc</code> 
+correctly calculates the required buffer size. The call to 
+<code>strcat</code> appends an additional string to the same buffer 
+causing a second overflow.</p>
 
+<sample src="OverflowCalculated.cpp" />
 </example>
-<references>
 
+<references>
 <li><a href="http://cwe.mitre.org/data/definitions/131.html">CWE-131: Incorrect Calculation of Buffer Size</a></li>
 <li>I. Gerg. <em>An Overview and Example of the Buffer-Overflow Exploit</em>. IANewsletter vol 7 no 4. 2005.</li>
 <li>M. Donaldson. <em>Inside the Buffer Overflow Attack: Mechanism, Method &amp; Prevention</em>. SANS Institute InfoSec Reading Room. 2002.</li>
-
 </references>
 </qhelp>

cpp/ql/src/Critical/OverflowDestination.qhelp

@@ -3,27 +3,31 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>The bounded copy functions <code>memcpy</code>, <code>memmove</code>, <code>strncpy</code>, <code>strncat</code> accept a size argument. You should call these functions with a size argument that is derived from the size of the destination buffer. Using a size argument that is derived from the source buffer may cause a buffer overflow. Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p>
-
-
+<p>The bounded copy functions <code>memcpy</code>, <code>memmove</code>, <code>strncpy</code>, <code>strncat</code> accept a size argument. 
+You should call these functions with a size argument that is derived from the size of the destination buffer. 
+Using a size argument that is derived from the source buffer may cause a buffer overflow. 
+Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p>
 </overview>
+
 <recommendation>
-<p>Check the highlighted function calls carefully, and ensure that the size parameter is derived from the size of the destination buffer,
+<p>Check the highlighted function calls carefully. 
+Ensure that the size parameter is derived from the size of the destination buffer, and 
 not the source buffer.</p>
 
 <include src="aliasAnalysisWarning.qhelp" />
-
 </recommendation>
-<example><sample src="OverflowDestination.cpp" />
 
 
+<example>
+<p>
+The code below shows an example where <code>strncpy</code> is called incorrectly, without checking the size of the destination buffer.
+In the second example the call has been updated to include the size of the destination buffer.</p>
 
+<sample src="OverflowDestination.cpp" />
 </example>
-<references>
 
-<li><a href="http://cwe.mitre.org/data/definitions/119.html">CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer</a></li>
+<references>
 <li>I. Gerg. <em>An Overview and Example of the Buffer-Overflow Exploit</em>. IANewsletter vol 7 no 4. 2005.</li>
 <li>M. Donaldson. <em>Inside the Buffer Overflow Attack: Mechanism, Method &amp; Prevention</em>. SANS Institute InfoSec Reading Room. 2002.</li>
-
 </references>
 </qhelp>