From 93efbb4807acf9e3d3995f891670582a152242a3 Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Fri, 25 Feb 2022 09:59:51 +0000 Subject: [PATCH] Refactor error reporting for bounds checks. (#464) This introduces a very limited formatter that can embed strings and hex representations of pointers / integers in an internal buffer. This is used to format error strings for passing to `Pal::error`. This is used, in turn, by a wrapper for reporting bounds checks, which can be used by external functions to implement bounds checks. This removes the sprintf_l usage from the bounds checks. This provides enough of a format implementation that the tests introduced in #465 can be refactored to use this, instead of their custom `printf` wrapper and that can be used by SNMALLOC_CHECK. This will be a follow-on PR. --- src/ds/helpers.h | 141 +++++++++++++++++++++++++++++++++ src/mem/bounds_checks.h | 108 +++++++++++++++++++++++++ src/mem/chunkallocator.h | 1 + src/override/memcpy.cc | 105 +----------------------- src/pal/pal.h | 26 ++++++ src/test/func/malloc/malloc.cc | 18 +++++ 6 files changed, 296 insertions(+), 103 deletions(-) create mode 100644 src/mem/bounds_checks.h diff --git a/src/ds/helpers.h b/src/ds/helpers.h index 9213294..095bbed 100644 --- a/src/ds/helpers.h +++ b/src/ds/helpers.h @@ -5,6 +5,7 @@ #include #include +#include #include namespace snmalloc @@ -249,4 +250,144 @@ namespace snmalloc static_assert(sizeof(TrivialInitAtomic) == sizeof(char)); static_assert(alignof(TrivialInitAtomic) == alignof(char)); + + /** + * Helper class for building fatal errors. Used by `report_fatal_error` to + * build an on-stack buffer containing the formatted string. + */ + template + class FatalErrorBuilder + { + /** + * The buffer that is used to store the formatted output. + */ + std::array buffer; + + /** + * Space in the buffer, excluding a trailing null terminator. + */ + static constexpr size_t SafeLength = BufferSize - 1; + + /** + * The insert position within `buffer`. + */ + size_t insert = 0; + + /** + * Add argument `i` from the tuple `args` to the output. This is + * implemented recursively because the different tuple elements can have + * different types and so the code for dispatching will depend on the type + * at the index. The compiler will lower this to a jump table in optimised + * builds. + */ + template + void add_tuple_arg(size_t i, const std::tuple& args) + { + if (i == I) + { + append(std::get(args)); + } + else if constexpr (I != 0) + { + add_tuple_arg(i, args); + } + } + + /** + * Append a single character into the buffer. This is the single primitive + * operation permitted on the buffer and performs bounds checks to ensure + * that there is space for the character and for a null terminator. + */ + void append_char(char c) + { + if (insert < SafeLength) + { + buffer[insert++] = c; + } + } + + /** + * Append a string to the buffer. + */ + void append(std::string_view sv) + { + for (auto c : sv) + { + append_char(c); + } + } + + /** + * Append a raw pointer to the buffer as a hex string. + */ + void append(void* ptr) + { + append(static_cast(reinterpret_cast(ptr))); + } + + /** + * Append a size to the buffer, as a hex string. + */ + void append(size_t s) + { + append_char('0'); + append_char('x'); + std::array buf; + const char hexdigits[] = "0123456789abcdef"; + // Length of string including null terminator + static_assert(sizeof(hexdigits) == 0x11); + for (long i = long(buf.size() - 1); i >= 0; i--) + { + buf[static_cast(i)] = hexdigits[s & 0xf]; + s >>= 4; + } + bool skipZero = true; + for (auto c : buf) + { + if (skipZero && (c == '0')) + { + continue; + } + skipZero = false; + append_char(c); + } + if (skipZero) + { + append_char('0'); + } + } + + public: + /** + * Constructor. Takes a format string and the arguments to output. + */ + template + SNMALLOC_FAST_PATH FatalErrorBuilder(const char* fmt, Args... args) + { + buffer[SafeLength] = 0; + size_t arg = 0; + auto args_tuple = std::forward_as_tuple(args...); + for (const char* s = fmt; *s != 0; ++s) + { + if (s[0] == '{' && s[1] == '}') + { + add_tuple_arg(arg++, args_tuple); + ++s; + } + else + { + append_char(*s); + } + } + append_char('\0'); + } + + /** + * Return the error buffer. + */ + const char* get_message() + { + return buffer.data(); + } + }; } // namespace snmalloc diff --git a/src/mem/bounds_checks.h b/src/mem/bounds_checks.h new file mode 100644 index 0000000..bbd1d7b --- /dev/null +++ b/src/mem/bounds_checks.h @@ -0,0 +1,108 @@ +#pragma once +#include "../snmalloc.h" + +namespace snmalloc +{ + /** + * Should we check loads? This defaults to on in debug builds, off in + * release (store-only checks) and can be overridden by defining the macro + * `SNMALLOC_CHECK_LOADS` to true or false. + */ + static constexpr bool CheckReads = +#ifdef SNMALLOC_CHECK_LOADS + SNMALLOC_CHECK_LOADS +#else + DEBUG +#endif + ; + + /** + * Should we fail fast when we encounter an error? With this set to true, we + * just issue a trap instruction and crash the process once we detect an + * 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. + */ + static constexpr bool FailFast = +#ifdef SNMALLOC_FAIL_FAST + SNMALLOC_FAIL_FAST +#else + !DEBUG +#endif + ; + + /** + * Report an error message for a failed bounds check and then abort the + * program. + * `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. + */ + template + SNMALLOC_FAST_PATH_INLINE void + check_bounds(const void* ptr, size_t len, const char* msg = "") + { + if constexpr ((Direction == CheckDirection::Write) || CheckReads) + { + 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); + } + } + +} diff --git a/src/mem/chunkallocator.h b/src/mem/chunkallocator.h index 88a9bcf..bd21034 100644 --- a/src/mem/chunkallocator.h +++ b/src/mem/chunkallocator.h @@ -1,5 +1,6 @@ #pragma once +#include "../backend/backend_concept.h" #include "../ds/mpmcstack.h" #include "../ds/spmcstack.h" #include "../mem/metaslab.h" diff --git a/src/override/memcpy.cc b/src/override/memcpy.cc index acfc6a6..9d96eba 100644 --- a/src/override/memcpy.cc +++ b/src/override/memcpy.cc @@ -1,55 +1,10 @@ +#include "../mem/bounds_checks.h" #include "override.h" -#include -#include -#include -#if __has_include() -# include -#endif - using namespace snmalloc; -// glibc lacks snprintf_l -#if defined(__linux__) || defined(__OpenBSD__) || defined(__DragonFly__) || \ - defined(__HAIKU__) || defined(__sun) -# define snprintf_l(buf, size, loc, msg, ...) \ - snprintf(buf, size, msg, __VA_ARGS__) -// Windows has it with an underscore prefix -#elif defined(_MSC_VER) -# define snprintf_l(buf, size, loc, msg, ...) \ - _snprintf_s_l(buf, size, _TRUNCATE, msg, loc, __VA_ARGS__) -#endif - namespace { - /** - * Should we check loads? This defaults to on in debug builds, off in - * release (store-only checks) - */ - static constexpr bool CheckReads = -#ifdef SNMALLOC_CHECK_LOADS - SNMALLOC_CHECK_LOADS -#else - DEBUG -#endif - ; - - /** - * Should we fail fast when we encounter an error? With this set to true, we - * just issue a trap instruction and crash the process once we detect an - * 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. - */ - static constexpr bool FailFast = -#ifdef SNMALLOC_FAIL_FAST - SNMALLOC_FAIL_FAST -#else - !DEBUG -#endif - ; - /** * The largest register size that we can use for loads and stores. These * types are expected to work for overlapping copies: we can always load them @@ -92,62 +47,6 @@ namespace #endif } - SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION void crashWithMessage - [[noreturn]] ( - void* p, size_t len, const char* msg, decltype(ThreadAlloc::get())& alloc) - { - // We're going to crash the program now, but try to avoid heap - // allocations if possible, since the heap may be in an undefined - // state. - std::array buffer; - snprintf_l( - buffer.data(), - buffer.size(), - /* Force C locale */ nullptr, - "%s: %p is in allocation %p--%p, offset 0x%zx is past the end.\n", - msg, - p, - alloc.template external_pointer(p), - alloc.template external_pointer(p), - len); - Pal::error(buffer.data()); - } - - /** - * 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. - */ - template - SNMALLOC_FAST_PATH_INLINE void - check_bounds(const void* ptr, size_t len, const char* msg = "") - { - if constexpr (!IsRead || CheckReads) - { - 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 - { - crashWithMessage(p, len, msg, alloc); - } - } - } - else - { - UNUSED(ptr, len, msg); - } - } - /** * Copy a block using the specified size. This copies as many complete * chunks of size `Size` as are possible from `len`. @@ -205,7 +104,7 @@ namespace // Check the bounds of the arguments. check_bounds( dst, len, "memcpy with destination out of bounds of heap allocation"); - check_bounds( + check_bounds( src, len, "memcpy with source out of bounds of heap allocation"); } // If this is a small size, do byte-by-byte copies. diff --git a/src/pal/pal.h b/src/pal/pal.h index 096df25..c18c165 100644 --- a/src/pal/pal.h +++ b/src/pal/pal.h @@ -142,4 +142,30 @@ namespace snmalloc # endif "Page size from system header does not match snmalloc config page size."); #endif + + /** + * Report a fatal error via a PAL-specific error reporting mechanism. This + * takes a format string and a set of arguments. The format string indicates + * the remaining arguments with "{}". This could be extended later to + * support indexing fairly easily, if we ever want to localise these error + * messages. + * + * The following are supported as arguments: + * + * - Characters (`char`), printed verbatim. + * - Strings (anything convertible to `std::string_view`), typically string + * literals because nothing on this path should be performing heap + * allocations. Printed verbatim. + * - Raw pointers (void*), printed as hex strings. + * - Integers (convertible to `size_t`), printed as hex strings. + * + * These types should be sufficient for allocator-related error messages. + */ + template + [[noreturn]] inline void report_fatal_error(Args... args) + { + FatalErrorBuilder msg{std::forward(args)...}; + Pal::error(msg.get_message()); + } + } // namespace snmalloc diff --git a/src/test/func/malloc/malloc.cc b/src/test/func/malloc/malloc.cc index a5b0acb..5426956 100644 --- a/src/test/func/malloc/malloc.cc +++ b/src/test/func/malloc/malloc.cc @@ -237,6 +237,24 @@ int main(int argc, char** argv) setup(); + // Smoke test the fatal error builder. Check that it can generate strings + // including all of the kinds of things that it expects to be able to format. + void* fakeptr = reinterpret_cast(static_cast(0x42)); + FatalErrorBuilder<1024> b{ + "testing pointer {} size_t {} message, {} world, null is {}", + fakeptr, + size_t(42), + "hello", + nullptr}; + if ( + strcmp( + "testing pointer 0x42 size_t 0x2a message, hello world, null is 0x0", + b.get_message()) != 0) + { + printf("Incorrect rendering of fatal error message: %s\n", b.get_message()); + abort(); + } + our_free(nullptr); /* A very large allocation size that we expect to fail. */