Merge pull request #862 from github/michaelrfairhurst/fix-resource-leak-analysis-qll-performance-issue

Fix performance issue in ResourceLeakAnalysis.qll

Author: knewbury01

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. |

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);
 }

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.

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<ResourceLeakConfigSig Config> {
       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<ResourceLeakConfigSig Config> {
     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, _)
     )
   }