MEM34-C and RULE-22-2: Reduce FPs

The query implementation is now a path-problem and only outputs results for flow from address-of expressions and global variable accesses that do not have allocation expressions assigned to them.

Author: Nikita Kraiouchkine

c/cert/src/rules/MEM34-C/OnlyFreeMemoryAllocatedDynamicallyCert.ql

@@ -3,7 +3,7 @@
  * @name MEM34-C: Only free memory allocated dynamically
  * @description Freeing memory that is not allocated dynamically can lead to heap corruption and
  *              undefined behavior.
- * @kind problem
+ * @kind path-problem
  * @precision high
  * @problem.severity error
  * @tags external/cert/id/mem34-c

c/common/test/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.expected

@@ -1,6 +1,24 @@
-| test.c:8:8:8:10 | g_p | Free expression frees non-dynamically allocated memory. | test.c:8:8:8:10 | g_p |  |
-| test.c:10:8:10:10 | g_p | Free expression frees $@ which was not dynamically allocated. | test.c:9:9:9:12 | & ... | memory |
-| test.c:15:33:15:35 | g_p | Free expression frees non-dynamically allocated memory. | test.c:15:33:15:35 | g_p |  |
-| test.c:17:36:17:38 | ptr | Free expression frees $@ which was not dynamically allocated. | test.c:24:7:24:8 | & ... | memory |
-| test.c:23:8:23:8 | p | Free expression frees $@ which was not dynamically allocated. | test.c:22:13:22:14 | & ... | memory |
-| test.c:42:10:42:10 | p | Free expression frees non-dynamically allocated memory. | test.c:42:10:42:10 | p |  |
+problems
+| test.c:8:8:8:10 | g_p | test.c:8:8:8:10 | g_p | test.c:8:8:8:10 | g_p | Free expression frees memory which was not dynamically allocated. |
+| test.c:10:8:10:10 | g_p | test.c:10:8:10:10 | g_p | test.c:10:8:10:10 | g_p | Free expression frees memory which was not dynamically allocated. |
+| test.c:12:8:12:10 | g_p | test.c:12:8:12:10 | g_p | test.c:12:8:12:10 | g_p | Free expression frees memory which was not dynamically allocated. |
+| test.c:16:33:16:35 | g_p | test.c:16:33:16:35 | g_p | test.c:16:33:16:35 | g_p | Free expression frees memory which was not dynamically allocated. |
+| test.c:18:36:18:38 | ptr | test.c:27:7:27:8 | & ... | test.c:18:36:18:38 | ptr | Free expression frees memory which was not dynamically allocated. |
+| test.c:26:8:26:8 | p | test.c:25:13:25:14 | & ... | test.c:26:8:26:8 | p | Free expression frees memory which was not dynamically allocated. |
+edges
+| test.c:18:24:18:26 | ptr | test.c:18:36:18:38 | ptr |
+| test.c:25:13:25:14 | & ... | test.c:26:8:26:8 | p |
+| test.c:27:7:27:8 | & ... | test.c:28:15:28:15 | p |
+| test.c:28:15:28:15 | p | test.c:18:24:18:26 | ptr |
+nodes
+| test.c:8:8:8:10 | g_p | semmle.label | g_p |
+| test.c:10:8:10:10 | g_p | semmle.label | g_p |
+| test.c:12:8:12:10 | g_p | semmle.label | g_p |
+| test.c:16:33:16:35 | g_p | semmle.label | g_p |
+| test.c:18:24:18:26 | ptr | semmle.label | ptr |
+| test.c:18:36:18:38 | ptr | semmle.label | ptr |
+| test.c:25:13:25:14 | & ... | semmle.label | & ... |
+| test.c:26:8:26:8 | p | semmle.label | p |
+| test.c:27:7:27:8 | & ... | semmle.label | & ... |
+| test.c:28:15:28:15 | p | semmle.label | p |
+subpaths

c/common/test/rules/onlyfreememoryallocateddynamicallyshared/test.c

@@ -9,24 +9,29 @@ void test_global(void) {
   g_p = &g_i;
   free(g_p); // NON_COMPLIANT
   g_p = malloc(10);
-  free(g_p); // COMPLIANT - but could be written to in different scope
+  free(g_p); // COMPLIANT[FALSE_POSITIVE] - but could be written to in different
+             // scope
 }
 
 void test_global_b(void) { free(g_p); } // NON_COMPLIANT
 
 void free_nested(void *ptr) { free(ptr); } // NON_COMPLIANT - some paths
 
