From 2f619a4a82e025154b03fdbf86a77931041a1445 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Thu, 20 Feb 2025 00:26:53 -0800 Subject: [PATCH 1/5] Fix performance issue in ResourceLeakAnalysis.qll --- .../codingstandards/cpp/resources/ResourceLeakAnalysis.qll | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll b/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll index 7d767b5cb..d1c1a369e 100644 --- a/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll +++ b/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll @@ -1,6 +1,6 @@ import cpp import semmle.code.cpp.dataflow.DataFlow -import semmle.code.cpp.valuenumbering.GlobalValueNumbering +import semmle.code.cpp.valuenumbering.HashCons import semmle.code.cpp.controlflow.Dominance import codeql.util.Boolean @@ -40,13 +40,14 @@ signature module ResourceLeakConfigSig { predicate isFree(ControlFlowNode node, DataFlow::Node resource); + bindingset[node] default DataFlow::Node getAnAlias(DataFlow::Node node) { DataFlow::localFlow(node, result) or exists(Expr current, Expr after | current in [node.asExpr(), node.asDefiningArgument()] and after in [result.asExpr(), result.asDefiningArgument()] and - globalValueNumber(current) = globalValueNumber(after) and + hashCons(current) = hashCons(after) and strictlyDominates(current, after) ) } From 1c86573cb785615a5e050b4556dc98b1209df743 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Thu, 20 Feb 2025 17:48:08 -0800 Subject: [PATCH 2/5] Remove getAnAliasRecursive() --- c/misra/test/rules/RULE-22-16/test.c | 8 ++++++++ .../cpp/resources/ResourceLeakAnalysis.qll | 15 +-------------- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/c/misra/test/rules/RULE-22-16/test.c b/c/misra/test/rules/RULE-22-16/test.c index 00764645a..d0d4f6ddc 100644 --- a/c/misra/test/rules/RULE-22-16/test.c +++ b/c/misra/test/rules/RULE-22-16/test.c @@ -104,4 +104,12 @@ void f15(int p) { } mtx_unlock(&m); } +} + +void f16(int p) { + mtx_t* ptr; + mtx_t *ptr_m1 = ptr; + mtx_t *ptr_m2 = ptr; + mtx_lock(ptr_m1); // COMPLIANT[FALSE_POSITIVE] + mtx_unlock(ptr_m2); } \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll b/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll index d1c1a369e..3dd61e934 100644 --- a/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll +++ b/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll @@ -64,19 +64,6 @@ module ResourceLeak { Config::isAllocate(cfgNode, resource) } - /** - * Get an alias of a resource, and aliases of nodes that are aliased by a resource. - */ - private DataFlow::Node getAnAliasRecursive(DataFlow::Node node) { - result = Config::getAnAlias(node) and - Config::isAllocate(_, node) - or - exists(DataFlow::Node parent | - node = getAnAliasRecursive(parent) and - result = Config::getAnAlias(parent) - ) - } - private predicate isLeakedAtControlPoint(TResource resource, ControlFlowNode cfgNode) { // Holds if this control point is where the resource was allocated (and therefore not freed). resource = TJustResource(_, cfgNode) @@ -86,7 +73,7 @@ module ResourceLeak { isLeakedAtControlPoint(resource, cfgNode.getAPredecessor()) and not exists(DataFlow::Node freed, DataFlow::Node resourceNode | Config::isFree(cfgNode, freed) and - freed = getAnAliasRecursive(resourceNode) and + freed = Config::getAnAlias(resourceNode) and resource = TJustResource(resourceNode, _) ) } From 56f2996bb0758fe693c87cf87d645d975ae0ec1a Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Thu, 20 Feb 2025 18:21:12 -0800 Subject: [PATCH 3/5] Add change note --- .../2025-02-20-rule-22-16-update-aliasing-for-performance.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 change_notes/2025-02-20-rule-22-16-update-aliasing-for-performance.md diff --git a/change_notes/2025-02-20-rule-22-16-update-aliasing-for-performance.md b/change_notes/2025-02-20-rule-22-16-update-aliasing-for-performance.md new file mode 100644 index 000000000..80ff92748 --- /dev/null +++ b/change_notes/2025-02-20-rule-22-16-update-aliasing-for-performance.md @@ -0,0 +1,3 @@ + - `RULE-22-16`, `ERR57-CPP`, `A15-1-4` - `MutexObjectsNotAlwaysUnlocked.ql`, `DoNotLeakResourcesWhenHandlingExceptions.ql`, `ValidResourcesStateBeforeThrow.ql`: + - Shared module `ResourceLeakAnalysis.qll` changed to not get aliases recursively for simplicity and improved performance. The recent update to these queries had logic intending to handle the case where an allocation node is an alias of a parent node, and the free operation releases that parent node. However, the behavior was incorrectly defined and not working, and in the presence of performance issues this behavior has been removed. + - (`RULE-22-16` only) The alias behavior has been updated to compare expressions with `HashCons` instead of `GlobalValueNumbering` for higher performance. GVN is more expensive generally, seemed to introduce low performance joins secondarily, and is stricter than `HashCons` in a contravening position, meaning a stricter analysis introduces a higher likelihood of false positives. \ No newline at end of file From 81663048818818eee39004e9d85f25a43ecad6ae Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Thu, 20 Feb 2025 18:48:40 -0800 Subject: [PATCH 4/5] format new test case --- c/misra/test/rules/RULE-22-16/test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c/misra/test/rules/RULE-22-16/test.c b/c/misra/test/rules/RULE-22-16/test.c index d0d4f6ddc..c97fb3d58 100644 --- a/c/misra/test/rules/RULE-22-16/test.c +++ b/c/misra/test/rules/RULE-22-16/test.c @@ -107,7 +107,7 @@ void f15(int p) { } void f16(int p) { - mtx_t* ptr; + mtx_t *ptr; mtx_t *ptr_m1 = ptr; mtx_t *ptr_m2 = ptr; mtx_lock(ptr_m1); // COMPLIANT[FALSE_POSITIVE] From 5a5f8b78901ff01ac12714b4c36576b5a23edce9 Mon Sep 17 00:00:00 2001 From: Mike Fairhurst Date: Thu, 20 Feb 2025 22:55:24 -0800 Subject: [PATCH 5/5] Commit changed test expectations --- .../test/rules/RULE-22-16/MutexObjectsNotAlwaysUnlocked.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/c/misra/test/rules/RULE-22-16/MutexObjectsNotAlwaysUnlocked.expected b/c/misra/test/rules/RULE-22-16/MutexObjectsNotAlwaysUnlocked.expected index dbee52ed5..46a295d75 100644 --- a/c/misra/test/rules/RULE-22-16/MutexObjectsNotAlwaysUnlocked.expected +++ b/c/misra/test/rules/RULE-22-16/MutexObjectsNotAlwaysUnlocked.expected @@ -7,3 +7,4 @@ WARNING: module 'DataFlow' has been deprecated and may be removed in future (Mut | test.c:72:3:72:10 | call to mtx_lock | Mutex 'g1' is locked here and may not always be subsequently unlocked. | | test.c:79:3:79:10 | call to mtx_lock | Mutex 'm' is locked here and may not always be subsequently unlocked. | | test.c:101:5:101:12 | call to mtx_lock | Mutex 'm' is locked here and may not always be subsequently unlocked. | +| test.c:113:3:113:10 | call to mtx_lock | Mutex 'ptr_m1' is locked here and may not always be subsequently unlocked. |