Merge pull request #672 from geoffw0/lgtm1605

CPP: Fix function pointer/lambda related false positives in 'Resource not released in destructor'

Author: jbj

change-notes/1.20/analysis-cpp.md

@@ -18,7 +18,7 @@
 | Suspicious pointer scaling (`cpp/suspicious-pointer-scaling`) | Fewer false positives | False positives involving types that are not uniquely named in the snapshot have been fixed. |
 | Lossy function result cast (`cpp/lossy-function-result-cast`) | Fewer false positive results | The whitelist of rounding functions built into this query has been expanded. |
 | Unused static variable (`cpp/unused-static-variable`) | Fewer false positive results | Variables with the attribute `unused` are now excluded from the query. |
-| Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Fix false positives where a resource is released via a virtual method call. |
+| Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Fix false positives where a resource is released via a virtual method call, function pointer, or lambda. |
 
 ## Changes to QL libraries
 

cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql

@@ -140,12 +140,12 @@ class Resource extends MemberVariable {
     )
   }
 
-  predicate acquisitionWithRequiredRelease(Assignment acquireAssign, string kind) {
+  predicate acquisitionWithRequiredKind(Assignment acquireAssign, string kind) {
     // acquireAssign is an assignment to this resource
     acquireAssign.(Assignment).getLValue() = this.getAnAccess() and
     // Should be in this class, but *any* member method will do
     this.inSameClass(acquireAssign) and
-    // Check that it is an acquisition function and return the corresponding free
+    // Check that it is an acquisition function and return the corresponding kind
     acquireExpr(acquireAssign.getRValue(), kind)
   }
 
@@ -158,15 +158,22 @@ predicate unreleasedResource(Resource r, Expr acquire, File f, int acquireLine)
     // Note: there could be several release functions, because there could be
     // several functions called 'fclose' for example. We want to check that
     // *none* of these functions are called to release the resource
-    r.acquisitionWithRequiredRelease(acquire, _) and
-    not exists(Expr releaseExpr, string releaseName |
-         r.acquisitionWithRequiredRelease(acquire, releaseName) and
-         releaseExpr = r.getAReleaseExpr(releaseName) and
+    r.acquisitionWithRequiredKind(acquire, _) and
+    not exists(Expr releaseExpr, string kind |
+         r.acquisitionWithRequiredKind(acquire, kind) and
+         releaseExpr = r.getAReleaseExpr(kind) and
          r.inDestructor(releaseExpr)
     )
     and f = acquire.getFile()
     and acquireLine = acquire.getLocation().getStartLine()
 
+    and not exists(ExprCall exprCall |
+      // expression call (function pointer or lambda) with `r` as an
+      // argument, which could release it.
+      exprCall.getAnArgument() = r.getAnAccess() and
+      r.inDestructor(exprCall)
+    )
+
     // check that any destructor for this class has a block; if it doesn't,
     // we must be missing information.
     and forall(Class c, Destructor d |
@@ -181,10 +188,12 @@ predicate unreleasedResource(Resource r, Expr acquire, File f, int acquireLine)
 
 predicate freedInSameMethod(Resource r, Expr acquire) {
   unreleasedResource(r, acquire, _, _) and
-  exists(Expr releaseExpr, string releaseName |
-    r.acquisitionWithRequiredRelease(acquire, releaseName) and
-    releaseExpr = r.getAReleaseExpr(releaseName) and
-    releaseExpr.getEnclosingFunction() = acquire.getEnclosingFunction()
+  exists(Expr releaseExpr, string kind |
+    r.acquisitionWithRequiredKind(acquire, kind) and
+    releaseExpr = r.getAReleaseExpr(kind) and
+    releaseExpr.getEnclosingFunction().getEnclosingAccessHolder*() = acquire.getEnclosingFunction()
+    	// here, `getEnclosingAccessHolder*` allows us to go from a nested function or lambda
+    	// expression to the class method enclosing it.
   )
 }
 
@@ -221,16 +230,21 @@ predicate leakedInSameMethod(Resource r, Expr acquire) {
           fc = acquire.getAChild*() // e.g. `r = new MyClass(this)`
         )
       )
+    ) or exists(FunctionAccess fa, string kind |
+      // the address of a function that releases `r` is taken (and likely
+      // used to release `r` at some point).
+      r.acquisitionWithRequiredKind(acquire, kind) and
+      fa.getTarget() = r.getAReleaseExpr(kind).getEnclosingFunction()
     )
   )
 }
 
 pragma[noopt] predicate badRelease(Resource r, Expr acquire, Function functionCallingRelease, int line) {
   unreleasedResource(r, acquire, _, _) and
-  exists(Expr releaseExpr, string releaseName,
+  exists(Expr releaseExpr, string kind,
          Location releaseExprLocation, Function acquireFunction |
-    r.acquisitionWithRequiredRelease(acquire, releaseName) and
-    releaseExpr = r.getAReleaseExpr(releaseName) and
+    r.acquisitionWithRequiredKind(acquire, kind) and
+    releaseExpr = r.getAReleaseExpr(kind) and
     releaseExpr.getEnclosingFunction() = functionCallingRelease and
     functionCallingRelease.getDeclaringType() = r.getDeclaringType() and
     releaseExprLocation = releaseExpr.getLocation() and

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected

@@ -10,6 +10,7 @@
 | DeleteThis.cpp:60:3:60:24 | ... = ... | Resource ptr14 is acquired by class MyClass3 but not released anywhere in this class. |
 | DeleteThis.cpp:127:3:127:20 | ... = ... | Resource d is acquired by class MyClass9 but not released anywhere in this class. |
 | ExternalOwners.cpp:49:3:49:20 | ... = ... | Resource a is acquired by class MyScreen but not released anywhere in this class. |
+| Lambda.cpp:24:3:24:21 | ... = ... | Resource r4 is acquired by class testLambda but not released anywhere in this class. |
 | ListDelete.cpp:21:3:21:21 | ... = ... | Resource first is acquired by class MyThingColection but not released anywhere in this class. |
 | NoDestructor.cpp:23:3:23:20 | ... = ... | Resource n is acquired by class MyClass5 but not released anywhere in this class. |
 | PlacementNew.cpp:36:3:36:36 | ... = ... | Resource p1 is acquired by class MyTestForPlacementNew but not released anywhere in this class. |

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/Lambda.cpp

@@ -0,0 +1,56 @@
+
+class testLambda
+{
+public:
+	testLambda()
+	{
+		r1 = new char[4096]; // GOOD
+		deleter1 = [](char *r) {
+			delete [] r;
+		};
+
+		r2 = new char[4096]; // GOOD
+		auto deleter2 = [this]() {
+			delete [] r2;
+		};
+		deleter2();
+
+		r3 = new char[4096]; // GOOD
+		auto deleter3 = [&r = r3]() {
+			delete [] r;
+		};
+		deleter3(); 
+
+		r4 = new char[4096]; // BAD
+
+		r5 = new char[4096]; // GOOD
+		deleter5 = &deleter_for_r5;
+
+		r6 = new char[4096]; // GOOD
+		deleter6 = &testLambda::deleter_for_r6;
+	}
+
+	static void deleter_for_r5(char *r)
+	{
+		delete [] r;
+	}
+
+	void deleter_for_r6()
+	{
+		delete [] r6;
+	}
+
+	~testLambda()
+	{
+		deleter1(r1);
+		deleter5(r5);
+		((*this).*deleter6)();
+	}
+
+private:
+	char *r1, *r2, *r3, *r4, *r5, *r6;
+
+	void (*deleter1)(char *r);
+	void (*deleter5)(char *r);
+	void (testLambda::*deleter6)();
+};