+void get_allocated_memory(void **p) { *p = malloc(10); }
+
 void test_local(void) {
   int i;
   int j;
   void *p = &i;
   free(p); // NON_COMPLIANT
   p = &j;
-  free_nested(p); // NON_COMPLIANT
+  free_nested(p); // NON_COMPLIANT - reported on line 18
   p = malloc(10);
   free(p); // COMPLIANT
   p = malloc(10);
   free_nested(p); // COMPLIANT
+  get_allocated_memory(&p);
+  free(p); // COMPLIANT
 }
 
 struct S {
@@ -39,7 +44,7 @@ void test_local_field_nested(struct S *s) { free(s->p); } // COMPLIANT
 void test_local_field(void) {
   struct S s;
   s.p = &s.i;
-  free(s.p); // NON_COMPLIANT
+  free(s.p); // NON_COMPLIANT[FALSE_NEGATIVE]
   s.p = malloc(10);
   free(s.p); // COMPLIANT
   s.p = malloc(10);

c/misra/src/rules/RULE-22-2/OnlyFreeMemoryAllocatedDynamicallyMisra.ql

@@ -3,7 +3,7 @@
  * @name RULE-22-2: A block of memory shall only be freed if it was allocated by means of a Standard Library function
  * @description Freeing memory that is not allocated dynamically can lead to heap corruption and
  *              undefined behavior.
- * @kind problem
+ * @kind path-problem
  * @precision high
  * @problem.severity error
  * @tags external/misra/id/rule-22-2

cpp/common/src/codingstandards/cpp/rules/freememorywhennolongerneededshared/FreeMemoryWhenNoLongerNeededShared.qll

@@ -12,20 +12,11 @@ import semmle.code.cpp.pointsto.PointsTo
 
 predicate allocated(FunctionCall fc) { allocExpr(fc, _) }
 
-/** Holds if there exists a call to a function that might free the allocation specified by `e`. */
-predicate freed(Expr e) {
-  freeExpr(_, e, _) or
-  exists(ExprCall c |
-    // cautiously assume that any ExprCall could be a call to free.
-    c.getAnArgument() = e
-  )
-}
-
 /** An expression for which there exists a function call that might free it. */
 class FreedExpr extends PointsToExpr {
-  FreedExpr() { freed(this) }
+  FreedExpr() { freeExprOrIndirect(this, _, _) }
 
-  override predicate interesting() { freed(this) }
+  override predicate interesting() { freeExprOrIndirect(this, _, _) }
 }
 
 /**

cpp/common/src/codingstandards/cpp/rules/onlyfreememoryallocateddynamicallyshared/OnlyFreeMemoryAllocatedDynamicallyShared.qll

@@ -9,6 +9,7 @@ import codingstandards.cpp.Exclusions
 import codingstandards.cpp.Allocations
 import semmle.code.cpp.dataflow.DataFlow
 import semmle.code.cpp.dataflow.DataFlow2
+import DataFlow::PathGraph
 
 /**
  * A pointer to potentially dynamically allocated memory
@@ -38,41 +39,54 @@ class FreeExprSink extends DataFlow::Node {
 }
 
 /**
- * A data-flow configuration that tracks flow from an `AllocExprSource` to
- * the value assigned to a variable.
+ * An `Expr` that is an `AddressOfExpr` of a `Variable`.
+ *
+ * `Field`s of `PointerType` are not included in order to reduce false-positives,
+ * as the data-flow library sometimes equates pointers to their underlying data.
  */
-class AllocExprSourceToAssignedValueConfig extends DataFlow2::Configuration {
-  AllocExprSourceToAssignedValueConfig() { this = "AllocExprSourceToAssignedValueConfig" }
-
-  override predicate isSource(DataFlow::Node source) { source instanceof AllocExprSource }
-
-  override predicate isSink(DataFlow::Node sink) {
-    sink.asExpr() = any(Variable v).getAnAssignedValue()
-  }
-}
-
-/**
- * An assignment of a value that is not a dynamically allocated pointer to a variable.
- */
-class NonDynamicallyAllocatedVariableAssignment extends DataFlow::Node {
-  NonDynamicallyAllocatedVariableAssignment() {
-    exists(Variable v |
-      this.asExpr() = v.getAnAssignedValue() and
-      not this.asExpr() instanceof NullValue and
-      not any(AllocExprSourceToAssignedValueConfig cfg).hasFlowTo(this)
-    )
+class AddressOfExprSourceNode extends Expr {
+  AddressOfExprSourceNode() {
+    exists(VariableAccess va |
+      this.(AddressOfExpr).getOperand() = va and
+      (
+        va.getTarget() instanceof StackVariable or
+        va.getTarget() instanceof GlobalVariable or
+        // allow address-of field, but only if that field is not a pointer type,
+        // as there may be nested allocations assigned to fields of pointer types.
+        va.(FieldAccess).getTarget().getUnderlyingType() instanceof ArithmeticType
+      )
+      or
+      this = va and
+      exists(GlobalVariable gv |
+        gv = va.getTarget() and
+        (
+          gv.getUnderlyingType() instanceof ArithmeticType or
+          not exists(gv.getAnAssignedValue()) or
+          exists(AddressOfExprSourceNode other |
+            DataFlow::localExprFlow(other, gv.getAnAssignedValue())
+          )
+        )
+      )
+    ) and
+    // exclude alloc(&allocated_ptr) cases
+    not any(DynamicMemoryAllocationToAddressOfDefiningArgConfig cfg)
+        .hasFlowTo(DataFlow::definitionByReferenceNodeFromArgument(this))
   }
 }
 
 /**
  * A data-flow configuration that tracks flow from an `AllocExprSource` to a `FreeExprSink`.
  */
-class DynamicMemoryAllocationToFreeConfig extends DataFlow::Configuration {
-  DynamicMemoryAllocationToFreeConfig() { this = "DynamicMemoryAllocationToFreeConfig" }
+class DynamicMemoryAllocationToAddressOfDefiningArgConfig extends DataFlow2::Configuration {
+  DynamicMemoryAllocationToAddressOfDefiningArgConfig() {
+    this = "DynamicMemoryAllocationToAddressOfDefiningArgConfig"
+  }
 
   override predicate isSource(DataFlow::Node source) { source instanceof AllocExprSource }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof FreeExprSink }
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asDefiningArgument() instanceof AddressOfExpr
+  }
 }
 
 /**
@@ -83,14 +97,14 @@ class NonDynamicPointerToFreeConfig extends DataFlow::Configuration {
   NonDynamicPointerToFreeConfig() { this = "NonDynamicPointerToFreeConfig" }
 
   override predicate isSource(DataFlow::Node source) {
-    source instanceof NonDynamicallyAllocatedVariableAssignment
+    source.asExpr() instanceof AddressOfExprSourceNode
   }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof FreeExprSink }
 
   override predicate isBarrierOut(DataFlow::Node node) {
     // the default interprocedural data-flow model flows through any field or array assignment
-    // expressionsto the qualifier (array base, pointer dereferenced, or qualifier) instead of the
+    // expressions to the qualifier (array base, pointer dereferenced, or qualifier) instead of the
     // individual element or field that the assignment modifies. this default behaviour causes
     // false positives for future frees of the object base, so we remove the edges
     // between those assignments from the graph with `isBarrierOut`.
@@ -103,25 +117,22 @@ class NonDynamicPointerToFreeConfig extends DataFlow::Configuration {
       )
     )
   }
+
+  override predicate isBarrierIn(DataFlow::Node node) {
+    // only the last source expression is relevant
+    isSource(node)
+  }
 }
 
 abstract class OnlyFreeMemoryAllocatedDynamicallySharedSharedQuery extends Query { }
 
 Query getQuery() { result instanceof OnlyFreeMemoryAllocatedDynamicallySharedSharedQuery }
 
 query predicate problems(
-  FreeExprSink free, string message, DataFlow::Node source, string sourceDescription
+  DataFlow::PathNode element, DataFlow::PathNode source, DataFlow::PathNode sink, string message
 ) {
-  not isExcluded(free.asExpr(), getQuery()) and
-  (
-    not any(DynamicMemoryAllocationToFreeConfig cfg).hasFlowTo(free) and
-    not any(NonDynamicPointerToFreeConfig cfg).hasFlowTo(free) and
-    message = "Free expression frees non-dynamically allocated memory." and
-    source = free and
-    sourceDescription = ""
-    or
-    any(NonDynamicPointerToFreeConfig cfg).hasFlow(source, free) and
-    message = "Free expression frees $@ which was not dynamically allocated." and
-    sourceDescription = "memory"
-  )
+  not isExcluded(element.getNode().asExpr(), getQuery()) and
+  element = sink and
+  any(NonDynamicPointerToFreeConfig cfg).hasFlowPath(source, sink) and
+  message = "Free expression frees memory which was not dynamically allocated."
 }

rule_packages/c/Memory2.json

@@ -113,7 +113,7 @@
       "queries": [
         {
           "description": "Freeing memory that is not allocated dynamically can lead to heap corruption and undefined behavior.",
-          "kind": "problem",
+          "kind": "path-problem",
           "name": "Only free memory allocated dynamically",
           "precision": "high",
           "severity": "error",
@@ -196,7 +196,7 @@
       "queries": [
         {
           "description": "Freeing memory that is not allocated dynamically can lead to heap corruption and undefined behavior.",
-          "kind": "problem",
+          "kind": "path-problem",
           "name": "A block of memory shall only be freed if it was allocated by means of a Standard Library function",
           "precision": "high",
           "severity": "error",