Merge pull request github#4881 from ihsinme/main

CPP: Add query for CWE-401 memory leak on unsuccessful call to realloc function

Author: geoffw0

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

@@ -0,0 +1,20 @@
+// BAD: on unsuccessful call to realloc, we will lose a pointer to a valid memory block
+if (currentSize < newSize)
+{
+	buffer = (unsigned char *)realloc(buffer, newSize);
+}
+
+
+
+// GOOD: this way we will exclude possible memory leak 
+unsigned char * tmp;
+if (currentSize < newSize)
+{
+	tmp = (unsigned char *)realloc(buffer, newSize);
+}
+if (tmp == NULL)
+{
+	free(buffer);
+} 
+else
+	buffer = tmp;

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

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Memory leak on failed call to realloc. 
+The expression <code>mem = realloc (mem, size)</code> is potentially dangerous, if the call fails, we will lose the pointer to the memory block.  
+An unsuccessful call is possible not only when trying to allocate a large amount of memory, but also when the process memory is strongly segmented.</p>
+
+<p>False positives include code in which immediately after calling the realloc function, the pointer is manipulated without first checking for validity. 
+In this case, an exception will occur in the program and it will terminate. 
+But from the point of view of safe coding, these places require the attention of developers.
+At this stage, false positives are also possible in situations where the exception handling is quite complicated and occurs outside the base block in which memory is redistributed.</p>
+
+</overview>
+<recommendation>
+
+<p>We recommend storing the result in a temporary variable and eliminating memory leak.</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates an erroneous and corrected use of the <code>realloc</code> function.</p>
+<sample src="MemoryLeakOnFailedCallToRealloc.c" />
+
+</example>
+<references>
+
+<li>
+  CERT C++ Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM51-CPP.+Properly+deallocate+dynamically+allocated+resources">MEM51-CPP. Properly deallocate dynamically allocated resources</a>.
+</li>
+<li>
+  CERT C Coding Standard:
+  <a href="https://wiki.sei.cmu.edu/confluence/display/c/WIN30-C.+Properly+pair+allocation+and+deallocation+functions">WIN30-C. Properly pair allocation and deallocation functions</a>.
+</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,69 @@
+/**
+ * @name Memory leak on failed call to realloc
+ * @description The expression mem = realloc (mem, size) is potentially dangerous, if the call fails, we will lose the pointer to the memory block.
+ *              We recommend storing the result in a temporary variable and eliminating memory leak.
+ * @kind problem
+ * @id cpp/memory-leak-on-failed-call-to-realloc
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-401
+ */
+
+import cpp
+
+/**
+ * A call to `realloc` of the form `v = realloc(v, size)`, for some variable `v`.
+ */
+class ReallocCallLeak extends FunctionCall {
+  Variable v;
+
+  ReallocCallLeak() {
+    exists(AssignExpr ex, VariableAccess va1, VariableAccess va2 |
+      this.getTarget().hasName("realloc") and
+      this = ex.getRValue() and
+      va1 = ex.getLValue() and
+      va2 = this.getArgument(0) and
+      va1 = v.getAnAccess() and
+      va2 = v.getAnAccess()
+    )
+  }
+
+  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() + "%")
+    )
+  }
+}
+
+from ReallocCallLeak rcl
+where
+  not rcl.isExistsIfWithExitCall() and
+  not rcl.isExistsAssertWithArgumentCall()
+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

@@ -0,0 +1,4 @@
+| test.c:34:29:34:35 | call to realloc | possible loss of original pointer on unsuccessful call realloc |
+| 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 |

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

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-401/MemoryLeakOnFailedCallToRealloc.ql