Merge pull request #16331 from mario-campos/mario-campos/guarded-free

Cpp: new experimental query cpp/guarded-free

Author: MathiasVP

cpp/ql/src/experimental/Best Practices/GuardedFree.cpp

@@ -0,0 +1,11 @@
+void test()
+{
+    char *foo = malloc(100);
+
+    // BAD
+    if (foo)          
+        free(foo);
+
+    // GOOD
+    free(foo);
+}

cpp/ql/src/experimental/Best Practices/GuardedFree.qhelp

@@ -0,0 +1,18 @@
+<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The <code>free</code> function, which deallocates heap memory, may accept a NULL pointer and take no action. Therefore, it is unnecessary to check its argument for the value of NULL before a function call to <code>free</code>. As such, these guards may hinder performance and readability.</p>
+</overview>
+<recommendation>
+<p>A function call to <code>free</code> should not depend upon the value of its argument. Delete the <code>if</code> condition preceeding a function call to <code>free</code> when its only purpose is to check the value of the pointer to be freed.</p>
+</recommendation>
+<example>
+<sample src = "GuardedFree.cpp" />
+</example>
+<references>
+<li>
+    The Open Group Base Specifications Issue 7, 2018 Edition:
+    <a href="https://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html">free - free allocated memory</a>
+</li>
+</references>
+</qhelp>

cpp/ql/src/experimental/Best Practices/GuardedFree.ql

@@ -0,0 +1,26 @@
+/**
+ * @name Guarded Free
+ * @description NULL-condition guards before function calls to the memory-deallocation
+ *              function free(3) are unnecessary, because passing NULL to free(3) is a no-op.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision very-high
+ * @id cpp/guarded-free
+ * @tags maintainability
+ *       readability
+ *       experimental
+ */
+
+import cpp
+import semmle.code.cpp.controlflow.Guards
+
+class FreeCall extends FunctionCall {
+  FreeCall() { this.getTarget().hasGlobalName("free") }
+}
+
+from GuardCondition gc, FreeCall fc, Variable v, BasicBlock bb
+where
+  gc.ensuresEq(v.getAnAccess(), 0, bb, false) and
+  fc.getArgument(0) = v.getAnAccess() and
+  bb = fc.getEnclosingStmt()
+select gc, "unnecessary NULL check before call to $@", fc, "free"