From 8a20a66f87e20170adf9cb9c4f624a4b28fb6676 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Mon, 16 May 2022 13:26:11 +0100 Subject: [PATCH 1/5] Alter FailFast behaviour of memcpy This commit changes the codegen for error messages for failed memcpys. This no longer generates a stack frame and correctly tail calls the error messages generator. It also turns the error messages on in Release builds. This will lead to better adoption experience. --- CMakeLists.txt | 11 ++-- src/snmalloc/global/bounds_checks.h | 95 +++++++++++------------------ src/snmalloc/global/memcpy.h | 13 ++-- src/test/perf/memcpy/memcpy.cc | 7 ++- 4 files changed, 56 insertions(+), 70 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9a5bb261e..5ec334771 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -211,16 +211,13 @@ endif() target_compile_definitions(snmalloc INTERFACE $<$:MALLOC_USABLE_SIZE_QUALIFIER=const>) -# In debug and CI builds, link the backtrace library so that we can get stack +# Link the backtrace library so that we can get stack # traces on errors. find_package(Backtrace) if(${Backtrace_FOUND}) - target_compile_definitions(snmalloc INTERFACE - $<${ci_or_debug}:SNMALLOC_BACKTRACE_HEADER="${Backtrace_HEADER}">) - target_link_libraries(snmalloc INTERFACE - $<${ci_or_debug}:${Backtrace_LIBRARIES}>) - target_include_directories(snmalloc INTERFACE - $<${ci_or_debug}:${Backtrace_INCLUDE_DIRS}>) + target_compile_definitions(snmalloc INTERFACE SNMALLOC_BACKTRACE_HEADER="${Backtrace_HEADER}") + target_link_libraries(snmalloc INTERFACE ${Backtrace_LIBRARIES}) + target_include_directories(snmalloc INTERFACE ${Backtrace_INCLUDE_DIRS}) endif() if(MSVC) diff --git a/src/snmalloc/global/bounds_checks.h b/src/snmalloc/global/bounds_checks.h index 66b67ec43..246fb3889 100644 --- a/src/snmalloc/global/bounds_checks.h +++ b/src/snmalloc/global/bounds_checks.h @@ -22,15 +22,17 @@ namespace snmalloc * error. With it set to false we print a helpful error message and then crash * the process. The process may be in an undefined state by the time the * check fails, so there are potentially security implications to turning this - * off. It defaults to true for debug builds, false for release builds and + * off. It defaults to false for both debug and release builds and * can be overridden by defining the macro `SNMALLOC_FAIL_FAST` to true or * false. + * + * Current default to true will help with adoption experience. */ static constexpr bool FailFast = #ifdef SNMALLOC_FAIL_FAST SNMALLOC_FAIL_FAST #else - !DEBUG + false #endif ; @@ -40,71 +42,48 @@ namespace snmalloc * `p` is the input pointer and `len` is the offset from this pointer of the * bounds. `msg` is the message that will be reported along with the * start and end of the real object's bounds. + * + * Note that this function never returns. We do not mark it [[NoReturn]] + * so as to generate better code. The function claims to return a void*, + * this is so it can be tail called in memcpy. Note [[NoReturn]] prevents + * tailcails in GCC and Clang. */ - SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION inline void - report_fatal_bounds_error [[noreturn]] ( - void* p, size_t len, const char* msg, decltype(ThreadAlloc::get())& alloc) - { - report_fatal_error( - "{}: {} is in allocation {}--{}, offset {} is past the end\n", - msg, - p, - alloc.template external_pointer(p), - alloc.template external_pointer(p), - len); - } - - /** - * The direction for a bounds check. - */ - enum class CheckDirection + SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION inline void* + report_fatal_bounds_error(const void* ptr, size_t len, const char* msg) { - /** - * A read bounds check, performed only when read checks are enabled. - */ - Read, + if constexpr (FailFast) + { + UNUSED(ptr, len, msg); + SNMALLOC_FAST_FAIL(); + } + else + { + auto& alloc = ThreadAlloc::get(); + void* p = const_cast(ptr); - /** - * A write bounds check, performed unconditionally. - */ - Write - }; + auto copy_end = pointer_offset(p, len); + auto object_end = alloc.template external_pointer(p); + report_fatal_error( + "Fatal Error!\n{}: \n\tcopy range [{}, {})\n\tallocation [{}, " + "{})\nrange goes beyond allocation by {} bytes \n", + msg, + p, + copy_end, + alloc.template external_pointer(p), + object_end, + pointer_diff(object_end, copy_end)); + } + } /** * Check whether a pointer + length is in the same object as the pointer. - * Fail with the error message from the third argument if not. * - * The template parameter indicates whether this is a read. If so, this - * function is a no-op when `CheckReads` is false. + * Returns true if the checks succeeds. */ - template< - CheckDirection Direction = CheckDirection::Write, - bool CheckBoth = CheckReads> - SNMALLOC_FAST_PATH_INLINE void - check_bounds(const void* ptr, size_t len, const char* msg = "") + SNMALLOC_FAST_PATH_INLINE bool check_bounds(const void* ptr, size_t len) { - if constexpr ((Direction == CheckDirection::Write) || CheckBoth) - { - auto& alloc = ThreadAlloc::get(); - void* p = const_cast(ptr); + auto& alloc = ThreadAlloc::get(); - if (SNMALLOC_UNLIKELY(!alloc.check_bounds(ptr, len))) - { - if constexpr (FailFast) - { - UNUSED(p, len, msg); - SNMALLOC_FAST_FAIL(); - } - else - { - report_fatal_bounds_error(p, len, msg, alloc); - } - } - } - else - { - UNUSED(ptr, len, msg); - } + return alloc.check_bounds(ptr, len); } - } // namespace snmalloc diff --git a/src/snmalloc/global/memcpy.h b/src/snmalloc/global/memcpy.h index beba37cd2..9b75ad719 100644 --- a/src/snmalloc/global/memcpy.h +++ b/src/snmalloc/global/memcpy.h @@ -320,10 +320,15 @@ namespace snmalloc if constexpr (Checked) { // Check the bounds of the arguments. - check_bounds( - dst, len, "memcpy with destination out of bounds of heap allocation"); - check_bounds( - src, len, "memcpy with source out of bounds of heap allocation"); + if constexpr (CheckReads) + { + if (SNMALLOC_UNLIKELY(!check_bounds(src, len))) + return report_fatal_bounds_error( + src, len, "memcpy with source out of bounds of heap allocation"); + } + if (SNMALLOC_UNLIKELY(!check_bounds(dst, len))) + return report_fatal_bounds_error( + dst, len, "memcpy with destination out of bounds of heap allocation"); } Arch::copy(dst, src, len); diff --git a/src/test/perf/memcpy/memcpy.cc b/src/test/perf/memcpy/memcpy.cc index 7642d60cc..64efad8cc 100644 --- a/src/test/perf/memcpy/memcpy.cc +++ b/src/test/perf/memcpy/memcpy.cc @@ -90,7 +90,12 @@ void memcpy_unchecked(void* dst, const void* src, size_t size) NOINLINE void memcpy_platform_checked(void* dst, const void* src, size_t size) { - check_bounds(dst, size, ""); + if (SNMALLOC_UNLIKELY(!check_bounds(dst, size))) + { + report_fatal_bounds_error(dst, size, ""); + return; + } + memcpy(dst, src, size); } From 1081572aeb5cd88d106dc40d588455e8a6c1f28d Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 19 May 2022 10:39:47 +0100 Subject: [PATCH 2/5] Code review feedback. --- CMakeLists.txt | 11 +++++--- src/snmalloc/global/bounds_checks.h | 41 +++++++++++++++++++---------- src/snmalloc/global/memcpy.h | 9 +++---- 3 files changed, 37 insertions(+), 24 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5ec334771..9a5bb261e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -211,13 +211,16 @@ endif() target_compile_definitions(snmalloc INTERFACE $<$:MALLOC_USABLE_SIZE_QUALIFIER=const>) -# Link the backtrace library so that we can get stack +# In debug and CI builds, link the backtrace library so that we can get stack # traces on errors. find_package(Backtrace) if(${Backtrace_FOUND}) - target_compile_definitions(snmalloc INTERFACE SNMALLOC_BACKTRACE_HEADER="${Backtrace_HEADER}") - target_link_libraries(snmalloc INTERFACE ${Backtrace_LIBRARIES}) - target_include_directories(snmalloc INTERFACE ${Backtrace_INCLUDE_DIRS}) + target_compile_definitions(snmalloc INTERFACE + $<${ci_or_debug}:SNMALLOC_BACKTRACE_HEADER="${Backtrace_HEADER}">) + target_link_libraries(snmalloc INTERFACE + $<${ci_or_debug}:${Backtrace_LIBRARIES}>) + target_include_directories(snmalloc INTERFACE + $<${ci_or_debug}:${Backtrace_INCLUDE_DIRS}>) endif() if(MSVC) diff --git a/src/snmalloc/global/bounds_checks.h b/src/snmalloc/global/bounds_checks.h index 246fb3889..0b102c993 100644 --- a/src/snmalloc/global/bounds_checks.h +++ b/src/snmalloc/global/bounds_checks.h @@ -22,9 +22,8 @@ namespace snmalloc * error. With it set to false we print a helpful error message and then crash * the process. The process may be in an undefined state by the time the * check fails, so there are potentially security implications to turning this - * off. It defaults to false for both debug and release builds and - * can be overridden by defining the macro `SNMALLOC_FAIL_FAST` to true or - * false. + * off. It defaults to false and can be overridden by defining the macro + * `SNMALLOC_FAIL_FAST` to true. * * Current default to true will help with adoption experience. */ @@ -44,11 +43,15 @@ namespace snmalloc * start and end of the real object's bounds. * * Note that this function never returns. We do not mark it [[NoReturn]] - * so as to generate better code. The function claims to return a void*, - * this is so it can be tail called in memcpy. Note [[NoReturn]] prevents - * tailcails in GCC and Clang. + * so as to generate better code, because [[NoReturn]] prevents tailcails + * in GCC and Clang. + * + * The function claims to return a FakeReturn, this is so it can be tail + * called where the bound checked function returns a value, for instance, in + * memcpy it is specialised to void*. */ - SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION inline void* + template + SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION inline FakeReturn report_fatal_bounds_error(const void* ptr, size_t len, const char* msg) { if constexpr (FailFast) @@ -61,17 +64,17 @@ namespace snmalloc auto& alloc = ThreadAlloc::get(); void* p = const_cast(ptr); - auto copy_end = pointer_offset(p, len); + auto range_end = pointer_offset(p, len); auto object_end = alloc.template external_pointer(p); report_fatal_error( - "Fatal Error!\n{}: \n\tcopy range [{}, {})\n\tallocation [{}, " + "Fatal Error!\n{}: \n\trange [{}, {})\n\tallocation [{}, " "{})\nrange goes beyond allocation by {} bytes \n", msg, p, - copy_end, + range_end, alloc.template external_pointer(p), object_end, - pointer_diff(object_end, copy_end)); + pointer_diff(object_end, range_end)); } } @@ -79,11 +82,21 @@ namespace snmalloc * Check whether a pointer + length is in the same object as the pointer. * * Returns true if the checks succeeds. + * + * The template parameter indicates whether the check should be performed. It + * defaults to true. If it is false, the check will always succeed. */ + template SNMALLOC_FAST_PATH_INLINE bool check_bounds(const void* ptr, size_t len) { - auto& alloc = ThreadAlloc::get(); - - return alloc.check_bounds(ptr, len); + if constexpr (PerformCheck) + { + auto& alloc = ThreadAlloc::get(); + return alloc.check_bounds(ptr, len); + } + else + { + return true; + } } } // namespace snmalloc diff --git a/src/snmalloc/global/memcpy.h b/src/snmalloc/global/memcpy.h index 9b75ad719..63cbd6631 100644 --- a/src/snmalloc/global/memcpy.h +++ b/src/snmalloc/global/memcpy.h @@ -320,12 +320,9 @@ namespace snmalloc if constexpr (Checked) { // Check the bounds of the arguments. - if constexpr (CheckReads) - { - if (SNMALLOC_UNLIKELY(!check_bounds(src, len))) - return report_fatal_bounds_error( - src, len, "memcpy with source out of bounds of heap allocation"); - } + if (SNMALLOC_UNLIKELY(!check_bounds(src, len))) + return report_fatal_bounds_error( + src, len, "memcpy with source out of bounds of heap allocation"); if (SNMALLOC_UNLIKELY(!check_bounds(dst, len))) return report_fatal_bounds_error( dst, len, "memcpy with destination out of bounds of heap allocation"); From 719dc4fc163efa776de3ee2a77909992a5b9926e Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 19 May 2022 11:02:22 +0100 Subject: [PATCH 3/5] Missing UNUSED --- src/snmalloc/global/bounds_checks.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/snmalloc/global/bounds_checks.h b/src/snmalloc/global/bounds_checks.h index 0b102c993..378f5439a 100644 --- a/src/snmalloc/global/bounds_checks.h +++ b/src/snmalloc/global/bounds_checks.h @@ -96,6 +96,7 @@ namespace snmalloc } else { + UNUSED(ptr, len); return true; } } From 26977dccd5ec3995f99328fc69c7fc990e5a4ade Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 19 May 2022 11:38:15 +0100 Subject: [PATCH 4/5] Simplify. --- src/snmalloc/global/memcpy.h | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/snmalloc/global/memcpy.h b/src/snmalloc/global/memcpy.h index 63cbd6631..5287f0cc4 100644 --- a/src/snmalloc/global/memcpy.h +++ b/src/snmalloc/global/memcpy.h @@ -317,16 +317,13 @@ namespace snmalloc return dst; } - if constexpr (Checked) - { - // Check the bounds of the arguments. - if (SNMALLOC_UNLIKELY(!check_bounds(src, len))) - return report_fatal_bounds_error( - src, len, "memcpy with source out of bounds of heap allocation"); - if (SNMALLOC_UNLIKELY(!check_bounds(dst, len))) - return report_fatal_bounds_error( - dst, len, "memcpy with destination out of bounds of heap allocation"); - } + // Check the bounds of the arguments. + if (SNMALLOC_UNLIKELY(!check_bounds(src, len))) + return report_fatal_bounds_error( + src, len, "memcpy with source out of bounds of heap allocation"); + if (SNMALLOC_UNLIKELY(!check_bounds(dst, len))) + return report_fatal_bounds_error( + dst, len, "memcpy with destination out of bounds of heap allocation"); Arch::copy(dst, src, len); return orig_dst; From 57f9bc1bc923bcdcf46df26a3880a8c4c5f17ecc Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 19 May 2022 13:30:54 +0100 Subject: [PATCH 5/5] Clang format issues. --- src/snmalloc/global/memcpy.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/snmalloc/global/memcpy.h b/src/snmalloc/global/memcpy.h index 5287f0cc4..ee60cd239 100644 --- a/src/snmalloc/global/memcpy.h +++ b/src/snmalloc/global/memcpy.h @@ -318,7 +318,7 @@ namespace snmalloc } // Check the bounds of the arguments. - if (SNMALLOC_UNLIKELY(!check_bounds(src, len))) + if (SNMALLOC_UNLIKELY(!check_bounds<(Checked && ReadsChecked)>(src, len))) return report_fatal_bounds_error( src, len, "memcpy with source out of bounds of heap allocation"); if (SNMALLOC_UNLIKELY(!check_bounds(dst, len)))