Merge pull request #17110 from geoffw0/memfree

C++: Improve cpp/memory-may-not-be-freed

Author: geoffw0

cpp/ql/src/Critical/MemoryMayNotBeFreed.ql

@@ -39,7 +39,7 @@ predicate allocCallOrIndirect(Expr e) {
       allocCallOrIndirect(rtn.getExpr())
       or
       // return variable assigned with alloc
-      exists(Variable v |
+      exists(StackVariable v |
         v = rtn.getExpr().(VariableAccess).getTarget() and
         allocCallOrIndirect(v.getAnAssignedValue()) and
         not assignedToFieldOrGlobal(v, _)

cpp/ql/src/change-notes/2024-07-31-memory-may-not-be-freed.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed false positives in the `cpp/memory-may-not-be-freed` ("Memory may not be freed") query involving class methods that returned an allocated field of that class being misidentified as allocators.

cpp/ql/test/query-tests/Critical/MemoryFreed/MemoryFreed.expected

@@ -120,6 +120,12 @@
 | test_free.cpp:346:26:346:28 | ptr |
 | test_free.cpp:356:19:356:19 | a |
 | test_free.cpp:357:19:357:19 | a |
+| test_free.cpp:370:10:370:10 | a |
+| test_free.cpp:373:10:373:10 | e |
+| test_free.cpp:382:10:382:10 | i |
+| test_free.cpp:383:10:383:10 | s |
+| test_free.cpp:395:14:395:19 | buffer |
+| test_free.cpp:424:10:424:16 | buffer2 |
 | virtual.cpp:18:10:18:10 | a |
 | virtual.cpp:19:10:19:10 | c |
 | virtual.cpp:38:10:38:10 | b |

cpp/ql/test/query-tests/Critical/MemoryFreed/MemoryNeverFreed.expected

@@ -14,3 +14,5 @@
 | test_free.cpp:167:15:167:21 | call to realloc | This memory is never freed. |
 | test_free.cpp:253:14:253:19 | call to malloc | This memory is never freed. |
 | test_free.cpp:261:6:261:12 | new | This memory is never freed. |
+| test_free.cpp:365:21:365:26 | call to malloc | This memory is never freed. |
+| test_free.cpp:368:31:368:36 | call to malloc | This memory is never freed. |

cpp/ql/test/query-tests/Critical/MemoryFreed/test_free.cpp

@@ -355,4 +355,74 @@ struct E {
 void test(E* e) {
     free(e->ec[0].a);
     free(e->ec[1].a); // GOOD
-}
+}
+
+// ---
+
+void test_return_by_parameter(int **out_i, MyStruct **out_ms) {
+    int *a = (int *)malloc(sizeof(int)); // GOOD (freed)
+    int *b = (int *)malloc(sizeof(int)); // GOOD (out parameter)
+    int *d = (int *)malloc(sizeof(int)); // BAD (not freed)
+    MyStruct *e = (MyStruct *)malloc(sizeof(MyStruct)); // GOOD (freed)
+    MyStruct *f = (MyStruct *)malloc(sizeof(MyStruct)); // GOOD (out parameter)
+    MyStruct *h = (MyStruct *)malloc(sizeof(MyStruct)); // BAD (not freed)
+
+    free(a);
+    *out_i = b;
+
+    free(e);
+    *out_ms = f;
+}
+
+void test_return_by_parameter_caller() {
+    int *i;
+    MyStruct *s;
+
+    test_return_by_parameter(&i, &s);
+    free(i);
+    free(s);
+}
+
+// ---
+
+class HasGetterAndFree {
+public:
+    HasGetterAndFree() {
+        buffer = malloc(1024);
+    }
+
+    ~HasGetterAndFree() {
+        free(buffer);
+    }
+
+    void *getBuffer() {
+        return buffer;
+    }
+
+    void *buffer;
+};
+
+class HasGetterNoFree {
+public:
+    HasGetterNoFree() {
+        buffer = malloc(1024);
+    }
+
+    void *getBuffer() {
+        return buffer;
+    }
+
+    void *buffer;
+};
+
+void testHasGetter() {
+    HasGetterAndFree hg;
+    void *buffer = hg.getBuffer(); // GOOD (freed in destructor)
+
+    HasGetterNoFree hg2;
+    void *buffer2 = hg2.getBuffer(); // GOOD (freed below)
+    free(buffer2);
+
+    HasGetterNoFree hg3;
+    void *buffer3 = hg3.getBuffer(); // BAD (not freed) [NOT DETECTED]
+}