Merge pull request #304 from geoffw0/resource-released

CPP: Fix false positive in AV Rule 79.ql

Author: jonas-semmle

change-notes/1.19/analysis-cpp.md

@@ -15,7 +15,7 @@
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
-| Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. |
+| Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. |
 | Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
 | Call to memory access function may overflow buffer | More correct results | Array indexing with a negative index is now detected by this query. |
 | Suspicious add with sizeof | Fewer false positive results | Arithmetic with void pointers (where allowed) is now excluded from this query. |

cpp/ql/src/jsf/4.10 Classes/AV Rule 79.ql

@@ -159,6 +159,17 @@ predicate unreleasedResource(Resource r, Expr acquire, File f, int acquireLine)
     )
     and f = acquire.getFile()
     and acquireLine = acquire.getLocation().getStartLine()
+
+    // check that any destructor for this class has a block; if it doesn't,
+    // we must be missing information.
+    and forall(Class c, Destructor d |
+      r.getDeclaringType().isConstructedFrom*(c) and
+      d = c.getAMember() and
+      not d.isCompilerGenerated() and
+      not d.isDefaulted() and
+      not d.isDeleted() |
+      exists(d.getBlock())
+    ) 
 }
 
 predicate freedInSameMethod(Resource r, Expr acquire) {

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/AV Rule 79.expected

@@ -17,3 +17,5 @@
 | Variants.cpp:65:3:65:17 | ... = ... | Resource a is acquired by class MyClass6 but not released anywhere in this class. |
 | Variants.cpp:66:3:66:36 | ... = ... | Resource b is acquired by class MyClass6 but not released anywhere in this class. |
 | Variants.cpp:67:3:67:41 | ... = ... | Resource c is acquired by class MyClass6 but not released anywhere in this class. |
+| Wrapped.cpp:46:3:46:22 | ... = ... | Resource ptr2 is acquired by class Wrapped2 but not released anywhere in this class. |
+| Wrapped.cpp:59:3:59:22 | ... = ... | Resource ptr4 is acquired by class Wrapped2 but not released anywhere in this class. |

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/NoDestructor.cpp

@@ -66,3 +66,25 @@ class MyClass7 : public MyClass7Base
 		n = new MyNumber(200); // GOOD: deleted in base class
 	}
 };
+
+template<class T>
+class TemplateWithDestructor
+{
+public:
+	TemplateWithDestructor(int len) {
+		ptr = new char[len]; // GOOD
+	}
+
+	~TemplateWithDestructor()
+	{
+		delete [] ptr;
+	}
+
+private:
+	char *ptr;
+};
+
+void test() {
+	TemplateWithDestructor<int *> *t_ptr = new TemplateWithDestructor<int *>(10);
+	//delete t_ptr; --- destructor never used
+}

cpp/ql/test/query-tests/jsf/4.10 Classes/AV Rule 79/Wrapped.cpp

@@ -37,3 +37,34 @@ class Wrapped
 private:
 	char *ptr1, *ptr2, *ptr3;
 };
+
+class Wrapped2
+{
+public:
+	Wrapped2(int len) {
+		ptr1 = new char[len]; // GOOD
+		ptr2 = new char[len]; // BAD: not released in destructor
+
+		Init(len);
+	}
+
+	~Wrapped2()
+	{
+		Shutdown();
+	}
+
+	void Init(int len)
+	{
+		ptr3 = new char[len]; // GOOD
+		ptr4 = new char[len]; // BAD: not released in destructor
+	}
+
+	void Shutdown()
+	{
+		delete [] ptr1;
+		delete [] ptr3;
+	}
+
+private:
+	char *ptr1, *ptr2, *ptr3, *ptr4;
+};