diff --git a/change-notes/1.20/analysis-cpp.md b/change-notes/1.20/analysis-cpp.md index 249540a1942c..61de213d50e8 100644 --- a/change-notes/1.20/analysis-cpp.md +++ b/change-notes/1.20/analysis-cpp.md @@ -15,6 +15,6 @@ |----------------------------|------------------------|------------------------------------------------------------------| | 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. | | 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 diff --git a/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql b/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql index 8f04625c508d..5960fa5de799 100644 --- a/cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql +++ b/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. ) } @@ -218,16 +227,21 @@ predicate leakedInSameMethod(Resource r, Expr acquire) { fc.getQualifier() = r.getAnAccess() or // e.g. `r->setOwner(this)` 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 diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected index 84f1a769b72c..e55ef3e3448f 100644 --- a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected +++ b/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. | diff --git a/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/Lambda.cpp b/cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/Lambda.cpp new file mode 100644 index 000000000000..1b3233c52718 --- /dev/null +++ b/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)(); +};