From 53ff7a002a8213a5d5e778bef2a895998d9890e1 Mon Sep 17 00:00:00 2001
From: Dan Liew <daniel.liew@imperial.ac.uk>
Date: Wed, 16 Dec 2015 18:13:11 +0000
Subject: Fix memory leaks of ``Array`` objects detected by ASan.

Some of these leaks were introduced by the factory constructor for Array
objects (f049ff3bc04daead8c3bb9f06e89e71e2054c82a) but a few others have
been around for far longer.

This leak was fixed by introducing a ``ArrayCache`` object which has two
purposes

* Retains ownership of all created ``Array`` objects and destroys them when
  the ``ArrayCache`` destructor is called.
* Mimic the caching behaviour for symbolic arrays that was introduced
  by f049ff3bc04daead8c3bb9f06e89e71e2054c82a where arrays with the same
  name and size get "uniqued".

The Executor now maintains a ``arrayCache`` member that it uses and
passes by pointer to objects that need to construct ``Array`` objects (i.e.
``ObjectState``). This way when the Executor is destroyed all the
``Array`` objects get freed which seems like the right time to do this.

For Kleaver the ``ParserImpl`` has a ``TheArrayCache`` member that is
used for building ``Array`` objects. This means that the Parser must
live as long as the built expressions will be used otherwise we will
have a use after free. I'm not sure this is the right design choice.
It might be better to transfer ownership of the ``Array`` objects to
the root ``Decl`` returned by the parser.
---
 unittests/Expr/ExprTest.cpp | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

(limited to 'unittests/Expr/ExprTest.cpp')

diff --git a/unittests/Expr/ExprTest.cpp b/unittests/Expr/ExprTest.cpp
index d05eb7ec..25129d8e 100644
--- a/unittests/Expr/ExprTest.cpp
+++ b/unittests/Expr/ExprTest.cpp
@@ -11,6 +11,7 @@
 #include "gtest/gtest.h"
 
 #include "klee/Expr.h"
+#include "klee/util/ArrayCache.h"
 
 using namespace klee;
 
@@ -29,9 +30,10 @@ TEST(ExprTest, BasicConstruction) {
 }
 
 TEST(ExprTest, ConcatExtract) {
-  const Array *array = Array::CreateArray("arr0", 256);
+  ArrayCache ac;
+  const Array *array = ac.CreateArray("arr0", 256);
   ref<Expr> read8 = Expr::createTempRead(array, 8);
-  const Array *array2 = Array::CreateArray("arr1", 256);
+  const Array *array2 = ac.CreateArray("arr1", 256);
   ref<Expr> read8_2 = Expr::createTempRead(array2, 8);
   ref<Expr> c100 = getConstant(100, 8);
 
@@ -81,10 +83,11 @@ TEST(ExprTest, ConcatExtract) {
 }
 
 TEST(ExprTest, ExtractConcat) {
-  const Array *array = Array::CreateArray("arr2", 256);
+  ArrayCache ac;
+  const Array *array = ac.CreateArray("arr2", 256);
   ref<Expr> read64 = Expr::createTempRead(array, 64);
 
-  const Array *array2 = Array::CreateArray("arr3", 256);
+  const Array *array2 = ac.CreateArray("arr3", 256);
   ref<Expr> read8_2 = Expr::createTempRead(array2, 8);
   
   ref<Expr> extract1 = ExtractExpr::create(read64, 36, 4);
-- 
cgit 1.4.1