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

C++: Update to bring into line with current guidelines, part 2 (SD-2777)

Author: geoffw0

cpp/ql/src/Critical/DescriptorMayNotBeClosed.qhelp

@@ -6,23 +6,25 @@
 
 <overview>
 <p>
-This rule looks at functions that return file or socket descriptors, but may return an error value before actually closing the resource.
-This can occur when an operation performed on the open descriptor fails, and the function returns with an error before closing the open resource. An improperly handled error could cause the function to leak resource descriptors.
+This query looks at functions that return file or socket descriptors, but may return an error value before actually closing the resource.
+This can occur when an operation performed on the open descriptor fails, and the function returns with an error before it closes the open resource. An improperly handled error could cause the function to leak resource descriptors. Failing to close resources in the function that opened them also makes it more difficult to detect leaks.
 </p> 
 
 <include src="dataFlowWarning.qhelp" />
-
 </overview>
-<recommendation>
-<p>Ensure that the function frees all the resources it acquired when an error occurs.</p>
 
+<recommendation>
+<p>When an error occurs, ensure that the function frees all the resources it holds open.</p>
 </recommendation>
+
 <example>
+<p>In the example below, the <code>sockfd</code> socket may remain open if an error is triggered. 
+The code should be updated to ensure that the socket is always closed when when the function ends.
+</p>
 <sample src="DescriptorMayNotBeClosed.cpp" />
-
-
-
-
-
 </example>
+
+<references>
+<li>SEI CERT C++ Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/cplusplus/ERR57-CPP.+Do+not+leak+resources+when+handling+exceptions">ERR57-CPP. Do not leak resources when handling exceptions</a>.</li>
+</references>
 </qhelp>

cpp/ql/src/Critical/DescriptorMayNotBeClosed.ql

