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. | diff --git a/c/misra/test/rules/RULE-22-16/test.c b/c/misra/test/rules/RULE-22-16/test.c index 00764645a..c97fb3d58 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/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 diff --git a/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll b/cpp/common/src/codingstandards/cpp/resources/ResourceLeakAnalysis.qll index 7d767b5cb..3dd61e934 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) ) } @@ -63,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) @@ -85,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, _) ) }