From 76d9bb57c9377d12011f96fb8c727eb75be4fe02 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 27 Oct 2022 08:20:05 +0100 Subject: [PATCH 01/16] Implement tracking full slabs and large allocations This adds an additional SeqSet that is used to track all the fully used slabs and large allocations. This gives more chances to detect memory leaks, and additionally catch some more UAF failures where the object is not recycled. --- src/snmalloc/ds_core/seqset.h | 12 +++--- src/snmalloc/mem/corealloc.h | 37 ++++++++++++++++++ src/snmalloc/mem/localalloc.h | 3 ++ src/test/func/statistics/stats.cc | 64 ++++++++++++++++++++++++++++--- 4 files changed, 104 insertions(+), 12 deletions(-) diff --git a/src/snmalloc/ds_core/seqset.h b/src/snmalloc/ds_core/seqset.h index ef854958c..600ec07df 100644 --- a/src/snmalloc/ds_core/seqset.h +++ b/src/snmalloc/ds_core/seqset.h @@ -83,6 +83,12 @@ namespace snmalloc #endif } + public: + /** + * Empty queue + */ + constexpr SeqSet() = default; + /** * Check for empty */ @@ -95,12 +101,6 @@ namespace snmalloc return head.next == &head; } - public: - /** - * Empty queue - */ - constexpr SeqSet() = default; - /** * Remove an element from the queue * diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 93df53e66..f2603ef2c 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -61,6 +61,12 @@ namespace snmalloc uint16_t length = 0; } alloc_classes[NUM_SMALL_SIZECLASSES]{}; + /** + * The set of all slabs and large allocations + * from this allocator that are full or almost full. + */ + SeqSet laden{}; + /** * Local entropy source and current version of keys for * this thread @@ -420,6 +426,9 @@ namespace snmalloc UNUSED(size); #endif + // Remove from set of fully used slabs. + meta->node.remove(); + Config::Backend::dealloc_chunk( get_backend_local_state(), *meta, p, size); @@ -436,6 +445,9 @@ namespace snmalloc // Wake slab up. meta->set_not_sleeping(sizeclass); + // Remove from set of fully used slabs. + meta->node.remove(); + alloc_classes[sizeclass].available.insert(meta); alloc_classes[sizeclass].length++; @@ -744,6 +756,10 @@ namespace snmalloc alloc_classes[sizeclass].length++; sl.insert(meta); } + else + { + laden.insert(meta); + } auto r = finish_alloc(p, sizeclass); return ticker.check_tick(r); @@ -811,6 +827,10 @@ namespace snmalloc alloc_classes[sizeclass].length++; alloc_classes[sizeclass].available.insert(meta); } + else + { + laden.insert(meta); + } auto r = finish_alloc(p, sizeclass); return ticker.check_tick(r); @@ -864,6 +884,15 @@ namespace snmalloc dealloc_local_slabs(sizeclass); } + laden.iterate([this, domesticate](BackendSlabMetadata* meta) + SNMALLOC_FAST_PATH_LAMBDA { + if (!meta->is_large()) + { + meta->free_queue.validate( + entropy.get_free_list_key(), domesticate); + } + }); + return posted; } @@ -916,6 +945,14 @@ namespace snmalloc size_class++; } + if (!laden.is_empty()) + { + if (result != nullptr) + *result = false; + else + report_fatal_error("debug_is_empty: found non-empty allocator"); + } + // Place the static stub message on the queue. init_message_queue(); diff --git a/src/snmalloc/mem/localalloc.h b/src/snmalloc/mem/localalloc.h index 82d75dabd..a9fde0f80 100644 --- a/src/snmalloc/mem/localalloc.h +++ b/src/snmalloc/mem/localalloc.h @@ -199,7 +199,10 @@ namespace snmalloc // Initialise meta data for a successful large allocation. if (meta != nullptr) + { meta->initialise_large(); + core_alloc->laden.insert(meta); + } if (zero_mem == YesZero && chunk.unsafe_ptr() != nullptr) { diff --git a/src/test/func/statistics/stats.cc b/src/test/func/statistics/stats.cc index 2de3e2d9b..a86deb463 100644 --- a/src/test/func/statistics/stats.cc +++ b/src/test/func/statistics/stats.cc @@ -1,16 +1,26 @@ -#include - +#ifdef SNMALLOC_PASS_THROUGH // This test depends on snmalloc internals int main() { -#ifndef SNMALLOC_PASS_THROUGH // This test depends on snmalloc internals + return 0; +} +#else +# include +# include +# include + +template +void debug_check_empty_1() +{ snmalloc::Alloc& a = snmalloc::ThreadAlloc::get(); bool result; - auto r = a.alloc(16); + auto r = a.alloc(size); snmalloc::debug_check_empty(&result); if (result != false) { + std::cout << "debug_check_empty failed to detect leaked memory:" << size + << std::endl; abort(); } @@ -19,14 +29,17 @@ int main() snmalloc::debug_check_empty(&result); if (result != true) { + std::cout << "debug_check_empty failed to say empty:" << size << std::endl; abort(); } - r = a.alloc(16); + r = a.alloc(size); snmalloc::debug_check_empty(&result); if (result != false) { + std::cout << "debug_check_empty failed to detect leaked memory:" << size + << std::endl; abort(); } @@ -35,7 +48,46 @@ int main() snmalloc::debug_check_empty(&result); if (result != true) { + std::cout << "debug_check_empty failed to say empty:" << size << std::endl; abort(); } -#endif } + +template +void debug_check_empty_2() +{ + snmalloc::Alloc& a = snmalloc::ThreadAlloc::get(); + bool result; + std::vector allocs; + size_t count = 2048; + + for (size_t i = 0; i < count; i++) + { + auto r = a.alloc(128); + allocs.push_back(r); + snmalloc::debug_check_empty(&result); + if (result != false) + { + std::cout << "False empty after " << i << " allocations of " << size + << std::endl; + abort(); + } + } + + for (size_t i = 0; i < count; i++) + { + a.dealloc(allocs[i]); + } + snmalloc::debug_check_empty(); +} + +int main() +{ + debug_check_empty_1<16>(); + debug_check_empty_1<1024 * 1024 * 32>(); + + debug_check_empty_2<32>(); + debug_check_empty_2<1024 * 1024 * 32>(); + return 0; +} +#endif \ No newline at end of file From c72fa293676fb33e632903066120b89c86d5cd89 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Mon, 31 Oct 2022 15:12:33 +0000 Subject: [PATCH 02/16] Make slabmeta track a slab interior pointer Use the head of the free list builder to track an interior pointer to the slab. This is unused unless the list contains something. Hence, we can use this to represent an interior pointer to the slab and report more accurate leaks. --- src/snmalloc/mem/corealloc.h | 47 +++++++++++++++++++++------------- src/snmalloc/mem/freelist.h | 30 +++++++++++----------- src/snmalloc/mem/localalloc.h | 3 ++- src/snmalloc/mem/metadata.h | 16 +++++++++--- src/snmalloc/mem/remotecache.h | 8 +++--- 5 files changed, 63 insertions(+), 41 deletions(-) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index f2603ef2c..d65f8aa1e 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -810,7 +810,8 @@ namespace snmalloc } // Set meta slab to empty. - meta->initialise(sizeclass); + meta->initialise( + sizeclass, address_cast(slab), entropy.get_free_list_key()); // Build a free list for the slab alloc_new_list(slab, meta, rsize, slab_size, entropy); @@ -912,7 +913,7 @@ namespace snmalloc c->remote_allocator = public_state(); // Set up remote cache. - c->remote_dealloc_cache.init(); + c->remote_dealloc_cache.init(entropy.get_free_list_key()); } /** @@ -921,36 +922,46 @@ namespace snmalloc */ bool debug_is_empty_impl(bool* result) { - auto test = [&result](auto& queue, smallsizeclass_t size_class) { - queue.iterate([&result, size_class](auto slab_metadata) { + auto& key = entropy.get_free_list_key(); + + auto error = [&result, &key](auto slab_metadata) { + + auto slab_interior = slab_metadata->get_slab_interior(key); + const PagemapEntry& entry = + Config::Backend::get_metaentry(slab_interior); + auto size_class = entry.get_sizeclass(); + auto slab_size = sizeclass_full_to_slab_size(size_class); + auto slab_start = bits::align_down(slab_interior, slab_size); + + if (result != nullptr) + *result = false; + else + report_fatal_error( + "debug_is_empty: found non-empty allocator: size={} on " + "slab_start {}", + sizeclass_full_to_size(size_class), + slab_start); + }; + + auto test = [&error](auto& queue) { + queue.iterate([&error](auto slab_metadata) { if (slab_metadata->needed() != 0) { - if (result != nullptr) - *result = false; - else - report_fatal_error( - "debug_is_empty: found non-empty allocator: size={} ({})", - sizeclass_to_size(size_class), - size_class); + error(slab_metadata); } }); }; bool sent_something = flush(true); - smallsizeclass_t size_class = 0; for (auto& alloc_class : alloc_classes) { - test(alloc_class.available, size_class); - size_class++; + test(alloc_class.available); } if (!laden.is_empty()) { - if (result != nullptr) - *result = false; - else - report_fatal_error("debug_is_empty: found non-empty allocator"); + error(laden.peek()); } // Place the static stub message on the queue. diff --git a/src/snmalloc/mem/freelist.h b/src/snmalloc/mem/freelist.h index 335f12881..18e410b53 100644 --- a/src/snmalloc/mem/freelist.h +++ b/src/snmalloc/mem/freelist.h @@ -115,7 +115,6 @@ namespace snmalloc class T { template< - bool, bool, SNMALLOC_CONCEPT(capptr::IsBound), SNMALLOC_CONCEPT(capptr::IsBound)> @@ -220,7 +219,6 @@ namespace snmalloc return reinterpret_cast*>(ptr); } - private: /** * Involutive encryption with raw pointers */ @@ -247,7 +245,6 @@ namespace snmalloc } } - public: /** * Encode next. We perform two convenient little bits of type-level * sleight of hand here: @@ -506,7 +503,6 @@ namespace snmalloc */ template< bool RANDOM, - bool INIT = true, SNMALLOC_CONCEPT(capptr::IsBound) BView = capptr::bounds::Alloc, SNMALLOC_CONCEPT(capptr::IsBound) BQueue = capptr::bounds::AllocWild> class Builder @@ -542,7 +538,7 @@ namespace snmalloc end[ix] = reinterpret_cast(p); } - Object::BHeadPtr cast_head(uint32_t ix) + Object::BHeadPtr cast_head(uint32_t ix) const { return Object::BHeadPtr::unsafe_from( static_cast*>(head[ix])); @@ -551,13 +547,7 @@ namespace snmalloc std::array length{}; public: - constexpr Builder() - { - if (INIT) - { - init(); - } - } + constexpr Builder() {} /** * Checks if the builder contains any elements. @@ -630,7 +620,7 @@ namespace snmalloc * encoded. */ Object::BHeadPtr - read_head(uint32_t index, const FreeListKey& key) + read_head(uint32_t index, const FreeListKey& key) const { return Object::decode_next( address_cast(&head[index]), cast_head(index), key); @@ -688,7 +678,7 @@ namespace snmalloc /** * Set the builder to a not building state. */ - constexpr void init() + constexpr void init(address_t slab, const FreeListKey& key) { for (size_t i = 0; i < LENGTH; i++) { @@ -697,6 +687,16 @@ namespace snmalloc { length[i] = 0; } + + // Head is not live when a building is initialised. + // We use this slot to store a pointer into the slab for the + // allocations. This then establishes the invariant that head is + // always (a possibly encoded) pointer into the slab, and thus + // the Freelist builder always knows which block it is referring too. + head[i] = Object::code_next( + address_cast(&head[i]), + reinterpret_cast*>(slab), + key); } } @@ -718,7 +718,7 @@ namespace snmalloc // empty, but you are not allowed to call this in the empty case. auto last = Object::BHeadPtr::unsafe_from( Object::from_next_ptr(cast_end(0))); - init(); + init(address_cast(head[0]), key); return {first, last}; } diff --git a/src/snmalloc/mem/localalloc.h b/src/snmalloc/mem/localalloc.h index a9fde0f80..f2827be88 100644 --- a/src/snmalloc/mem/localalloc.h +++ b/src/snmalloc/mem/localalloc.h @@ -200,7 +200,8 @@ namespace snmalloc // Initialise meta data for a successful large allocation. if (meta != nullptr) { - meta->initialise_large(); + meta->initialise_large( + address_cast(chunk), local_cache.entropy.get_free_list_key()); core_alloc->laden.insert(meta); } diff --git a/src/snmalloc/mem/metadata.h b/src/snmalloc/mem/metadata.h index fabf85611..e4243b98d 100644 --- a/src/snmalloc/mem/metadata.h +++ b/src/snmalloc/mem/metadata.h @@ -440,12 +440,13 @@ namespace snmalloc /** * Initialise FrontendSlabMetadata for a slab. */ - void initialise(smallsizeclass_t sizeclass) + void initialise( + smallsizeclass_t sizeclass, address_t slab, const FreeListKey& key) { static_assert( std::is_base_of::value, "Template should be a subclass of FrontendSlabMetadata"); - free_queue.init(); + free_queue.init(slab, key); // Set up meta data as if the entire slab has been turned into a free // list. This means we don't have to check for special cases where we have // returned all the elements, but this is a slab that is still being bump @@ -461,10 +462,10 @@ namespace snmalloc * * Set needed so immediately moves to slow path. */ - void initialise_large() + void initialise_large(address_t slab, const FreeListKey& key) { // We will push to this just to make the fast path clean. - free_queue.init(); + free_queue.init(slab, key); // Flag to detect that it is a large alloc on the slow path large_ = true; @@ -579,6 +580,13 @@ namespace snmalloc return {p, !sleeping}; } + + // Returns a pointer to somewhere in the slab. May not be the + // start of the slab. + address_t get_slab_interior(const FreeListKey& key) const + { + return address_cast(free_queue.read_head(0, key)); + } }; /** diff --git a/src/snmalloc/mem/remotecache.h b/src/snmalloc/mem/remotecache.h index a415a1daa..49dcc5f51 100644 --- a/src/snmalloc/mem/remotecache.h +++ b/src/snmalloc/mem/remotecache.h @@ -17,7 +17,7 @@ namespace snmalloc */ struct RemoteDeallocCache { - std::array, REMOTE_SLOTS> list; + std::array, REMOTE_SLOTS> list; /** * The total amount of memory we are waiting for before we will dispatch @@ -165,14 +165,16 @@ namespace snmalloc * Must be called before anything else to ensure actually initialised * not just zero init. */ - void init() + void init(const FreeListKey& key) { #ifndef NDEBUG initialised = true; #endif for (auto& l : list) { - l.init(); + // We do not need to initialise with a particular slab, so pass + // an address of 0. + l.init(0, key); } capacity = REMOTE_CACHE; } From 6a07fd89c31e7df27e2840cbf8cd656bf26cfc28 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Mon, 31 Oct 2022 15:36:21 +0000 Subject: [PATCH 03/16] clangformat --- src/snmalloc/mem/corealloc.h | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index d65f8aa1e..8bab53850 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -885,14 +885,13 @@ namespace snmalloc dealloc_local_slabs(sizeclass); } - laden.iterate([this, domesticate](BackendSlabMetadata* meta) - SNMALLOC_FAST_PATH_LAMBDA { - if (!meta->is_large()) - { - meta->free_queue.validate( - entropy.get_free_list_key(), domesticate); - } - }); + laden.iterate([this, domesticate]( + BackendSlabMetadata* meta) SNMALLOC_FAST_PATH_LAMBDA { + if (!meta->is_large()) + { + meta->free_queue.validate(entropy.get_free_list_key(), domesticate); + } + }); return posted; } @@ -925,7 +924,6 @@ namespace snmalloc auto& key = entropy.get_free_list_key(); auto error = [&result, &key](auto slab_metadata) { - auto slab_interior = slab_metadata->get_slab_interior(key); const PagemapEntry& entry = Config::Backend::get_metaentry(slab_interior); From 5758b7f6370684971a9dd6e220de81ea0cbd7fc5 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Mon, 31 Oct 2022 15:37:52 +0000 Subject: [PATCH 04/16] clangtidy --- src/snmalloc/mem/freelist.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/snmalloc/mem/freelist.h b/src/snmalloc/mem/freelist.h index 18e410b53..8a4aebfd5 100644 --- a/src/snmalloc/mem/freelist.h +++ b/src/snmalloc/mem/freelist.h @@ -547,7 +547,7 @@ namespace snmalloc std::array length{}; public: - constexpr Builder() {} + constexpr Builder() = default; /** * Checks if the builder contains any elements. From d492c36e1566274024c7b61275e7ccc828a0c2f9 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Mon, 31 Oct 2022 15:39:14 +0000 Subject: [PATCH 05/16] clangtidy --- src/snmalloc/mem/freelist.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/snmalloc/mem/freelist.h b/src/snmalloc/mem/freelist.h index 8a4aebfd5..ee778591c 100644 --- a/src/snmalloc/mem/freelist.h +++ b/src/snmalloc/mem/freelist.h @@ -528,7 +528,7 @@ namespace snmalloc // This enables branch free enqueuing. std::array end{nullptr}; - Object::BQueuePtr* cast_end(uint32_t ix) + [[nodiscard]] Object::BQueuePtr* cast_end(uint32_t ix) const { return reinterpret_cast*>(end[ix]); } @@ -538,7 +538,7 @@ namespace snmalloc end[ix] = reinterpret_cast(p); } - Object::BHeadPtr cast_head(uint32_t ix) const + [[nodiscard]] Object::BHeadPtr cast_head(uint32_t ix) const { return Object::BHeadPtr::unsafe_from( static_cast*>(head[ix])); @@ -619,7 +619,7 @@ namespace snmalloc * and is thus subject to encoding if the next_object pointers * encoded. */ - Object::BHeadPtr + [[nodiscard]] Object::BHeadPtr read_head(uint32_t index, const FreeListKey& key) const { return Object::decode_next( From 465d6f5c03787bc35decf53a0814d52fa195dd36 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 1 Nov 2022 09:30:26 +0000 Subject: [PATCH 06/16] Clang tidy again. --- src/snmalloc/mem/metadata.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/snmalloc/mem/metadata.h b/src/snmalloc/mem/metadata.h index e4243b98d..9dc21eb13 100644 --- a/src/snmalloc/mem/metadata.h +++ b/src/snmalloc/mem/metadata.h @@ -583,7 +583,7 @@ namespace snmalloc // Returns a pointer to somewhere in the slab. May not be the // start of the slab. - address_t get_slab_interior(const FreeListKey& key) const + [[nodiscard]] address_t get_slab_interior(const FreeListKey& key) const { return address_cast(free_queue.read_head(0, key)); } From 95581ec93f4c432d124b46780e641c52c878e645 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 1 Nov 2022 11:51:31 +0000 Subject: [PATCH 07/16] Fixing provenance. --- src/snmalloc/mem/corealloc.h | 2 +- src/snmalloc/mem/freelist.h | 4 ++-- src/snmalloc/mem/localalloc.h | 2 +- src/snmalloc/mem/metadata.h | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 8bab53850..d31b49f12 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -811,7 +811,7 @@ namespace snmalloc // Set meta slab to empty. meta->initialise( - sizeclass, address_cast(slab), entropy.get_free_list_key()); + sizeclass, slab.unsafe_ptr(), entropy.get_free_list_key()); // Build a free list for the slab alloc_new_list(slab, meta, rsize, slab_size, entropy); diff --git a/src/snmalloc/mem/freelist.h b/src/snmalloc/mem/freelist.h index ee778591c..ba7346f63 100644 --- a/src/snmalloc/mem/freelist.h +++ b/src/snmalloc/mem/freelist.h @@ -678,7 +678,7 @@ namespace snmalloc /** * Set the builder to a not building state. */ - constexpr void init(address_t slab, const FreeListKey& key) + constexpr void init(void* slab, const FreeListKey& key) { for (size_t i = 0; i < LENGTH; i++) { @@ -718,7 +718,7 @@ namespace snmalloc // empty, but you are not allowed to call this in the empty case. auto last = Object::BHeadPtr::unsafe_from( Object::from_next_ptr(cast_end(0))); - init(address_cast(head[0]), key); + init(head[0], key); return {first, last}; } diff --git a/src/snmalloc/mem/localalloc.h b/src/snmalloc/mem/localalloc.h index f2827be88..17a6269a6 100644 --- a/src/snmalloc/mem/localalloc.h +++ b/src/snmalloc/mem/localalloc.h @@ -201,7 +201,7 @@ namespace snmalloc if (meta != nullptr) { meta->initialise_large( - address_cast(chunk), local_cache.entropy.get_free_list_key()); + chunk.unsafe_ptr(), local_cache.entropy.get_free_list_key()); core_alloc->laden.insert(meta); } diff --git a/src/snmalloc/mem/metadata.h b/src/snmalloc/mem/metadata.h index 9dc21eb13..f26ebe46b 100644 --- a/src/snmalloc/mem/metadata.h +++ b/src/snmalloc/mem/metadata.h @@ -441,7 +441,7 @@ namespace snmalloc * Initialise FrontendSlabMetadata for a slab. */ void initialise( - smallsizeclass_t sizeclass, address_t slab, const FreeListKey& key) + smallsizeclass_t sizeclass, void* slab, const FreeListKey& key) { static_assert( std::is_base_of::value, @@ -462,7 +462,7 @@ namespace snmalloc * * Set needed so immediately moves to slow path. */ - void initialise_large(address_t slab, const FreeListKey& key) + void initialise_large(void* slab, const FreeListKey& key) { // We will push to this just to make the fast path clean. free_queue.init(slab, key); From 135a67073be2baedde2bf6478b71b947aae9c094 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 1 Nov 2022 14:10:19 +0000 Subject: [PATCH 08/16] Clangformat --- src/snmalloc/mem/metadata.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/snmalloc/mem/metadata.h b/src/snmalloc/mem/metadata.h index f26ebe46b..dae6ecd54 100644 --- a/src/snmalloc/mem/metadata.h +++ b/src/snmalloc/mem/metadata.h @@ -440,8 +440,8 @@ namespace snmalloc /** * Initialise FrontendSlabMetadata for a slab. */ - void initialise( - smallsizeclass_t sizeclass, void* slab, const FreeListKey& key) + void + initialise(smallsizeclass_t sizeclass, void* slab, const FreeListKey& key) { static_assert( std::is_base_of::value, From 211edb5f37599dd1c4a42a75c0eaac50977797a2 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 1 Nov 2022 17:23:45 +0000 Subject: [PATCH 09/16] Clang tidy. --- src/snmalloc/mem/remotecache.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/snmalloc/mem/remotecache.h b/src/snmalloc/mem/remotecache.h index 49dcc5f51..e63bc3e1c 100644 --- a/src/snmalloc/mem/remotecache.h +++ b/src/snmalloc/mem/remotecache.h @@ -173,8 +173,8 @@ namespace snmalloc for (auto& l : list) { // We do not need to initialise with a particular slab, so pass - // an address of 0. - l.init(0, key); + // a nullptr. + l.init(nullptr, key); } capacity = REMOTE_CACHE; } From 9b77a3f5bacdfab9e3223740a794274f20306274 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Sun, 18 Dec 2022 22:45:45 +0000 Subject: [PATCH 10/16] Add assert for sanity --- src/snmalloc/mem/corealloc.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index d31b49f12..9cd97a603 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -927,6 +927,7 @@ namespace snmalloc auto slab_interior = slab_metadata->get_slab_interior(key); const PagemapEntry& entry = Config::Backend::get_metaentry(slab_interior); + SNMALLOC_ASSERT(slab_metadata == entry.get_slab_metadata()); auto size_class = entry.get_sizeclass(); auto slab_size = sizeclass_full_to_slab_size(size_class); auto slab_start = bits::align_down(slab_interior, slab_size); From a0c44a30cf6731df08fcede2f54f069397ee36e9 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Sun, 18 Dec 2022 22:47:41 +0000 Subject: [PATCH 11/16] Make reinterpret_cast more descriptive. Add an operation to get a tag free pointer from an address_t, and use it --- src/snmalloc/aal/address.h | 9 +++++++++ src/snmalloc/mem/freelist.h | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/snmalloc/aal/address.h b/src/snmalloc/aal/address.h index 2a9d614ae..26824a4ee 100644 --- a/src/snmalloc/aal/address.h +++ b/src/snmalloc/aal/address.h @@ -290,4 +290,13 @@ namespace snmalloc return static_cast(a - pointer_align_down(a)); } + /** + * Convert an address_t to a pointer. The returned pointer should never be followed. + * On CHERI following this pointer will result in a capability violation. + */ + template + SNMALLOC_FAST_PATH_INLINE T* useless_ptr_from_addr(address_t p) + { + return reinterpret_cast(p); + } } // namespace snmalloc diff --git a/src/snmalloc/mem/freelist.h b/src/snmalloc/mem/freelist.h index ba7346f63..a4ad423a1 100644 --- a/src/snmalloc/mem/freelist.h +++ b/src/snmalloc/mem/freelist.h @@ -695,7 +695,7 @@ namespace snmalloc // the Freelist builder always knows which block it is referring too. head[i] = Object::code_next( address_cast(&head[i]), - reinterpret_cast*>(slab), + useless_ptr_from_addr>(address_cast(slab)), key); } } From b057e3976e2751a1d8684f5782ff75a787250b05 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Sun, 18 Dec 2022 22:49:19 +0000 Subject: [PATCH 12/16] Clangformat --- src/snmalloc/aal/address.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/snmalloc/aal/address.h b/src/snmalloc/aal/address.h index 26824a4ee..36fef45d2 100644 --- a/src/snmalloc/aal/address.h +++ b/src/snmalloc/aal/address.h @@ -291,8 +291,9 @@ namespace snmalloc } /** - * Convert an address_t to a pointer. The returned pointer should never be followed. - * On CHERI following this pointer will result in a capability violation. + * Convert an address_t to a pointer. The returned pointer should never be + * followed. On CHERI following this pointer will result in a capability + * violation. */ template SNMALLOC_FAST_PATH_INLINE T* useless_ptr_from_addr(address_t p) From 669c0d55c5bc63a27d802f760556e7222772cc0c Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 20 Dec 2022 09:10:47 +0000 Subject: [PATCH 13/16] CR --- src/snmalloc/aal/address.h | 2 +- src/snmalloc/mem/corealloc.h | 2 +- src/snmalloc/mem/freelist.h | 6 +++--- src/snmalloc/mem/localalloc.h | 2 +- src/snmalloc/mem/metadata.h | 6 +++--- src/snmalloc/mem/remotecache.h | 4 ++-- src/test/func/statistics/stats.cc | 2 +- 7 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/snmalloc/aal/address.h b/src/snmalloc/aal/address.h index 36fef45d2..d10474c27 100644 --- a/src/snmalloc/aal/address.h +++ b/src/snmalloc/aal/address.h @@ -298,6 +298,6 @@ namespace snmalloc template SNMALLOC_FAST_PATH_INLINE T* useless_ptr_from_addr(address_t p) { - return reinterpret_cast(p); + return reinterpret_cast(static_cast(p)); } } // namespace snmalloc diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 9cd97a603..4b5b9828f 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -811,7 +811,7 @@ namespace snmalloc // Set meta slab to empty. meta->initialise( - sizeclass, slab.unsafe_ptr(), entropy.get_free_list_key()); + sizeclass, address_cast(slab), entropy.get_free_list_key()); // Build a free list for the slab alloc_new_list(slab, meta, rsize, slab_size, entropy); diff --git a/src/snmalloc/mem/freelist.h b/src/snmalloc/mem/freelist.h index a4ad423a1..2830f9b8f 100644 --- a/src/snmalloc/mem/freelist.h +++ b/src/snmalloc/mem/freelist.h @@ -678,7 +678,7 @@ namespace snmalloc /** * Set the builder to a not building state. */ - constexpr void init(void* slab, const FreeListKey& key) + constexpr void init(address_t slab, const FreeListKey& key) { for (size_t i = 0; i < LENGTH; i++) { @@ -695,7 +695,7 @@ namespace snmalloc // the Freelist builder always knows which block it is referring too. head[i] = Object::code_next( address_cast(&head[i]), - useless_ptr_from_addr>(address_cast(slab)), + useless_ptr_from_addr>(slab), key); } } @@ -718,7 +718,7 @@ namespace snmalloc // empty, but you are not allowed to call this in the empty case. auto last = Object::BHeadPtr::unsafe_from( Object::from_next_ptr(cast_end(0))); - init(head[0], key); + init(address_cast(head[0]), key); return {first, last}; } diff --git a/src/snmalloc/mem/localalloc.h b/src/snmalloc/mem/localalloc.h index 17a6269a6..f2827be88 100644 --- a/src/snmalloc/mem/localalloc.h +++ b/src/snmalloc/mem/localalloc.h @@ -201,7 +201,7 @@ namespace snmalloc if (meta != nullptr) { meta->initialise_large( - chunk.unsafe_ptr(), local_cache.entropy.get_free_list_key()); + address_cast(chunk), local_cache.entropy.get_free_list_key()); core_alloc->laden.insert(meta); } diff --git a/src/snmalloc/mem/metadata.h b/src/snmalloc/mem/metadata.h index dae6ecd54..9dc21eb13 100644 --- a/src/snmalloc/mem/metadata.h +++ b/src/snmalloc/mem/metadata.h @@ -440,8 +440,8 @@ namespace snmalloc /** * Initialise FrontendSlabMetadata for a slab. */ - void - initialise(smallsizeclass_t sizeclass, void* slab, const FreeListKey& key) + void initialise( + smallsizeclass_t sizeclass, address_t slab, const FreeListKey& key) { static_assert( std::is_base_of::value, @@ -462,7 +462,7 @@ namespace snmalloc * * Set needed so immediately moves to slow path. */ - void initialise_large(void* slab, const FreeListKey& key) + void initialise_large(address_t slab, const FreeListKey& key) { // We will push to this just to make the fast path clean. free_queue.init(slab, key); diff --git a/src/snmalloc/mem/remotecache.h b/src/snmalloc/mem/remotecache.h index e63bc3e1c..90e1eee55 100644 --- a/src/snmalloc/mem/remotecache.h +++ b/src/snmalloc/mem/remotecache.h @@ -173,8 +173,8 @@ namespace snmalloc for (auto& l : list) { // We do not need to initialise with a particular slab, so pass - // a nullptr. - l.init(nullptr, key); + // a null address. + l.init(0, key); } capacity = REMOTE_CACHE; } diff --git a/src/test/func/statistics/stats.cc b/src/test/func/statistics/stats.cc index a86deb463..a72efc802 100644 --- a/src/test/func/statistics/stats.cc +++ b/src/test/func/statistics/stats.cc @@ -63,7 +63,7 @@ void debug_check_empty_2() for (size_t i = 0; i < count; i++) { - auto r = a.alloc(128); + auto r = a.alloc(size); allocs.push_back(r); snmalloc::debug_check_empty(&result); if (result != false) From bb1d52c386fcc22a8b589fa51e637d40d68397c8 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 20 Dec 2022 09:18:00 +0000 Subject: [PATCH 14/16] Fix calculation of number of allocations. --- src/test/func/statistics/stats.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/func/statistics/stats.cc b/src/test/func/statistics/stats.cc index a72efc802..b17675c75 100644 --- a/src/test/func/statistics/stats.cc +++ b/src/test/func/statistics/stats.cc @@ -59,7 +59,8 @@ void debug_check_empty_2() snmalloc::Alloc& a = snmalloc::ThreadAlloc::get(); bool result; std::vector allocs; - size_t count = 2048; + // 2GB of allocations + size_t count = 2048 * 1024 * 1024 / size; for (size_t i = 0; i < count; i++) { From d5e0e606730cbe3bf0a429971ae405fca4d7231b Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 20 Dec 2022 09:21:07 +0000 Subject: [PATCH 15/16] Fix calculation of number of allocations. --- src/test/func/statistics/stats.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/func/statistics/stats.cc b/src/test/func/statistics/stats.cc index b17675c75..ed411ee38 100644 --- a/src/test/func/statistics/stats.cc +++ b/src/test/func/statistics/stats.cc @@ -59,8 +59,8 @@ void debug_check_empty_2() snmalloc::Alloc& a = snmalloc::ThreadAlloc::get(); bool result; std::vector allocs; - // 2GB of allocations - size_t count = 2048 * 1024 * 1024 / size; + // 1GB of allocations + size_t count = 1024 * 1024 * 1024 / size; for (size_t i = 0; i < count; i++) { From daa94e4b882ac14e70bc869841040cc51cbe367d Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 20 Dec 2022 09:51:23 +0000 Subject: [PATCH 16/16] Fix test --- src/test/func/statistics/stats.cc | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/test/func/statistics/stats.cc b/src/test/func/statistics/stats.cc index ed411ee38..c8db1cad7 100644 --- a/src/test/func/statistics/stats.cc +++ b/src/test/func/statistics/stats.cc @@ -11,6 +11,7 @@ int main() template void debug_check_empty_1() { + std::cout << "debug_check_empty_1 " << size << std::endl; snmalloc::Alloc& a = snmalloc::ThreadAlloc::get(); bool result; @@ -56,14 +57,19 @@ void debug_check_empty_1() template void debug_check_empty_2() { + std::cout << "debug_check_empty_2 " << size << std::endl; snmalloc::Alloc& a = snmalloc::ThreadAlloc::get(); bool result; std::vector allocs; // 1GB of allocations - size_t count = 1024 * 1024 * 1024 / size; + size_t count = snmalloc::bits::min(2048, 1024 * 1024 * 1024 / size); for (size_t i = 0; i < count; i++) { + if (i % (count / 16) == 0) + { + std::cout << "." << std::flush; + } auto r = a.alloc(size); allocs.push_back(r); snmalloc::debug_check_empty(&result); @@ -74,21 +80,39 @@ void debug_check_empty_2() abort(); } } + std::cout << std::endl; for (size_t i = 0; i < count; i++) { + if (i % (count / 16) == 0) + { + std::cout << "." << std::flush; + } + snmalloc::debug_check_empty(&result); + if (result != false) + { + std::cout << "False empty after " << i << " deallocations of " << size + << std::endl; + abort(); + } a.dealloc(allocs[i]); } + std::cout << std::endl; snmalloc::debug_check_empty(); } int main() { debug_check_empty_1<16>(); + debug_check_empty_1<16384>(); + debug_check_empty_1<65536>(); debug_check_empty_1<1024 * 1024 * 32>(); debug_check_empty_2<32>(); + debug_check_empty_2<16384>(); + debug_check_empty_2<65535>(); debug_check_empty_2<1024 * 1024 * 32>(); + return 0; } #endif \ No newline at end of file