@@ -1,6 +1,6 @@
 /**
  * @name Open descriptor may not be closed
- * @description A function may return before closing a socket or file that was opened in the function. Closing resources in the same function that opened them ties the lifetime of the resource to that of the function call, making it easier to avoid and detect resource leaks.
+ * @description Failing to close resources in the function that opened them makes it difficult to avoid and detect resource leaks.
  * @kind problem
  * @id cpp/descriptor-may-not-be-closed
  * @problem.severity warning

cpp/ql/src/Critical/DescriptorNeverClosed.qhelp

@@ -6,20 +6,26 @@
 
 <overview>
 <p>
-This rule finds calls to <code>open</code> or <code>socket</code> with no corresponding <code>close</code> call in the entire program.
+This rule finds calls to <code>open</code> or <code>socket</code> where there is no corresponding <code>close</code> call in the program analyzed.
 Leaving descriptors open will cause a resource leak that will persist even after the program terminates.
 </p>
 
 <include src="aliasAnalysisWarning.qhelp" />
-
 </overview>
+
 <recommendation>
 <p>Ensure that all file or socket descriptors allocated by the program are freed before it terminates.</p>
-
 </recommendation>
-<example><sample src="DescriptorNeverClosed.cpp" />
-
 
+<example>
+<p>In the example below, the <code>sockfd</code> socket remains open when the <code>main</code> program finishes. 
+The code should be updated to ensure that the socket is always closed when the program terminates.
+</p>
 
+<sample src="DescriptorNeverClosed.cpp" />
 </example>
+
+<references>
+<li>SEI CERT C++ Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/cplusplus/ERR57-CPP.+Do+not+leak+resources+when+handling+exceptions">ERR57-CPP. Do not leak resources when handling exceptions</a>.</li>
+</references>
 </qhelp>

cpp/ql/src/Critical/DescriptorNeverClosed.ql

@@ -1,6 +1,6 @@
 /**
  * @name Open descriptor never closed
- * @description A function always returns before closing a socket or file that was opened in the function. Closing resources in the same function that opened them ties the lifetime of the resource to that of the function call, making it easier to avoid and detect resource leaks.
+ * @description Functions that always return before closing the socket or file they opened leak resources.
  * @kind problem
  * @id cpp/descriptor-never-closed
  * @problem.severity warning

cpp/ql/src/Critical/InitialisationNotRun.qhelp

@@ -5,23 +5,28 @@
 
 
 <overview>
-<p>The rule finds code that initializes a global variable (i.e. uses it as an L-value) but is never called from <code>main</code>.
+<p>The query finds code that initializes a global variable (that is, uses it as an L-value) but is never called from <code>main</code>.
 Unless there is another entry point that triggers the initialization, the code should be modified so that the variable is initialized.
 Uninitialized variables may contain any value, as 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.
 </p>
 
 <include src="callGraphWarning.qhelp" />
-
 </overview>
+
 <recommendation>
-<p>Examine the code and ensure that the initialization is always run.</p>
+<p>Examine the code and ensure that the variable is always initialized before it is used.</p>
 
 </recommendation>
-<example><sample src="InitialisationNotRun.cpp" />
-
-
-
+<example>
+<p>In the example below, the code that triggers the initialization of <code>g_storage</code> is not run from <code>main</code>. 
+Unless the variable is initialized by another method, the call on line 10 may dereference a null pointer.
+</p>
 
+<sample src="InitialisationNotRun.cpp" />
 </example>
+
+<references>
+<li>C++ reference: <a href="https://en.cppreference.com/book/uninitialized">uninitialized variables</a>.</li>
+</references>
 </qhelp>

cpp/ql/src/Critical/InitialisationNotRun.ql

@@ -1,6 +1,6 @@
 /**
  * @name Initialization code not run
- * @description A global variable has initialization code, but that code is never run (i.e. is called directly or indirectly from main()). Accessing uninitialized variables leads to undefined results.
+ * @description Not running initialization code may lead to unexpected behavior.
  * @kind problem
  * @id cpp/initialization-not-run
  * @problem.severity warning

cpp/ql/src/Critical/LateNegativeTest.qhelp

@@ -6,29 +6,39 @@
 
 <overview>
 <p>
-This rule finds integer values that are first used to index an array and
+This query finds integer values that are first used to index an array and
 subsequently tested for being negative. If it is relevant to perform this test
-at all then it should probably happen <em>before</em> the indexing, not
+at all then it should happen <em>before</em> the indexing, not
 after. Otherwise, if the value is negative then the program will have failed
 before performing the test.
 </p>
-<include src="dataFlowWarning.qhelp" />
 
+<include src="dataFlowWarning.qhelp" />
 </overview>
+
 <recommendation>
 <p>
-See if the value needs checking before being used as array index. Double-check
+See if the value needs to be checked before being used as array index. Double-check
 if the value is derived from user input. If the value clearly cannot be
 negative then the negativity test is redundant and can be removed.
 </p>
-
 </recommendation>
-<example>
-<sample src="LateNegativeTest.cpp" />
-
-
-
 
+<example>
+<p>The example below includes two functions that use the value <code>recordIdx</code> to 
+index an array and a test to verify that the value is positive. 
+The test is made after <code>records</code> is indexed for <code>printRecord</code> and 
+before <code>records</code> is indexed for <code>processRecord</code>. 
+Unless the value of <code>recordIdx</code> cannot be negative, the test should be 
+updated to run before <em>both</em> times the array is indexed. 
+If the value cannot be negative, the test should be removed.
+</p>
 
+<sample src="LateNegativeTest.cpp" />
 </example>
+
+<references>
+<li>cplusplus.com: <a href="http://www.cplusplus.com/doc/tutorial/pointers/">Pointers</a>.</li>
+<li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/ARR30-C.+Do+not+form+or+use+out-of-bounds+pointers+or+array+subscripts">ARR30-C. Do not form or use out-of-bounds pointers or array subscripts</a>.</li>
+</references>
 </qhelp>

cpp/ql/src/Critical/LateNegativeTest.ql

@@ -1,8 +1,8 @@
 /**
  * @name Pointer offset used before it is checked
- * @description A value is used as a pointer offset before it is tested for
- *              being positive/negative. This may mean that an unsuitable
- *              pointer offset will be used before the test occurs.
+ * @description Accessing a pointer or array using an offset before 
+ *              checking if the value is positive 
+ *              may result in unexpected behavior.
  * @kind problem
  * @id cpp/late-negative-test
  * @problem.severity warning

cpp/ql/src/Critical/MissingNegativityTest.qhelp

@@ -6,33 +6,35 @@
 
 <overview>
 <p>
-This rule finds pointer arithmetic expressions that use a value returned from a function before the value is checked to be positive.
-Most pointer arithmetic and almost all array element accesses use a positive value for offsets. A negative value is more likely than not
+This query finds pointer arithmetic expressions that use a value returned from a function without checking that the value is positive.
+Most pointer arithmetic and almost all array element accesses use a positive value for offsets. A negative value is likely to be 
 a defect in the returning function. Checking pointer offsets (particularly if they derive from user input) is necessary to avoid 
 buffer overruns.
 </p>
 
 <p>
-The rules only looks at return values of functions that may return a negative value (not all functions).
+The query looks only at the return values of functions that may return a negative value (not all functions).
 </p>
 
-
 <include src="dataFlowWarning.qhelp" />
-
 </overview>
+
 <recommendation>
 <p>
-Check the function and see whether it needs to check the value to be positive.
+Review the function.  Determine whether it needs to check that the value is positive before performing pointer arithmetic.
 </p>
-
 </recommendation>
-<example>
-<sample src="MissingNegativityTest.cpp" />
-
-
-
-
 
+<example>
+<p>The example below shows an example of this problem. There is no check to ensure that the value of <code>recordIdx</code> 
+is positive and safe to use as an array offset.
+</p>
 
+<sample src="MissingNegativityTest.cpp" />
 </example>
+
+<references>
+<li>cplusplus.com: <a href="http://www.cplusplus.com/doc/tutorial/pointers/">Pointers</a>.</li>
+<li>SEI CERT C Coding Standard: <a href="https://wiki.sei.cmu.edu/confluence/display/c/ARR30-C.+Do+not+form+or+use+out-of-bounds+pointers+or+array+subscripts">ARR30-C. Do not form or use out-of-bounds pointers or array subscripts</a>.</li>
+</references>
 </qhelp>

cpp/ql/src/Critical/MissingNegativityTest.ql

@@ -1,6 +1,7 @@
 /**
  * @name Unchecked return value used as offset
- * @description A return value from a function is used as a pointer offset before it is checked for being positive/negative. Integer values used as pointer offsets should be checked, especially if they are derived from user input.
+ * @description  Using a return value as a pointer offset without checking that the value is positive
+ *               may lead to buffer overruns.
  * @kind problem
  * @id cpp/missing-negativity-test
  * @problem.severity warning