From 8971bb25ce32f2b12698ca40c0f73011d9587cf1 Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Tue, 5 Apr 2016 11:58:24 +0200 Subject: [PATCH] heap: release ds pool meta data when destructed This patch makes sure that the dataspace pool is flushed before destructing the heap-local allocator-avl instance. With the original destruction order, the allocator would still contain dangling allocations on the account of the dataspace pool when destructed. In practice, this caused no problem because the underlying backing store is eventually freed on the destruction of the pool. But it triggers a runtime warning of the allocator since it has become more strict with regard to dangling allocations. --- repos/base/include/base/heap.h | 39 +++++++++------------- repos/base/src/base/heap/heap.cc | 56 ++++++++++++++++++++++++-------- 2 files changed, 57 insertions(+), 38 deletions(-) diff --git a/repos/base/include/base/heap.h b/repos/base/include/base/heap.h index cbd57c1f9..982f6f297 100644 --- a/repos/base/include/base/heap.h +++ b/repos/base/include/base/heap.h @@ -15,6 +15,7 @@ #define _INCLUDE__BASE__HEAP_H_ #include +#include #include #include #include @@ -64,10 +65,6 @@ class Genode::Heap : public Allocator Dataspace(Ram_dataspace_capability c, void *local_addr, size_t size) : cap(c), local_addr(local_addr), size(size) { } - - inline void * operator new(Genode::size_t, void* addr) { - return addr; } - inline void operator delete(void*) { } }; /* @@ -77,31 +74,23 @@ class Genode::Heap : public Allocator struct Dataspace_pool : public List { Ram_session *ram_session; /* RAM session for backing store */ - Rm_session *rm_session; /* region manager */ + Rm_session *rm_session; - Dataspace_pool(Ram_session *ram_session, Rm_session *rm_session) - : ram_session(ram_session), rm_session(rm_session) { } + Dataspace_pool(Ram_session *ram, Rm_session *rm) + : ram_session(ram), rm_session(rm) { } - /** - * Destructor - */ ~Dataspace_pool(); void reassign_resources(Ram_session *ram, Rm_session *rm) { ram_session = ram, rm_session = rm; } }; - /* - * NOTE: The order of the member variables is important for - * the calling order of the destructors! - */ - - Lock _lock; - Dataspace_pool _ds_pool; /* list of dataspaces */ - Allocator_avl _alloc; /* local allocator */ - size_t _quota_limit; - size_t _quota_used; - size_t _chunk_size; + Lock _lock; + Volatile_object _alloc; /* local allocator */ + Dataspace_pool _ds_pool; /* list of dataspaces */ + size_t _quota_limit; + size_t _quota_used; + size_t _chunk_size; /** * Allocate a new dataspace of the specified size @@ -140,15 +129,17 @@ class Genode::Heap : public Allocator void *static_addr = 0, size_t static_size = 0) : + _alloc(nullptr), _ds_pool(ram_session, rm_session), - _alloc(0), _quota_limit(quota_limit), _quota_used(0), _chunk_size(MIN_CHUNK_SIZE) { if (static_addr) - _alloc.add_range((addr_t)static_addr, static_size); + _alloc->add_range((addr_t)static_addr, static_size); } + ~Heap(); + /** * Reconfigure quota limit * @@ -171,7 +162,7 @@ class Genode::Heap : public Allocator bool alloc(size_t, void **) override; void free(void *, size_t) override; size_t consumed() const override { return _quota_used; } - size_t overhead(size_t size) const override { return _alloc.overhead(size); } + size_t overhead(size_t size) const override { return _alloc->overhead(size); } bool need_size_for_free() const override { return false; } }; diff --git a/repos/base/src/base/heap/heap.cc b/repos/base/src/base/heap/heap.cc index 47da4e66f..49b0f73f5 100644 --- a/repos/base/src/base/heap/heap.cc +++ b/repos/base/src/base/heap/heap.cc @@ -11,9 +11,9 @@ * under the terms of the GNU General Public License version 2. */ +#include #include #include -#include #include #include @@ -36,8 +36,13 @@ Heap::Dataspace_pool::~Dataspace_pool() remove(ds); - /* have the destructor of the 'cap' member called */ - delete ds; + /* + * Call 'Dataspace' destructor to properly release the RAM dataspace + * capabilities. Note that we don't free the 'Dataspace' object at the + * local allocator because this is already done by the 'Heap' + * destructor prior executing the 'Dataspace_pool' destructor. + */ + ds->~Dataspace(); rm_session->detach(ds_local_addr); ram_session->free(ds_cap); @@ -84,17 +89,17 @@ Heap::Dataspace *Heap::_allocate_dataspace(size_t size, bool enforce_separate_me } else { /* add new local address range to our local allocator */ - _alloc.add_range((addr_t)ds_addr, size); + _alloc->add_range((addr_t)ds_addr, size); /* allocate the Dataspace structure */ - if (_alloc.alloc_aligned(sizeof(Heap::Dataspace), &ds_meta_data_addr, log2(sizeof(addr_t))).is_error()) { + if (_alloc->alloc_aligned(sizeof(Heap::Dataspace), &ds_meta_data_addr, log2(sizeof(addr_t))).is_error()) { PWRN("could not allocate dataspace meta data - this should never happen"); return 0; } } - ds = new (ds_meta_data_addr) Heap::Dataspace(new_ds_cap, ds_addr, size); + ds = construct_at(ds_meta_data_addr, new_ds_cap, ds_addr, size); _ds_pool.insert(ds); @@ -104,7 +109,7 @@ Heap::Dataspace *Heap::_allocate_dataspace(size_t size, bool enforce_separate_me bool Heap::_try_local_alloc(size_t size, void **out_addr) { - if (_alloc.alloc_aligned(size, out_addr, log2(sizeof(addr_t))).is_error()) + if (_alloc->alloc_aligned(size, out_addr, log2(sizeof(addr_t))).is_error()) return false; _quota_used += size; @@ -215,18 +220,41 @@ void Heap::free(void *addr, size_t size) _quota_used -= ds->size; - /* have the destructor of the 'cap' member called */ - delete ds; - _alloc.free(ds); + destroy(*_alloc, ds); } else { /* - * forward request to our local allocator - */ - _alloc.free(addr, size); + * forward request to our local allocator + */ + _alloc->free(addr, size); _quota_used -= size; - } } + + +Heap::~Heap() +{ + /* + * Revert allocations of heap-internal 'Dataspace' objects. Otherwise, the + * subsequent destruction of the 'Allocator_avl' would detect those blocks + * as dangling allocations. + * + * Since no new allocations can occur at the destruction time of the + * 'Heap', it is safe to release the 'Dataspace' objects at the allocator + * yet still access them afterwards during the destruction of the + * 'Allocator_avl'. + */ + for (Heap::Dataspace *ds = _ds_pool.first(); ds; ds = ds->next()) + _alloc->free(ds, sizeof(Dataspace)); + + /* + * Destruct 'Allocator_avl' before destructing the dataspace pool. This + * order is important because some dataspaces of the dataspace pool are + * used as backing store for the allocator's meta data. If we destroyed + * the object pool before the allocator, the subsequent attempt to destruct + * the allocator would access no-longer-present backing store. + */ + _alloc.destruct(); +}