From a8dd065fd7397a80c997f203b0b57c497adec646 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 6 Jun 2019 13:21:22 +0100 Subject: [PATCH 1/7] A few checks for easy corruptions Will detect corruption caused by either * Use-after-free * Double-free Neither is comprehensive. Full temporal safety is not possible. This just aids with debugging. --- src/mem/alloc.h | 5 +++++ src/mem/metaslab.h | 55 ++++++++++++++++++++++++++++++++++++++++++++-- src/mem/slab.h | 12 +++++++--- 3 files changed, 67 insertions(+), 5 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index 3ee67c7..af9f741 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -817,6 +817,11 @@ namespace snmalloc { Superslab* super = Superslab::get(p); + +#ifndef NDEBUG + if (p->target_id() != super->get_allocator()->id()) + error("Detected memory corruption. Potential use-after-free"); +#endif if (super->get_kind() == Super) { Slab* slab = Slab::get(p); diff --git a/src/mem/metaslab.h b/src/mem/metaslab.h index e933b19..3c2ff08 100644 --- a/src/mem/metaslab.h +++ b/src/mem/metaslab.h @@ -91,6 +91,32 @@ namespace snmalloc return reinterpret_cast(pointer_offset(slab, link)); } + /// Value used to check for corruptions in a block + static constexpr size_t POISON = + static_cast(bits::is64() ? 0xDEADBEEFDEAD0000 : 0xDEAD0000); + + /// Store next pointer in a block. In Debug using magic value to detect some + /// simple corruptions. + static void store_next(void* p, uint16_t head) + { +#ifdef NDEBUG + *static_cast(p) = head; +#else + *static_cast(p) = head ^ POISON; +#endif + } + + /// Accessor function for the next pointer in a block. + /// In Debug checks for simple corruptions. + static uint16_t follow_next(void* node) + { + size_t next = *static_cast(node); +#ifndef NDEBUG + if ((next ^ POISON) > 0xFFFF) + error("Detected memory corruption. Use-after-free."); +#endif + return static_cast(next); + } bool valid_head(bool is_short) { size_t size = sizeclass_to_size(sizeclass); @@ -122,8 +148,33 @@ namespace snmalloc // Block is not full assert(SLAB_SIZE > accounted_for); - // Walk bump-free-list-segment accounting for unused space + // Walk bump-free-list-segment checking for cycles. + // Using + // https://en.wikipedia.org/wiki/Cycle_detection#Floyd's_Tortoise_and_Hare + // We don't expect a cycle, so worst case is only followed by a crash, so + // slow doesn't mater. uint16_t curr = head; + uint16_t curr_slow = head; + bool both = false; + while ((curr & 1) != 1) + { + curr = follow_next(pointer_offset(slab, curr)); + if (both) + { + curr_slow = follow_next(pointer_offset(slab, curr_slow)); + } + + if (curr == curr_slow) + { + error("Free list contains a cycle, typically indicates double free."); + } + + both = !both; + } + + + // Walk bump-free-list-segment accounting for unused space + curr = head; while ((curr & 1) != 1) { // Check we are looking at a correctly aligned block @@ -137,7 +188,7 @@ namespace snmalloc assert(curr != link); // Iterate bump/free list segment - curr = *reinterpret_cast(pointer_offset(slab, curr)); + curr = follow_next(pointer_offset(slab, curr)); } if (curr != 1) diff --git a/src/mem/slab.h b/src/mem/slab.h index 21afeb9..cdc2e99 100644 --- a/src/mem/slab.h +++ b/src/mem/slab.h @@ -51,8 +51,7 @@ namespace snmalloc void* node = pointer_offset(this, head); // Read the next slot from the memory that's about to be allocated. - uint16_t next = *static_cast(node); - meta.head = next; + meta.head = Metaslab::follow_next(node); p = remove_cache_friendly_offset(node, meta.sizeclass); } @@ -101,6 +100,12 @@ namespace snmalloc Metaslab& meta = super->get_meta(this); bool was_full = meta.is_full(); + +#ifndef NDEBUG + if (meta.is_unused()) + error("Detected potential double free."); +#endif + meta.debug_slab_invariant(is_short(), this); meta.sub_use(); @@ -152,7 +157,8 @@ namespace snmalloc assert(meta.valid_head(is_short())); // Set the next pointer to the previous head. - *static_cast(p) = head; + Metaslab::store_next(p, head); + meta.debug_slab_invariant(is_short(), this); } return Superslab::NoSlabReturn; From 88d9534453250e22a5393555b517f36f2943184a Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Wed, 12 Jun 2019 13:54:14 +0100 Subject: [PATCH 2/7] CR Feedback + clangformat --- src/mem/alloc.h | 1 - src/mem/metaslab.h | 73 ++++++++++++++++++++++++++-------------------- src/mem/slab.h | 2 +- 3 files changed, 43 insertions(+), 33 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index af9f741..ce6c494 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -817,7 +817,6 @@ namespace snmalloc { Superslab* super = Superslab::get(p); - #ifndef NDEBUG if (p->target_id() != super->get_allocator()->id()) error("Detected memory corruption. Potential use-after-free"); diff --git a/src/mem/metaslab.h b/src/mem/metaslab.h index 3c2ff08..2d74747 100644 --- a/src/mem/metaslab.h +++ b/src/mem/metaslab.h @@ -92,27 +92,28 @@ namespace snmalloc } /// Value used to check for corruptions in a block - static constexpr size_t POISON = + static constexpr size_t POISON = static_cast(bits::is64() ? 0xDEADBEEFDEAD0000 : 0xDEAD0000); /// Store next pointer in a block. In Debug using magic value to detect some /// simple corruptions. static void store_next(void* p, uint16_t head) - { + { #ifdef NDEBUG *static_cast(p) = head; #else - *static_cast(p) = head ^ POISON; + *static_cast(p) = + head ^ POISON ^ (static_cast(head) << (bits::BITS - 16)); #endif } /// Accessor function for the next pointer in a block. - /// In Debug checks for simple corruptions. + /// In Debug checks for simple corruptions. static uint16_t follow_next(void* node) - { + { size_t next = *static_cast(node); #ifndef NDEBUG - if ((next ^ POISON) > 0xFFFF) + if (((next ^ POISON) ^ (next << (bits::BITS - 16))) > 0xFFFF) error("Detected memory corruption. Use-after-free."); #endif return static_cast(next); @@ -129,6 +130,38 @@ namespace snmalloc return ((head_start - slab_start) % size) == 0; } + /** + * Check bump-free-list-segment for cycles + * + * Using + * https://en.wikipedia.org/wiki/Cycle_detection#Floyd's_Tortoise_and_Hare + * We don't expect a cycle, so worst case is only followed by a crash, so + * slow doesn't mater. + **/ + void debug_slab_acyclic_free_list(Slab* slab) + { +#ifndef NDEBUG + uint16_t curr = head; + uint16_t curr_slow = head; + bool both = false; + while ((curr & 1) != 1) + { + curr = follow_next(pointer_offset(slab, curr)); + if (both) + { + curr_slow = follow_next(pointer_offset(slab, curr_slow)); + } + + if (curr == curr_slow) + { + error("Free list contains a cycle, typically indicates double free."); + } + + both = !both; + } +#endif + } + void debug_slab_invariant(bool is_short, Slab* slab) { #if !defined(NDEBUG) && !defined(SNMALLOC_CHEAP_CHECKS) @@ -145,36 +178,14 @@ namespace snmalloc // 'link' value is not important if full. return; } + // Block is not full assert(SLAB_SIZE > accounted_for); - // Walk bump-free-list-segment checking for cycles. - // Using - // https://en.wikipedia.org/wiki/Cycle_detection#Floyd's_Tortoise_and_Hare - // We don't expect a cycle, so worst case is only followed by a crash, so - // slow doesn't mater. - uint16_t curr = head; - uint16_t curr_slow = head; - bool both = false; - while ((curr & 1) != 1) - { - curr = follow_next(pointer_offset(slab, curr)); - if (both) - { - curr_slow = follow_next(pointer_offset(slab, curr_slow)); - } - - if (curr == curr_slow) - { - error("Free list contains a cycle, typically indicates double free."); - } - - both = !both; - } - + debug_slab_acyclic_free_list(slab); // Walk bump-free-list-segment accounting for unused space - curr = head; + uint16_t curr = head; while ((curr & 1) != 1) { // Check we are looking at a correctly aligned block diff --git a/src/mem/slab.h b/src/mem/slab.h index cdc2e99..67ff01b 100644 --- a/src/mem/slab.h +++ b/src/mem/slab.h @@ -100,7 +100,7 @@ namespace snmalloc Metaslab& meta = super->get_meta(this); bool was_full = meta.is_full(); - + #ifndef NDEBUG if (meta.is_unused()) error("Detected potential double free."); From 58179783887fa7cf59a9609dee942c4ab49599c0 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Wed, 12 Jun 2019 14:05:02 +0100 Subject: [PATCH 3/7] Make new.cc only compile once Each test was compiling new.cc for every go. This optimises that to just compile it once. --- CMakeLists.txt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3649761..2bb1590 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -112,6 +112,8 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY) enable_testing() + add_library(overide_new STATIC src/override/new.cc) + set(TESTDIR ${CMAKE_CURRENT_SOURCE_DIR}/src/test) subdirlist(TEST_CATEGORIES ${TESTDIR}) list(REVERSE TEST_CATEGORIES) @@ -123,12 +125,12 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY) aux_source_directory(${TESTDIR}/${TEST_CATEGORY}/${TEST} SRC) set(TESTNAME "${TEST_CATEGORY}-${TEST}-${SUPER_SLAB_SIZE}") - add_executable(${TESTNAME} ${SRC} src/override/new.cc) + add_executable(${TESTNAME} ${SRC}) if (${SUPER_SLAB_SIZE} EQUAL 1) target_compile_definitions(${TESTNAME} PRIVATE IS_ADDRESS_SPACE_CONSTRAINED) endif() target_include_directories(${TESTNAME} PRIVATE src) - target_link_libraries(${TESTNAME} snmalloc_lib) + target_link_libraries(${TESTNAME} snmalloc_lib overide_new) if (${TEST} MATCHES "release-.*") message(STATUS "Adding test: ${TESTNAME} only for release configs") add_test(NAME ${TESTNAME} COMMAND ${TESTNAME} CONFIGURATIONS "Release") From d23a1ccbdf8344335b7c38174bae1a5f088fb996 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Wed, 22 May 2019 13:20:43 +0100 Subject: [PATCH 4/7] Make clangformat a macro to allow sharing. --- CMakeLists.txt | 68 ++++++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2bb1590..e5e3597 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -26,6 +26,41 @@ macro(warnings_high) endif() endmacro() +macro(clangformat_targets) + # The clang-format tool is installed under a variety of different names. Try + # to find a sensible one. Only look for 6.0 and 7.0 versions explicitly - we + # don't know whether our clang-format file will work with newer versions of the + # tool + set(CLANG_FORMAT_NAMES + clang-format-7.0 + clang-format-6.0 + clang-format70 + clang-format60 + clang-format) + + # Loop over each of the possible names of clang-format and try to find one. + set(CLANG_FORMAT CLANG_FORMAT-NOTFOUND) + foreach (NAME IN ITEMS ${CLANG_FORMAT_NAMES}) + if (${CLANG_FORMAT} STREQUAL "CLANG_FORMAT-NOTFOUND") + find_program(CLANG_FORMAT ${NAME}) + endif () + endforeach() + + # If we've found a clang-format tool, generate a target for it, otherwise emit + # a warning. + if (${CLANG_FORMAT} STREQUAL "CLANG_FORMAT-NOTFOUND") + message(WARNING "Not generating clangformat target, no clang-format tool found") + else () + message(STATUS "Generating clangformat target using ${CLANG_FORMAT}") + file(GLOB_RECURSE ALL_SOURCE_FILES *.cc *.h *.hh) + add_custom_target( + clangformat + COMMAND ${CLANG_FORMAT} + -i + ${ALL_SOURCE_FILES}) + endif() +endmacro() + # The main target for snmalloc add_library(snmalloc_lib INTERFACE) target_include_directories(snmalloc_lib INTERFACE src/) @@ -145,36 +180,5 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY) endforeach() endforeach() - # The clang-format tool is installed under a variety of different names. Try - # to find a sensible one. Only look for 6.0 and 7.0 versions explicitly - we - # don't know whether our clang-format file will work with newer versions of the - # tool - set(CLANG_FORMAT_NAMES - clang-format-7.0 - clang-format-6.0 - clang-format70 - clang-format60 - clang-format) - - # Loop over each of the possible names of clang-format and try to find one. - set(CLANG_FORMAT CLANG_FORMAT-NOTFOUND) - foreach (NAME IN ITEMS ${CLANG_FORMAT_NAMES}) - if (${CLANG_FORMAT} STREQUAL "CLANG_FORMAT-NOTFOUND") - find_program(CLANG_FORMAT ${NAME}) - endif () - endforeach() - - # If we've found a clang-format tool, generate a target for it, otherwise emit - # a warning. - if (${CLANG_FORMAT} STREQUAL "CLANG_FORMAT-NOTFOUND") - message(WARNING "Not generating clangformat target, no clang-format tool found") - else () - message(STATUS "Generating clangformat target using ${CLANG_FORMAT}") - file(GLOB_RECURSE ALL_SOURCE_FILES *.cc *.h *.hh) - add_custom_target( - clangformat - COMMAND ${CLANG_FORMAT} - -i - ${ALL_SOURCE_FILES}) - endif() + clangformat_targets() endif() From be1898c4e8978deec1bd43089ad30966f5d69fa7 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Wed, 12 Jun 2019 14:12:47 +0100 Subject: [PATCH 5/7] Fixed warning. --- src/mem/metaslab.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mem/metaslab.h b/src/mem/metaslab.h index 2d74747..068edb8 100644 --- a/src/mem/metaslab.h +++ b/src/mem/metaslab.h @@ -159,6 +159,8 @@ namespace snmalloc both = !both; } +#else + UNUSED(slab); #endif } From 7153f2169c456c9ff7b4abbf2ad4b2c1583f603f Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Wed, 12 Jun 2019 15:04:25 +0100 Subject: [PATCH 6/7] Reverting separate compilation of new.cc. --- CMakeLists.txt | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e5e3597..8a5eb4b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -147,8 +147,6 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY) enable_testing() - add_library(overide_new STATIC src/override/new.cc) - set(TESTDIR ${CMAKE_CURRENT_SOURCE_DIR}/src/test) subdirlist(TEST_CATEGORIES ${TESTDIR}) list(REVERSE TEST_CATEGORIES) @@ -160,12 +158,12 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY) aux_source_directory(${TESTDIR}/${TEST_CATEGORY}/${TEST} SRC) set(TESTNAME "${TEST_CATEGORY}-${TEST}-${SUPER_SLAB_SIZE}") - add_executable(${TESTNAME} ${SRC}) + add_executable(${TESTNAME} ${SRC} src/override/new.cc) if (${SUPER_SLAB_SIZE} EQUAL 1) target_compile_definitions(${TESTNAME} PRIVATE IS_ADDRESS_SPACE_CONSTRAINED) endif() target_include_directories(${TESTNAME} PRIVATE src) - target_link_libraries(${TESTNAME} snmalloc_lib overide_new) + target_link_libraries(${TESTNAME} snmalloc_lib) if (${TEST} MATCHES "release-.*") message(STATUS "Adding test: ${TESTNAME} only for release configs") add_test(NAME ${TESTNAME} COMMAND ${TESTNAME} CONFIGURATIONS "Release") From cc6a9775d6127dbd0bac568ff5e76f4ee9b0dfd1 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Wed, 12 Jun 2019 15:26:42 +0100 Subject: [PATCH 7/7] Made checks on client have own macro. --- src/mem/alloc.h | 8 ++++---- src/mem/allocconfig.h | 6 ++++++ src/mem/metaslab.h | 4 ++-- src/mem/slab.h | 2 +- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index ce6c494..703c163 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -469,7 +469,7 @@ namespace snmalloc return; } -# ifndef NDEBUG +# ifdef CHECK_CLIENT if (size > 64 || address_cast(super) != address_cast(p)) { error("Not deallocating start of an object"); @@ -817,7 +817,7 @@ namespace snmalloc { Superslab* super = Superslab::get(p); -#ifndef NDEBUG +#ifdef CHECK_CLIENT if (p->target_id() != super->get_allocator()->id()) error("Detected memory corruption. Potential use-after-free"); #endif @@ -1013,7 +1013,7 @@ namespace snmalloc void small_dealloc(Superslab* super, void* p, uint8_t sizeclass) { -#ifndef NDEBUG +#ifdef CHECK_CLIENT Slab* slab = Slab::get(p); if (!slab->is_start_of_object(super, p)) { @@ -1150,7 +1150,7 @@ namespace snmalloc stats().sizeclass_dealloc(sizeclass); bool was_full = slab->dealloc(p, large_allocator.memory_provider); -#ifndef NDEBUG +#ifdef CHECK_CLIENT if (!is_multiple_of_sizeclass( sizeclass_to_size(sizeclass), address_cast(slab) + SUPERSLAB_SIZE - address_cast(p))) diff --git a/src/mem/allocconfig.h b/src/mem/allocconfig.h index 3b2cb14..e1a3117 100644 --- a/src/mem/allocconfig.h +++ b/src/mem/allocconfig.h @@ -4,6 +4,12 @@ namespace snmalloc { +// The CHECK_CLIENT macro is used to turn on minimal checking of the client +// calling the API correctly. +#if !defined(NDEBUG) && !defined(CHECK_CLIENT) +# define CHECK_CLIENT +#endif + // 0 intermediate bits results in power of 2 small allocs. 1 intermediate // bit gives additional sizeclasses at the midpoint between each power of 2. // 2 intermediate bits gives 3 intermediate sizeclasses, etc. diff --git a/src/mem/metaslab.h b/src/mem/metaslab.h index 068edb8..7bb2a32 100644 --- a/src/mem/metaslab.h +++ b/src/mem/metaslab.h @@ -99,7 +99,7 @@ namespace snmalloc /// simple corruptions. static void store_next(void* p, uint16_t head) { -#ifdef NDEBUG +#ifndef CHECK_CLIENT *static_cast(p) = head; #else *static_cast(p) = @@ -112,7 +112,7 @@ namespace snmalloc static uint16_t follow_next(void* node) { size_t next = *static_cast(node); -#ifndef NDEBUG +#ifdef CHECK_CLIENT if (((next ^ POISON) ^ (next << (bits::BITS - 16))) > 0xFFFF) error("Detected memory corruption. Use-after-free."); #endif diff --git a/src/mem/slab.h b/src/mem/slab.h index 67ff01b..0442740 100644 --- a/src/mem/slab.h +++ b/src/mem/slab.h @@ -101,7 +101,7 @@ namespace snmalloc bool was_full = meta.is_full(); -#ifndef NDEBUG +#ifdef CHECK_CLIENT if (meta.is_unused()) error("Detected potential double free."); #endif