Merge pull request github#5804 from MathiasVP/improve-detect-and-handle-memory-allocation-errors

C++: Improve qhelp and tests for cpp/detect-and-handle-memory-allocation-errors

Author: geoffw0

cpp/ql/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.cpp

@@ -1,35 +1,43 @@
-// BAD: on memory allocation error, the program terminates.
-void badFunction(const int *source, std::size_t length) noexcept {
-  int * dest = new int[length];
+// BAD: the allocation will throw an unhandled exception
+// instead of returning a null pointer.
+void bad1(std::size_t length) noexcept {
+  int* dest = new int[length];
+  if(!dest) {
+    return;
+  }
   std::memset(dest, 0, length);
-// ..
+  // ...
 }
-// GOOD: memory allocation error will be handled.
-void goodFunction(const int *source, std::size_t length) noexcept {
+
+// BAD: the allocation won't throw an exception, but
+// instead return a null pointer.
+void bad2(std::size_t length) noexcept {
   try {
-       int * dest = new int[length];
-  } catch(std::bad_alloc) {
+    int* dest = new(std::nothrow) int[length];
+    std::memset(dest, 0, length);
+    // ...
+  } catch(std::bad_alloc&) {
     // ...
   }
-  std::memset(dest, 0, length);
-// ..
 }
-// BAD: memory allocation error will not be handled.
-void badFunction(const int *source, std::size_t length) noexcept {
+
+// GOOD: the allocation failure is handled appropiately.
+void good1(std::size_t length) noexcept {
   try {
-       int * dest = new (std::nothrow) int[length];
-  } catch(std::bad_alloc) {
+    int* dest = new int[length];
+    std::memset(dest, 0, length);
+    // ...
+  } catch(std::bad_alloc&) {
     // ...
   }
-  std::memset(dest, 0, length);
-// ..
 }
-// GOOD: memory allocation error will be handled.
-void goodFunction(const int *source, std::size_t length) noexcept {
-  int * dest = new (std::nothrow) int[length];
-  if (!dest) {
-      return;
+
+// GOOD: the allocation failure is handled appropiately.
+void good2(std::size_t length) noexcept {
+  int* dest = new int[length];
+  if(!dest) {
+    return;
   }
   std::memset(dest, 0, length);
-// ..
+  // ...
 }

cpp/ql/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.qhelp

@@ -3,16 +3,20 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p>When using the <code>new</code> operator to allocate memory, you need to pay attention to the different ways of detecting errors. <code>::operator new(std::size_t)</code> throws an exception on error, whereas <code>::operator new(std::size_t, const std::nothrow_t &amp;)</code> returns zero on error. The programmer can get confused and check the error that occurs when allocating memory incorrectly. That can lead to an unhandled program termination or to a violation of the program logic.</p>
+<p>Different overloads of the <code>new</code> operator handle allocation failures in different ways.
+If <code>new T</code> fails for some type <code>T</code>, it throws a <code>std::bad_alloc</code> exception,
+but <code>new(std::nothrow) T</code> returns a null pointer. If the programmer does not use the corresponding
+method of error handling, allocation failure may go unhandled and could cause the program to behave in
+unexpected ways.</p>
 
 </overview>
 <recommendation>
 
-<p>Use the correct error detection method corresponding with the memory allocation.</p>
+<p>Make sure that exceptions are handled appropriately if <code>new T</code> is used. On the other hand,
+make sure to handle the possibility of null pointers if <code>new(std::nothrow) T</code> is used.</p>
 
 </recommendation>
 <example>
-<p>The following example demonstrates various approaches to detecting memory allocation errors using the <code>new</code> operator.</p>
 <sample src="WrongInDetectingAndHandlingMemoryAllocationErrors.cpp" />
 
 </example>

cpp/ql/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql

@@ -1,7 +1,6 @@
 /**
  * @name Detect And Handle Memory Allocation Errors
- * @description --::operator new(std::size_t) throws an exception on error, and ::operator new(std::size_t, const std::nothrow_t &) returns zero on error.
- *              --the programmer can get confused when check the error that occurs when allocating memory incorrectly.
+ * @description `operator new` throws an exception on allocation failures, while `operator new(std::nothrow)` returns a null pointer. Mixing up these two failure conditions can result in unexpected behavior.
  * @kind problem
  * @id cpp/detect-and-handle-memory-allocation-errors
  * @problem.severity warning

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-570/semmle/tests/WrongInDetectingAndHandlingMemoryAllocationErrors.expected

@@ -1,5 +1,9 @@
-| test.cpp:30:15:30:26 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:38:9:38:20 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:50:13:50:38 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:51:22:51:47 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:53:18:53:43 | call to operator new[] | memory allocation error check is incorrect or missing |
+| test.cpp:29:13:29:24 | call to operator new[] | memory allocation error check is incorrect or missing |
+| test.cpp:37:13:37:24 | call to operator new[] | memory allocation error check is incorrect or missing |
+| test.cpp:41:13:41:24 | call to operator new[] | memory allocation error check is incorrect or missing |
+| test.cpp:49:8:49:19 | call to operator new[] | memory allocation error check is incorrect or missing |
+| test.cpp:58:8:58:19 | call to operator new[] | memory allocation error check is incorrect or missing |
+| test.cpp:63:8:63:19 | call to operator new[] | memory allocation error check is incorrect or missing |
+| test.cpp:92:5:92:31 | call to operator new[] | memory allocation error check is incorrect or missing |
+| test.cpp:93:15:93:41 | call to operator new[] | memory allocation error check is incorrect or missing |
+| test.cpp:96:10:96:36 | call to operator new[] | memory allocation error check is incorrect or missing |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-570/semmle/tests/test.cpp

@@ -1,97 +1,137 @@
-#define NULL ((void*)0)
+#define NULL ((void *)0)
+
+namespace std {
+struct nothrow_t {};
+typedef unsigned long size_t;
+
 class exception {};
+class bad_alloc : public exception {};
 
-namespace std{
-  struct nothrow_t {};
-  typedef unsigned long size_t;
-  class bad_alloc{
-    const char* what() const throw();
-  };
-  extern const std::nothrow_t nothrow;
-}
+extern const std::nothrow_t nothrow;
+} // namespace std
 
 using namespace std;
 
-void* operator new(std::size_t _Size);
-void* operator new[](std::size_t _Size);
-void* operator new( std::size_t count, const std::nothrow_t& tag ) noexcept;
-void* operator new[]( std::size_t count, const std::nothrow_t& tag ) noexcept;
-
-void badNew_0_0()
-{
-    while (true) {
-        new int[100]; // BAD [NOT DETECTED]
-        if(!(new int[100])) // BAD [NOT DETECTED]
-            return;
-    }
-}
-void badNew_0_1()
-{
-    int * i = new int[100]; // BAD
-    if(i == 0)
-        return;
-    if(!i)
-        return;
-    if(i == NULL)
-        return;
-    int * j;
-    j = new int[100]; // BAD
-    if(j == 0)
-        return;
-    if(!j)
-        return;
-    if(j == NULL)
-        return;
+void *operator new(std::size_t);
+void *operator new[](std::size_t);
+void *operator new(std::size_t, const std::nothrow_t &) noexcept;
+void *operator new[](std::size_t, const std::nothrow_t &) noexcept;
+
+void bad_new_in_condition() {
+  if (!(new int)) { // BAD [NOT DETECTED]
+    return;
+  }
 }
-void badNew_1_0()
-{
-    try {
-        while (true) {
-            new(std::nothrow) int[100]; // BAD
-            int* p = new(std::nothrow) int[100]; // BAD
-            int* p1;
-            p1 = new(std::nothrow) int[100]; // BAD
-        }
-    } catch (const exception &){//const std::bad_alloc& e) {
-//        std::cout << e.what() << '\n';
-    }
+
+void foo(int**);
+
+void bad_new_missing_exception_handling() {
+  int *p1 = new int[100]; // BAD
+  if (p1 == 0)
+    return;
+
+  int *p2 = new int[100]; // BAD [NOT DETECTED]
+  if (!p2)
+    return;
+
+  int *p3 = new int[100]; // BAD
+  if (p3 == NULL)
+    return;
+
+  int *p4 = new int[100]; // BAD
+  if (p4 == nullptr)
+    return;
+
+  int *p5 = new int[100]; // BAD [NOT DETECTED]
+  if (p5) {} else return;
+
+  int *p6;
+  p6 = new int[100]; // BAD
+  if (p6 == 0) return;
+
+  int *p7;
+  p7 = new int[100]; // BAD [NOT DETECTED]
+  if (!p7)
+    return;
+
+  int *p8;
+  p8 = new int[100]; // BAD
+  if (p8 == NULL)
+    return;
+
+  int *p9;
+  p9 = new int[100]; // BAD
+  if (p9 != nullptr) {
+  } else
+    return;
+
+  int *p10;
+  p10 = new int[100]; // BAD [NOT DETECTED]
+  if (p10 != 0) {
+  }
+
+  int *p11;
+  do {
+    p11 = new int[100]; // BAD [NOT DETECTED]
+  } while (!p11);
+
+  int* p12 = new int[100];
+  foo(&p12);
+  if(p12) {} else return; // GOOD: p12 is probably modified in foo, so it's
+                          // not the return value of the new that's checked.
+
+  int* p13 = new int[100];
+  foo(&p13);
+  if(!p13) {
+      return;
+  } else { }; // GOOD: same as above.
 }
-void badNew_1_1()
-{
-    while (true) {
-        int* p = new(std::nothrow) int[100]; // BAD [NOT DETECTED]
-        new(std::nothrow) int[100]; // BAD [NOT DETECTED]
-    }
+
+void bad_new_nothrow_in_exception_body() {
+  try {
+    new (std::nothrow) int[100];           // BAD
+    int *p1 = new (std::nothrow) int[100]; // BAD
+
+    int *p2;
+    p2 = new (std::nothrow) int[100]; // BAD
+  } catch (const std::bad_alloc &) {
+  }
 }
 
-void goodNew_0_0()
-{
-    try {
-        while (true) {
-            new int[100]; // GOOD
-        }
-    } catch (const exception &){//const std::bad_alloc& e) {
-//        std::cout << e.what() << '\n';
-    }
+void good_new_has_exception_handling() {
+  try {
+    int *p1 = new int[100]; // GOOD
+  } catch (...) {
+  }
 }
 
-void goodNew_1_0()
-{
-    while (true) {
-        int* p = new(std::nothrow) int[100]; // GOOD
-        if (p == nullptr) {
-//            std::cout << "Allocation returned nullptr\n";
-            break;
-        }
-        int* p1;
-        p1 = new(std::nothrow) int[100]; // GOOD
-        if (p1 == nullptr) {
-//            std::cout << "Allocation returned nullptr\n";
-            break;
-        }
-        if (new(std::nothrow) int[100] == nullptr) { // GOOD
-//            std::cout << "Allocation returned nullptr\n";
-            break;
-        }
-    }
+void good_new_handles_nullptr() {
+  int *p1 = new (std::nothrow) int[100]; // GOOD
+  if (p1 == nullptr)
+    return;
+
+  int *p2;
+  p2 = new (std::nothrow) int[100]; // GOOD
+  if (p2 == nullptr)
+    return;
+
+  int *p3;
+  p3 = new (std::nothrow) int[100]; // GOOD
+  if (p3 != nullptr) {
+  }
+
+  int *p4;
+  p4 = new (std::nothrow) int[100]; // GOOD
+  if (p4) {
+  } else
+    return;
+
+  int *p5;
+  p5 = new (std::nothrow) int[100]; // GOOD
+  if (p5 != nullptr) {
+  } else
+    return;
+
+  if (new (std::nothrow) int[100] == nullptr)
+    return; // GOOD
 }