C++: General solution for functions that may exit.

Author: geoffw0

cpp/ql/src/experimental/Security/CWE/CWE-401/MemoryLeakOnFailedCallToRealloc.ql

@@ -13,6 +13,19 @@
 
 import cpp
 
+/**
+ * A function call that potentially does not return (such as `exit`).
+ */
+class CallMayNotReturn extends FunctionCall {
+  CallMayNotReturn() {
+    // call that is known to not return
+    not exists(this.(ControlFlowNode).getASuccessor())
+    or
+    // call to another function that may not return
+    exists(CallMayNotReturn exit | getTarget() = exit.getEnclosingFunction())
+  }
+}
+
 /**
  * A call to `realloc` of the form `v = realloc(v, size)`, for some variable `v`.
  */
@@ -30,40 +43,15 @@ class ReallocCallLeak extends FunctionCall {
     )
   }
 
-  predicate isExistsIfWithExitCall() {
-    exists(IfStmt ifc |
-      this.getArgument(0) = v.getAnAccess() and
-      ifc.getCondition().getAChild*() = v.getAnAccess() and
-      ifc.getEnclosingFunction() = this.getEnclosingFunction() and
-      ifc.getLocation().getStartLine() >= this.getArgument(0).getLocation().getStartLine() and
-      exists(FunctionCall fc |
-        fc.getTarget().hasName("exit") and
-        fc.getEnclosingFunction() = this.getEnclosingFunction() and
-        (ifc.getThen().getAChild*() = fc or ifc.getElse().getAChild*() = fc)
-      )
-      or
-      exists(FunctionCall fc, FunctionCall ftmp1, FunctionCall ftmp2 |
-        ftmp1.getTarget().hasName("exit") and
-        ftmp2.(ControlFlowNode).getASuccessor*() = ftmp1 and
-        fc = ftmp2.getEnclosingFunction().getACallToThisFunction() and
-        fc.getEnclosingFunction() = this.getEnclosingFunction() and
-        (ifc.getThen().getAChild*() = fc or ifc.getElse().getAChild*() = fc)
-      )
-    )
-  }
-
-  predicate isExistsAssertWithArgumentCall() {
-    exists(FunctionCall fc |
-      fc.getTarget().hasName("__assert_fail") and
-      this.getEnclosingFunction() = fc.getEnclosingFunction() and
-      fc.getLocation().getStartLine() > this.getArgument(0).getLocation().getEndLine() and
-      fc.getArgument(0).toString().matches("%" + this.getArgument(0).toString() + "%")
-    )
+  /**
+   * Holds if failure of this allocation may be handled by termination, for
+   * example a call to `exit()`.
+   */
+  predicate mayHandleByTermination() {
+    this.(ControlFlowNode).getASuccessor*() instanceof CallMayNotReturn
   }
 }
 
 from ReallocCallLeak rcl
-where
-  not rcl.isExistsIfWithExitCall() and
-  not rcl.isExistsAssertWithArgumentCall()
+where not rcl.mayHandleByTermination()
 select rcl, "possible loss of original pointer on unsuccessful call realloc"

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-401/semmle/tests/MemoryLeakOnFailedCallToRealloc.expected

@@ -4,4 +4,3 @@
 | test.c:186:29:186:35 | call to realloc | possible loss of original pointer on unsuccessful call realloc |
 | test.c:282:29:282:35 | call to realloc | possible loss of original pointer on unsuccessful call realloc |
 | test.c:299:26:299:32 | call to realloc | possible loss of original pointer on unsuccessful call realloc |
-| test.c:316:33:316:39 | call to realloc | possible loss of original pointer on unsuccessful call realloc |

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

@@ -310,7 +310,7 @@ void abort(void);
 
 unsigned char *noBadResize_4_1(unsigned char *buffer, size_t currentSize, size_t newSize)
 {
-	// GOOD: program to end [FALSE POSITIVE]
+	// GOOD: program to end
 	if (currentSize < newSize)
 	{
 		if (buffer = (unsigned char *)realloc(buffer, newSize))