From c445de8eb47578f6da1f73e5f88aac8e90b6a9cf Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 19 May 2022 14:20:45 +0100 Subject: [PATCH] Alter FailFast behaviour of memcpy (#526) 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. --- src/snmalloc/global/bounds_checks.h | 115 +++++++++++++--------------- src/snmalloc/global/memcpy.h | 13 ++-- src/test/perf/memcpy/memcpy.cc | 7 +- 3 files changed, 66 insertions(+), 69 deletions(-) diff --git a/src/snmalloc/global/bounds_checks.h b/src/snmalloc/global/bounds_checks.h index 66b67ec..378f543 100644 --- a/src/snmalloc/global/bounds_checks.h +++ b/src/snmalloc/global/bounds_checks.h @@ -22,15 +22,16 @@ 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 - * 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. */ static constexpr bool FailFast = #ifdef SNMALLOC_FAIL_FAST SNMALLOC_FAIL_FAST #else - !DEBUG + false #endif ; @@ -40,71 +41,63 @@ 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. - */ - 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 - { - /** - * A read bounds check, performed only when read checks are enabled. - */ - Read, - - /** - * A write bounds check, performed unconditionally. - */ - Write - }; - - /** - * 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. + * Note that this function never returns. We do not mark it [[NoReturn]] + * 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*. */ - template< - CheckDirection Direction = CheckDirection::Write, - bool CheckBoth = CheckReads> - SNMALLOC_FAST_PATH_INLINE void - check_bounds(const void* ptr, size_t len, const char* msg = "") + template + SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION inline FakeReturn + report_fatal_bounds_error(const void* ptr, size_t len, const char* msg) { - if constexpr ((Direction == CheckDirection::Write) || CheckBoth) + if constexpr (FailFast) + { + UNUSED(ptr, len, msg); + SNMALLOC_FAST_FAIL(); + } + else { auto& alloc = ThreadAlloc::get(); void* p = const_cast(ptr); - 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); + auto range_end = pointer_offset(p, len); + auto object_end = alloc.template external_pointer(p); + report_fatal_error( + "Fatal Error!\n{}: \n\trange [{}, {})\n\tallocation [{}, " + "{})\nrange goes beyond allocation by {} bytes \n", + msg, + p, + range_end, + alloc.template external_pointer(p), + object_end, + pointer_diff(object_end, range_end)); } } + /** + * 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) + { + if constexpr (PerformCheck) + { + auto& alloc = ThreadAlloc::get(); + return alloc.check_bounds(ptr, len); + } + else + { + UNUSED(ptr, len); + return true; + } + } } // namespace snmalloc diff --git a/src/snmalloc/global/memcpy.h b/src/snmalloc/global/memcpy.h index beba37c..ee60cd2 100644 --- a/src/snmalloc/global/memcpy.h +++ b/src/snmalloc/global/memcpy.h @@ -317,14 +317,13 @@ namespace snmalloc return dst; } - if constexpr (Checked) - { - // Check the bounds of the arguments. - check_bounds( - dst, len, "memcpy with destination out of bounds of heap allocation"); - check_bounds( + // Check the bounds of the arguments. + 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))) + return report_fatal_bounds_error( + dst, len, "memcpy with destination out of bounds of heap allocation"); Arch::copy(dst, src, len); return orig_dst; diff --git a/src/test/perf/memcpy/memcpy.cc b/src/test/perf/memcpy/memcpy.cc index 7642d60..64efad8 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); }