Merge pull request #4979 from geoffw0/cpp401

C++: Improvements to experimental query cpp/memory-leak-on-failed-call-to-realloc

Author: MathiasVP

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

@@ -2,3 +2,5 @@
 | test.c:63:29:63:35 | call to realloc | possible loss of original pointer on unsuccessful call realloc |
 | test.c:139:29:139:35 | call to realloc | possible loss of original pointer on unsuccessful call realloc |
 | 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 |

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

@@ -2,7 +2,7 @@
 #define NULL ((void*)0)
 
 #define assert(x) if (!(x)) __assert_fail(#x,__FILE__,__LINE__)
-void __assert_fail(const char *assertion, const char *file, int line) {  }
+void __assert_fail(const char *assertion, const char *file, int line);
 
 void aFakeFailed_1(int file, int line)
 {
@@ -272,3 +272,50 @@ unsigned char * noBadResize_2_7(unsigned char * buffer,size_t currentSize,size_t
 	myASSERT_2(buffer);
 	return buffer;
 }
+
+unsigned char *goodResize_3_1(unsigned char *buffer, size_t currentSize, size_t newSize)
+{
+	// GOOD: this way we will exclude possible memory leak [FALSE POSITIVE]
+	unsigned char *tmp = buffer;
+	if (currentSize < newSize)
+	{
+		buffer = (unsigned char *)realloc(buffer, newSize);
+		if (buffer == NULL)
+		{
+			free(tmp);
+			return NULL;
+		} 
+	}
+
+	return buffer;
+}
+
+unsigned char *goodResize_3_2(unsigned char *buffer, size_t currentSize, size_t newSize)
+{
+	// GOOD: this way we will exclude possible memory leak [FALSE POSITIVE]
+	unsigned char *tmp = buffer;
+	if (currentSize < newSize)
+	{
+		tmp = (unsigned char *)realloc(tmp, newSize);
+		if (tmp != 0)
+		{
+			buffer = tmp;
+		} 
+	}
+
+	return buffer;
+}
+
+void abort(void);
+
+unsigned char *noBadResize_4_1(unsigned char *buffer, size_t currentSize, size_t newSize)
+{
+	// GOOD: program to end
+	if (currentSize < newSize)
+	{
+		if (buffer = (unsigned char *)realloc(buffer, newSize))
+			abort();
+	}
+
+	return buffer;
+}