From 8f653f8ad92935311cab49d624aef0941c06cb63 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Wed, 29 May 2024 22:25:37 +0000 Subject: [PATCH] bionic_allocator: more detailed and consistent error reporting. I only came to improve the signature mismatch error, but I was then annoyed by the copy & paste of the other checks. get_chunk_size() seems to be deliberately avoiding any checks, though I think that might be a bug, and there should be a get_chunk_size() that _does_ check for most callers, and a get_chunk_size_unchecked() for the stuff that seems to want to only be "best effort" (but does still have _some_ possibility of aborting, in addition to the possibility of segfaulting). Also a bit of "include what you use" after cider complained about all the unused includes in bionic_allocator.h. Bug: https://issuetracker.google.com/341850283 Change-Id: I278b495601353733af516a2d60ed10feb9cef36b --- libc/bionic/bionic_allocator.cpp | 34 +++++++++++++++----------------- libc/bionic/heap_tagging.cpp | 1 + libc/bionic/malloc_limit.cpp | 1 + libc/private/bionic_allocator.h | 9 +++------ linker/linker_main.cpp | 1 + 5 files changed, 22 insertions(+), 24 deletions(-) diff --git a/libc/bionic/bionic_allocator.cpp b/libc/bionic/bionic_allocator.cpp index 80e8b087d..41baf8b18 100644 --- a/libc/bionic/bionic_allocator.cpp +++ b/libc/bionic/bionic_allocator.cpp @@ -299,7 +299,7 @@ inline void* BionicAllocator::alloc_impl(size_t align, size_t size) { log2_size = kSmallObjectMinSizeLog2; } - return get_small_object_allocator(log2_size)->alloc(); + return get_small_object_allocator_unchecked(log2_size)->alloc(); } void* BionicAllocator::alloc(size_t size) { @@ -330,9 +330,10 @@ inline page_info* BionicAllocator::get_page_info_unchecked(void* ptr) { inline page_info* BionicAllocator::get_page_info(void* ptr) { page_info* info = get_page_info_unchecked(ptr); if (memcmp(info->signature, kSignature, sizeof(kSignature)) != 0) { - async_safe_fatal("invalid pointer %p (page signature mismatch)", ptr); + async_safe_fatal("invalid pointer %p (page signature %04x instead of %04x)", ptr, + *reinterpret_cast(info->signature), + *reinterpret_cast(kSignature)); } - return info; } @@ -353,12 +354,7 @@ void* BionicAllocator::realloc(void* ptr, size_t size) { if (info->type == kLargeObject) { old_size = info->allocated_size - (static_cast(ptr) - reinterpret_cast(info)); } else { - BionicSmallObjectAllocator* allocator = get_small_object_allocator(info->type); - if (allocator != info->allocator_addr) { - async_safe_fatal("invalid pointer %p (page signature mismatch)", ptr); - } - - old_size = allocator->get_block_size(); + old_size = get_small_object_allocator(info, ptr)->get_block_size(); } if (old_size < size) { @@ -377,16 +373,10 @@ void BionicAllocator::free(void* ptr) { } page_info* info = get_page_info(ptr); - if (info->type == kLargeObject) { munmap(info, info->allocated_size); } else { - BionicSmallObjectAllocator* allocator = get_small_object_allocator(info->type); - if (allocator != info->allocator_addr) { - async_safe_fatal("invalid pointer %p (invalid allocator address for the page)", ptr); - } - - allocator->free(ptr); + get_small_object_allocator(info, ptr)->free(ptr); } } @@ -402,7 +392,7 @@ size_t BionicAllocator::get_chunk_size(void* ptr) { return info->allocated_size - (static_cast(ptr) - reinterpret_cast(info)); } - BionicSmallObjectAllocator* allocator = get_small_object_allocator(info->type); + BionicSmallObjectAllocator* allocator = get_small_object_allocator_unchecked(info->type); if (allocator != info->allocator_addr) { // Invalid pointer. return 0; @@ -410,7 +400,7 @@ size_t BionicAllocator::get_chunk_size(void* ptr) { return allocator->get_block_size(); } -BionicSmallObjectAllocator* BionicAllocator::get_small_object_allocator(uint32_t type) { +BionicSmallObjectAllocator* BionicAllocator::get_small_object_allocator_unchecked(uint32_t type) { if (type < kSmallObjectMinSizeLog2 || type > kSmallObjectMaxSizeLog2) { async_safe_fatal("invalid type: %u", type); } @@ -418,3 +408,11 @@ BionicSmallObjectAllocator* BionicAllocator::get_small_object_allocator(uint32_t initialize_allocators(); return &allocators_[type - kSmallObjectMinSizeLog2]; } + +BionicSmallObjectAllocator* BionicAllocator::get_small_object_allocator(page_info* pi, void* ptr) { + BionicSmallObjectAllocator* result = get_small_object_allocator_unchecked(pi->type); + if (result != pi->allocator_addr) { + async_safe_fatal("invalid pointer %p (invalid allocator address for the page)", ptr); + } + return result; +} \ No newline at end of file diff --git a/libc/bionic/heap_tagging.cpp b/libc/bionic/heap_tagging.cpp index c8a025f57..cadab3ceb 100644 --- a/libc/bionic/heap_tagging.cpp +++ b/libc/bionic/heap_tagging.cpp @@ -34,6 +34,7 @@ #include #include #include +#include extern "C" void scudo_malloc_disable_memory_tagging(); extern "C" void scudo_malloc_set_track_allocation_stacks(int); diff --git a/libc/bionic/malloc_limit.cpp b/libc/bionic/malloc_limit.cpp index deb63f4fd..5128a3526 100644 --- a/libc/bionic/malloc_limit.cpp +++ b/libc/bionic/malloc_limit.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include diff --git a/libc/private/bionic_allocator.h b/libc/private/bionic_allocator.h index 342fd5149..987266913 100644 --- a/libc/private/bionic_allocator.h +++ b/libc/private/bionic_allocator.h @@ -28,13 +28,9 @@ #pragma once -#include -#include #include -#include -#include #include -#include +#include const uint32_t kSmallObjectMaxSizeLog2 = 10; const uint32_t kSmallObjectMinSizeLog2 = 4; @@ -120,7 +116,8 @@ class BionicAllocator { inline void* alloc_impl(size_t align, size_t size); inline page_info* get_page_info_unchecked(void* ptr); inline page_info* get_page_info(void* ptr); - BionicSmallObjectAllocator* get_small_object_allocator(uint32_t type); + BionicSmallObjectAllocator* get_small_object_allocator_unchecked(uint32_t type); + BionicSmallObjectAllocator* get_small_object_allocator(page_info* pi, void* ptr); void initialize_allocators(); BionicSmallObjectAllocator* allocators_; diff --git a/linker/linker_main.cpp b/linker/linker_main.cpp index e27fd9175..2b230a870 100644 --- a/linker/linker_main.cpp +++ b/linker/linker_main.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include "linker.h" #include "linker_auxv.h"