From 4195e9e1a736c6d3711b1281dd7cc43e7d2f8316 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Fri, 5 Jul 2024 13:58:56 +0200 Subject: [PATCH] MEM_guarded: Improve error reporting usefulness. The main change from this commit is the usage of ASAN poisoning (if available) to trigger an ASAN report on the erroring memory block. The main benefit is the report of the allocation backtrace of that faulty memory block. Pull Request: https://projects.blender.org/blender/blender/pulls/124231 --- .../intern/mallocn_guarded_impl.cc | 91 +++++++++++------- intern/guardedalloc/intern/mallocn_inline.hh | 60 ++++++++++++ intern/guardedalloc/intern/mallocn_intern.hh | 15 --- .../intern/mallocn_lockfree_impl.cc | 92 +++++++++++-------- 4 files changed, 174 insertions(+), 84 deletions(-) diff --git a/intern/guardedalloc/intern/mallocn_guarded_impl.cc b/intern/guardedalloc/intern/mallocn_guarded_impl.cc index 8f8ca9fe7e9..b8a21319de8 100644 --- a/intern/guardedalloc/intern/mallocn_guarded_impl.cc +++ b/intern/guardedalloc/intern/mallocn_guarded_impl.cc @@ -212,18 +212,10 @@ static bool malloc_debug_memset = false; /* implementation */ /* --------------------------------------------------------------------- */ -#ifdef __GNUC__ -__attribute__((format(printf, 1, 2))) -#endif -static void -print_error(const char *str, ...) +static void print_error(const char *message, va_list str_format_args) { - char buf[1024]; - va_list ap; - - va_start(ap, str); - vsnprintf(buf, sizeof(buf), str, ap); - va_end(ap); + char buf[512]; + vsnprintf(buf, sizeof(buf), message, str_format_args); buf[sizeof(buf) - 1] = '\0'; if (error_callback) { @@ -234,6 +226,50 @@ print_error(const char *str, ...) } } +#ifdef __GNUC__ +__attribute__((format(printf, 1, 2))) +#endif +static void +print_error(const char *message, ...) +{ + va_list str_format_args; + va_start(str_format_args, message); + print_error(message, str_format_args); + va_end(str_format_args); +} + +#ifdef __GNUC__ +__attribute__((format(printf, 2, 3))) +#endif +static void +report_error_on_address(const void *vmemh, const char *message, ...) +{ + va_list str_format_args; + + va_start(str_format_args, message); + print_error(message, str_format_args); + va_end(str_format_args); + + if (vmemh == nullptr) { + MEM_trigger_error_on_memory_block(nullptr, 0); + return; + } + + const MemHead *memh = static_cast(vmemh); + memh--; + size_t len = memh->len; + + const void *address = memh; + size_t size = len + sizeof(*memh) + sizeof(MemTail); + if (UNLIKELY(memh->alignment > 0)) { + const MemHeadAligned *memh_aligned = memh; + address = MEMHEAD_REAL_PTR(memh_aligned); + size = len + sizeof(*memh_aligned) + MEMHEAD_ALIGN_PADDING(memh_aligned->alignment) + + sizeof(MemTail); + } + MEM_trigger_error_on_memory_block(address, size); +} + static pthread_mutex_t thread_lock = PTHREAD_MUTEX_INITIALIZER; static void mem_lock_thread() @@ -290,12 +326,9 @@ void *MEM_guarded_dupallocN(const void *vmemh) memh--; if ((memh->flag & MEMHEAD_FLAG_FROM_CPP_NEW) != 0) { - print_error( - "Attempt to use C-style MEM_dupallocN on a pointer created with CPP-style MEM_new or " - "new\n"); -#ifdef WITH_ASSERT_ABORT - abort(); -#endif + report_error_on_address(vmemh, + "Attempt to use C-style MEM_dupallocN on a pointer created with " + "CPP-style MEM_new or new\n"); } #ifndef DEBUG_MEMDUPLINAME @@ -353,12 +386,9 @@ void *MEM_guarded_reallocN_id(void *vmemh, size_t len, const char *str) memh--; if ((memh->flag & MEMHEAD_FLAG_FROM_CPP_NEW) != 0) { - print_error( - "Attempt to use C-style MEM_reallocN on a pointer created with CPP-style MEM_new or " - "new\n"); -#ifdef WITH_ASSERT_ABORT - abort(); -#endif + report_error_on_address(vmemh, + "Attempt to use C-style MEM_reallocN on a pointer created with " + "CPP-style MEM_new or new\n"); } if (LIKELY(memh->alignment == 0)) { @@ -398,12 +428,9 @@ void *MEM_guarded_recallocN_id(void *vmemh, size_t len, const char *str) memh--; if ((memh->flag & MEMHEAD_FLAG_FROM_CPP_NEW) != 0) { - print_error( - "Attempt to use C-style MEM_recallocN on a pointer created with CPP-style MEM_new or " - "new\n"); -#ifdef WITH_ASSERT_ABORT - abort(); -#endif + report_error_on_address(vmemh, + "Attempt to use C-style MEM_recallocN on a pointer created with " + "CPP-style MEM_new or new\n"); } if (LIKELY(memh->alignment == 0)) { @@ -1012,11 +1039,9 @@ void MEM_guarded_freeN(void *vmemh, const AllocationType allocation_type) if (allocation_type != AllocationType::NEW_DELETE && (memh->flag & MEMHEAD_FLAG_FROM_CPP_NEW) != 0) { - print_error( + report_error_on_address( + vmemh, "Attempt to use C-style MEM_freeN on a pointer created with CPP-style MEM_new or new\n"); -#ifdef WITH_ASSERT_ABORT - abort(); -#endif } if (memh->tag1 == MEMFREE && memh->tag2 == MEMFREE) { diff --git a/intern/guardedalloc/intern/mallocn_inline.hh b/intern/guardedalloc/intern/mallocn_inline.hh index 27abaee4b32..5c80c29cbe7 100644 --- a/intern/guardedalloc/intern/mallocn_inline.hh +++ b/intern/guardedalloc/intern/mallocn_inline.hh @@ -9,6 +9,27 @@ * \ingroup intern_mem */ +#include + +/* BEGIN copied from BLI_asan.h */ + +/* Clang defines this. */ +#ifndef __has_feature +# define __has_feature(x) 0 +#endif + +#if (defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)) && \ + (!defined(_MSC_VER) || _MSC_VER > 1929) /* MSVC 2019 and below doesn't ship ASAN headers. */ +# include "sanitizer/asan_interface.h" +# define WITH_ASAN +#else +/* Ensure return value is used. Just using UNUSED_VARS results in a warning. */ +# define ASAN_POISON_MEMORY_REGION(addr, size) (void)(0 && ((size) != 0 && (addr) != NULL)) +# define ASAN_UNPOISON_MEMORY_REGION(addr, size) (void)(0 && ((size) != 0 && (addr) != NULL)) +#endif + +/* END copied from BLI_asan.h */ + MEM_INLINE bool MEM_size_safe_multiply(size_t a, size_t b, size_t *result) { /* A size_t with its high-half bits all set to 1. */ @@ -27,3 +48,42 @@ MEM_INLINE bool MEM_size_safe_multiply(size_t a, size_t b, size_t *result) */ return ((high_bits & (a | b)) == 0 || (*result / b == a)); } + +/** + * Util to trigger an error for the given memory block. + * + * When ASAN is available, it will poison the memory block before accessing it, to trigger a + * detailed ASAN report. Otherwise, it will abort if aborting on assert is set. + */ +#ifdef WITH_ASAN +MEM_INLINE void MEM_trigger_error_on_memory_block(const void *address, const size_t size) +{ + if (address == nullptr) { +# ifdef WITH_ASSERT_ABORT + abort(); +# endif + return; + } + + /* Trigger ASAN error by poisoning the memory and accessing it. */ + ASAN_POISON_MEMORY_REGION(address, size); + char *buffer = const_cast(static_cast(address)); + const char c = *buffer; + *buffer &= 255; + *buffer = c; + + /* In case ASAN is set to not terminate on error, but abort on assert is requested. */ +# ifdef WITH_ASSERT_ABORT + abort(); +# endif + ASAN_UNPOISON_MEMORY_REGION(address, size); +} +#else +MEM_INLINE void MEM_trigger_error_on_memory_block(const void * /* address */, + const size_t /* size */) +{ +# ifdef WITH_ASSERT_ABORT + abort(); +# endif +} +#endif diff --git a/intern/guardedalloc/intern/mallocn_intern.hh b/intern/guardedalloc/intern/mallocn_intern.hh index b6e17005bc0..219dfe680de 100644 --- a/intern/guardedalloc/intern/mallocn_intern.hh +++ b/intern/guardedalloc/intern/mallocn_intern.hh @@ -69,21 +69,6 @@ size_t malloc_usable_size(void *ptr); # define MEM_INLINE static inline #endif -/* BEGIN copied from BLI_asan.h */ - -/* Clang defines this. */ -#ifndef __has_feature -# define __has_feature(x) 0 -#endif - -#if (defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)) && \ - (!defined(_MSC_VER) || _MSC_VER > 1929) /* MSVC 2019 and below doesn't ship ASAN headers. */ -# include "sanitizer/asan_interface.h" -# define WITH_ASAN -#endif - -/* END copied from BLI_asan.h */ - #define IS_POW2(a) (((a) & ((a)-1)) == 0) /* Extra padding which needs to be applied on MemHead to make it aligned. */ diff --git a/intern/guardedalloc/intern/mallocn_lockfree_impl.cc b/intern/guardedalloc/intern/mallocn_lockfree_impl.cc index a6cc4b5bfba..694404d8ca1 100644 --- a/intern/guardedalloc/intern/mallocn_lockfree_impl.cc +++ b/intern/guardedalloc/intern/mallocn_lockfree_impl.cc @@ -74,18 +74,10 @@ enum { #define MEMHEAD_IS_FROM_CPP_NEW(memhead) ((memhead)->len & size_t(MEMHEAD_FLAG_FROM_CPP_NEW)) #define MEMHEAD_LEN(memhead) ((memhead)->len & ~size_t(MEMHEAD_FLAG_MASK)) -#ifdef __GNUC__ -__attribute__((format(printf, 1, 2))) -#endif -static void -print_error(const char *str, ...) +static void print_error(const char *message, va_list str_format_args) { char buf[512]; - va_list ap; - - va_start(ap, str); - vsnprintf(buf, sizeof(buf), str, ap); - va_end(ap); + vsnprintf(buf, sizeof(buf), message, str_format_args); buf[sizeof(buf) - 1] = '\0'; if (error_callback) { @@ -93,6 +85,48 @@ print_error(const char *str, ...) } } +#ifdef __GNUC__ +__attribute__((format(printf, 1, 2))) +#endif +static void +print_error(const char *message, ...) +{ + va_list str_format_args; + va_start(str_format_args, message); + print_error(message, str_format_args); + va_end(str_format_args); +} + +#ifdef __GNUC__ +__attribute__((format(printf, 2, 3))) +#endif +static void +report_error_on_address(const void *vmemh, const char *message, ...) +{ + va_list str_format_args; + + va_start(str_format_args, message); + print_error(message, str_format_args); + va_end(str_format_args); + + if (vmemh == nullptr) { + MEM_trigger_error_on_memory_block(nullptr, 0); + return; + } + + MemHead *memh = MEMHEAD_FROM_PTR(vmemh); + size_t len = MEMHEAD_LEN(memh); + + void *address = memh; + size_t size = len + sizeof(*memh); + if (UNLIKELY(MEMHEAD_IS_ALIGNED(memh))) { + MemHeadAligned *memh_aligned = MEMHEAD_ALIGNED_FROM_PTR(vmemh); + address = MEMHEAD_REAL_PTR(memh_aligned); + size = len + sizeof(*memh_aligned) + MEMHEAD_ALIGN_PADDING(memh_aligned->alignment); + } + MEM_trigger_error_on_memory_block(address, size); +} + size_t MEM_lockfree_allocN_len(const void *vmemh) { if (LIKELY(vmemh)) { @@ -109,10 +143,7 @@ void MEM_lockfree_freeN(void *vmemh, AllocationType allocation_type) } if (UNLIKELY(vmemh == nullptr)) { - print_error("Attempt to free nullptr pointer\n"); -#ifdef WITH_ASSERT_ABORT - abort(); -#endif + report_error_on_address(vmemh, "Attempt to free nullptr pointer\n"); return; } @@ -120,11 +151,9 @@ void MEM_lockfree_freeN(void *vmemh, AllocationType allocation_type) size_t len = MEMHEAD_LEN(memh); if (allocation_type != AllocationType::NEW_DELETE && MEMHEAD_IS_FROM_CPP_NEW(memh)) { - print_error( + report_error_on_address( + vmemh, "Attempt to use C-style MEM_freeN on a pointer created with CPP-style MEM_new or new\n"); -#ifdef WITH_ASSERT_ABORT - abort(); -#endif } memory_usage_block_free(len); @@ -149,12 +178,9 @@ void *MEM_lockfree_dupallocN(const void *vmemh) const size_t prev_size = MEM_lockfree_allocN_len(vmemh); if (MEMHEAD_IS_FROM_CPP_NEW(memh)) { - print_error( - "Attempt to use C-style MEM_dupallocN on a pointer created with CPP-style MEM_new or " - "new\n"); -#ifdef WITH_ASSERT_ABORT - abort(); -#endif + report_error_on_address(vmemh, + "Attempt to use C-style MEM_dupallocN on a pointer created with " + "CPP-style MEM_new or new\n"); } if (UNLIKELY(MEMHEAD_IS_ALIGNED(memh))) { @@ -179,12 +205,9 @@ void *MEM_lockfree_reallocN_id(void *vmemh, size_t len, const char *str) const size_t old_len = MEM_lockfree_allocN_len(vmemh); if (MEMHEAD_IS_FROM_CPP_NEW(memh)) { - print_error( - "Attempt to use C-style MEM_reallocN on a pointer created with CPP-style MEM_new or " - "new\n"); -#ifdef WITH_ASSERT_ABORT - abort(); -#endif + report_error_on_address(vmemh, + "Attempt to use C-style MEM_reallocN on a pointer created with " + "CPP-style MEM_new or new\n"); } if (LIKELY(!MEMHEAD_IS_ALIGNED(memh))) { @@ -225,12 +248,9 @@ void *MEM_lockfree_recallocN_id(void *vmemh, size_t len, const char *str) const size_t old_len = MEM_lockfree_allocN_len(vmemh); if (MEMHEAD_IS_FROM_CPP_NEW(memh)) { - print_error( - "Attempt to use C-style MEM_recallocN on a pointer created with CPP-style MEM_new or " - "new\n"); -#ifdef WITH_ASSERT_ABORT - abort(); -#endif + report_error_on_address(vmemh, + "Attempt to use C-style MEM_recallocN on a pointer created with " + "CPP-style MEM_new or new\n"); } if (LIKELY(!MEMHEAD_IS_ALIGNED(memh))) {