Merge pull request #16939 from geoffw0/docsforautofix

C++: Assorted minor doc improvements

Author: MathiasVP

cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql

@@ -172,5 +172,5 @@ where
   not arg.isFromUninstantiatedTemplate(_) and
   not actual.getUnspecifiedType() instanceof ErroneousType
 select arg,
-  "This argument should be of type '" + expected.getName() + "' but is of type '" +
+  "This format specifier for type '" + expected.getName() + "' does not match the argument type '" +
     actual.getUnspecifiedType().getName() + "'."

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.cpp

@@ -5,22 +5,23 @@
 
 
 <overview>
-<p>This rule finds return statements that return pointers to an object allocated on the stack. 
-The lifetime of a stack allocated memory location only lasts until the function returns, and 
-the contents of that memory become undefined after that. Clearly, using a pointer to stack 
+<p>This rule finds return statements that return pointers to an object allocated on the stack.
+The lifetime of a stack allocated memory location only lasts until the function returns, and
+the contents of that memory become undefined after that. Clearly, using a pointer to stack
 memory after the function has already returned will have undefined results. </p>
 
 </overview>
 <recommendation>
-<p>Use the functions of the <tt>malloc</tt> family to dynamically allocate memory on the heap for data that is used across function calls.</p>
+<p>Use the functions of the <tt>malloc</tt> family, or <tt>new</tt>, to dynamically allocate memory on the heap for data that is used across function calls.</p>
 
 </recommendation>
-<example><sample src="ReturnStackAllocatedMemory.cpp" />
-
-
-
-
+<example>
+<p>The following example allocates an object on the stack and returns a pointer to it. This is incorrect because the object is deallocated
+when the function returns, and the pointer becomes invalid.</p>
+<sample src="ReturnStackAllocatedMemoryBad.cpp" />
 
+<p>To fix this, allocate the object on the heap using <tt>new</tt> and return a pointer to the heap-allocated object.</p>
+<sample src="ReturnStackAllocatedMemoryGood.cpp" />
 </example>
 
 <references>

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.qhelp

@@ -0,0 +1,5 @@
+Record *mkRecord(int value) {
+	Record myRecord(value);
+
+	return &myRecord; // BAD: returns a pointer to `myRecord`, which is a stack-allocated object.
+}

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemoryBad.cpp

@@ -0,0 +1,5 @@
+Record *mkRecord(int value) {
+	Record *myRecord = new Record(value);
+
+	return myRecord; // GOOD: returns a pointer to a `myRecord`, which is a heap-allocated object.
+}

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemoryGood.cpp

@@ -1,5 +1,14 @@
-unsigned limit = get_limit();
-unsigned total = 0;
-while (limit - total > 0) { // wrong: if `total` is greater than `limit` this will underflow and continue executing the loop.
+uint32_t limit = get_limit();
+uint32_t total = 0;
+
+while (limit - total > 0) { // BAD: if `total` is greater than `limit` this will underflow and continue executing the loop.
   total += get_data();
-}
+}
+
+while (total < limit) { // GOOD: never underflows here because there is no arithmetic.
+  total += get_data();
+}
+
+while ((int64_t)limit - total > 0) { // GOOD: never underflows here because the result always fits in an `int64_t`.
+  total += get_data();
+}

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.c

@@ -1,15 +1,17 @@
 char *file_name;
 FILE *f_ptr;
- 
+
 /* Initialize file_name */
- 
+
 f_ptr = fopen(file_name, "w");
 if (f_ptr == NULL)  {
   /* Handle error */
 }
- 
+
 /* ... */
- 
+
 if (chmod(file_name, S_IRUSR) == -1) {
   /* Handle error */
-}
+}
+
+fclose(f_ptr);

cpp/ql/src/Security/CWE/CWE-367/TOCTOUFilesystemRaceBad.c

@@ -1,8 +1,8 @@
 char *file_name;
 int fd;
- 
+
 /* Initialize file_name */
- 
+
 fd = open(
   file_name,
   O_WRONLY | O_CREAT | O_EXCL,
@@ -11,9 +11,11 @@ fd = open(
 if (fd == -1) {
   /* Handle error */
 }
- 
+
 /* ... */
- 
+
 if (fchmod(fd, S_IRUSR) == -1) {
   /* Handle error */
-}
+}
+
+close(fd);

cpp/ql/src/Security/CWE/CWE-367/TOCTOUFilesystemRaceGood.c

@@ -34,7 +34,7 @@ void good1(std::size_t length) noexcept {
 
 // GOOD: the allocation failure is handled appropriately.
 void good2(std::size_t length) noexcept {
-  int* dest = new int[length];
+  int* dest = new(std::nothrow) int[length];
   if(!dest) {
     return;
   }

cpp/ql/src/Security/CWE/CWE-570/IncorrectAllocationErrorHandling.cpp

@@ -1,11 +1,38 @@
 void write_default_config_bad() {
 	// BAD - this is world-writable so any user can overwrite the config
 	int out = creat(OUTFILE, 0666);
-	dprintf(out, DEFAULT_CONFIG);
+	if (out < 0) {
+		// handle error
+	}
+
+	dprintf(out, "%s", DEFAULT_CONFIG);
+	close(out);
 }
 
 void write_default_config_good() {
 	// GOOD - this allows only the current user to modify the file
 	int out = creat(OUTFILE, S_IWUSR | S_IRUSR);
-	dprintf(out, DEFAULT_CONFIG);
+	if (out < 0) {
+		// handle error
+	}
+
+	dprintf(out, "%s", DEFAULT_CONFIG);
+	close(out);
+}
+
+void write_default_config_good_2() {
+	// GOOD - this allows only the current user to modify the file
+	int out = open(OUTFILE, O_WRONLY | O_CREAT, S_IWUSR | S_IRUSR);
+	if (out < 0) {
+		// handle error
+	}
+
+	FILE *fd = fdopen(out, "w");
+	if (fd == NULL) {
+		close(out);
+		// handle error
+	}
+
+	fprintf(fd, "%s", DEFAULT_CONFIG);
+	fclose(fd);
 }