From d7c784ef2f31bb39cc9418d984bd42c03bb1fb25 Mon Sep 17 00:00:00 2001 From: Mario Campos Date: Thu, 25 Apr 2024 16:29:37 -0500 Subject: [PATCH 1/4] Initial commit of experimental query cpp/guarded-free. --- .../Best Practices/GuardedFree.cpp | 11 +++++ .../Best Practices/GuardedFree.qhelp | 18 +++++++ .../Best Practices/GuardedFree.ql | 48 +++++++++++++++++++ 3 files changed, 77 insertions(+) create mode 100644 cpp/ql/src/experimental/Best Practices/GuardedFree.cpp create mode 100644 cpp/ql/src/experimental/Best Practices/GuardedFree.qhelp create mode 100644 cpp/ql/src/experimental/Best Practices/GuardedFree.ql diff --git a/cpp/ql/src/experimental/Best Practices/GuardedFree.cpp b/cpp/ql/src/experimental/Best Practices/GuardedFree.cpp new file mode 100644 index 000000000000..5242e3da8f5e --- /dev/null +++ b/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); +} \ No newline at end of file diff --git a/cpp/ql/src/experimental/Best Practices/GuardedFree.qhelp b/cpp/ql/src/experimental/Best Practices/GuardedFree.qhelp new file mode 100644 index 000000000000..77fdd4670008 --- /dev/null +++ b/cpp/ql/src/experimental/Best Practices/GuardedFree.qhelp @@ -0,0 +1,18 @@ + + + +

The free 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 free. As such, these guards may hinder performance and readability.

+ + +

A function call to free should not depend upon the value of its argument. Delete the if condition preceeding a function call to free when its only purpose is to check the value of the pointer to be freed.

+ + + + + +
  • + The Open Group Base Specifications Issue 7, 2018 Edition: + free - free allocated memory +
    • +     +     \ No newline at end of file diff --git a/cpp/ql/src/experimental/Best Practices/GuardedFree.ql b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql new file mode 100644 index 000000000000..ec8f39c491d7 --- /dev/null +++ b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql @@ -0,0 +1,48 @@ +/** + * @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 + +class FreeCall extends FunctionCall { + FreeCall() { this.getTarget().hasName("free") } +} + +from IfStmt stmt, FreeCall fc, Variable v +where + stmt.getThen() = fc.getEnclosingStmt() and + ( + stmt.getCondition() = v.getAnAccess() and + fc.getArgument(0) = v.getAnAccess() + or + exists(PointerDereferenceExpr cond, PointerDereferenceExpr arg | + fc.getArgument(0) = arg and + stmt.getCondition() = cond and + cond.getOperand+() = v.getAnAccess() and + arg.getOperand+() = v.getAnAccess() + ) + or + exists(ArrayExpr cond, ArrayExpr arg | + fc.getArgument(0) = arg and + stmt.getCondition() = cond and + cond.getArrayBase+() = v.getAnAccess() and + arg.getArrayBase+() = v.getAnAccess() + ) + or + exists(NEExpr eq | + fc.getArgument(0) = v.getAnAccess() and + stmt.getCondition() = eq and + eq.getAnOperand() = v.getAnAccess() and + eq.getAnOperand().getValue() = "0" + ) + ) +select stmt, "unnecessary NULL check before call to $@", fc, "free" From 3195f0c8288fd00af329ad25a0043e069a376c40 Mon Sep 17 00:00:00 2001 From: Mario Campos Date: Fri, 26 Apr 2024 09:10:40 -0500 Subject: [PATCH 2/4] Use more specific `hasGlobalName()` for stdlib function free(3) Based on the CodeQL documentation's example of strncpy(3) and strlen(3): https://codeql.github.com/docs/codeql-language-guides/hash-consing-and-value-numbering/#example-query --- cpp/ql/src/experimental/Best Practices/GuardedFree.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Best Practices/GuardedFree.ql b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql index ec8f39c491d7..07d279271592 100644 --- a/cpp/ql/src/experimental/Best Practices/GuardedFree.ql +++ b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql @@ -14,7 +14,7 @@ import cpp class FreeCall extends FunctionCall { - FreeCall() { this.getTarget().hasName("free") } + FreeCall() { this.getTarget().hasGlobalName("free") } } from IfStmt stmt, FreeCall fc, Variable v From c480431ec0aa5f67fcb472b8a58430f012090795 Mon Sep 17 00:00:00 2001 From: Mario Campos Date: Wed, 1 May 2024 10:59:16 -0500 Subject: [PATCH 3/4] C++: simplify cpp/guarded-free This new form is more declarative by use of the `GuardCondition`. Thanks to the tireless effort of @MathiasVP! --- .../Best Practices/GuardedFree.ql | 33 +++---------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/cpp/ql/src/experimental/Best Practices/GuardedFree.ql b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql index 07d279271592..1316c38143b0 100644 --- a/cpp/ql/src/experimental/Best Practices/GuardedFree.ql +++ b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql @@ -17,32 +17,9 @@ class FreeCall extends FunctionCall { FreeCall() { this.getTarget().hasGlobalName("free") } } -from IfStmt stmt, FreeCall fc, Variable v +from GuardCondition gc, FreeCall fc, Variable v, BasicBlock bb where - stmt.getThen() = fc.getEnclosingStmt() and - ( - stmt.getCondition() = v.getAnAccess() and - fc.getArgument(0) = v.getAnAccess() - or - exists(PointerDereferenceExpr cond, PointerDereferenceExpr arg | - fc.getArgument(0) = arg and - stmt.getCondition() = cond and - cond.getOperand+() = v.getAnAccess() and - arg.getOperand+() = v.getAnAccess() - ) - or - exists(ArrayExpr cond, ArrayExpr arg | - fc.getArgument(0) = arg and - stmt.getCondition() = cond and - cond.getArrayBase+() = v.getAnAccess() and - arg.getArrayBase+() = v.getAnAccess() - ) - or - exists(NEExpr eq | - fc.getArgument(0) = v.getAnAccess() and - stmt.getCondition() = eq and - eq.getAnOperand() = v.getAnAccess() and - eq.getAnOperand().getValue() = "0" - ) - ) -select stmt, "unnecessary NULL check before call to $@", fc, "free" + 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" From 5a7a1dc92eead406994d97320dd888f7f03e7514 Mon Sep 17 00:00:00 2001 From: Mario Campos Date: Wed, 1 May 2024 11:00:19 -0500 Subject: [PATCH 4/4] C++: forgot to import semmle.code.cpp.controlflow.Guards --- cpp/ql/src/experimental/Best Practices/GuardedFree.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/ql/src/experimental/Best Practices/GuardedFree.ql b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql index 1316c38143b0..2d504d9bc057 100644 --- a/cpp/ql/src/experimental/Best Practices/GuardedFree.ql +++ b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql @@ -12,6 +12,7 @@ */ import cpp +import semmle.code.cpp.controlflow.Guards class FreeCall extends FunctionCall { FreeCall() { this.getTarget().hasGlobalName("free") }