From 86aa28644cbab2d6abab140f52561038a84daa78 Mon Sep 17 00:00:00 2001 From: Robert Norton Date: Thu, 24 Feb 2022 10:09:29 +0000 Subject: [PATCH] Errno fix (#463) Correctly set errno on failure and improve the related test. Previously the malloc test would emit an error message but not abort if the errno was not as expected on failure. This was because the return in the null == true case prevented the check for failed == true at the end of check_result from being reached. To resolve this just abort immediately as in the null case. Also add tests of allocations that are expected to fail for calloc and malloc. To make the tests pass we need to set errno in several places, making sure to keep this off the fast path. We must also take care not to attempt to zero nullptr in case of calloc failure. See microsoft/snmalloc#461 and microsoft/snmalloc#463. --- src/backend/address_space_core.h | 1 + src/mem/chunkallocator.h | 1 + src/mem/external_alloc.h | 7 ++++++- src/mem/localalloc.h | 4 ++-- src/override/malloc.cc | 4 ++++ src/pal/pal_windows.h | 7 ++++++- src/test/func/malloc/malloc.cc | 23 ++++++++++++++++------- 7 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/backend/address_space_core.h b/src/backend/address_space_core.h index fa3aa25..533b173 100644 --- a/src/backend/address_space_core.h +++ b/src/backend/address_space_core.h @@ -151,6 +151,7 @@ namespace snmalloc if (align_bits == (bits::BITS - 1)) { // Out of memory + errno = ENOMEM; return nullptr; } diff --git a/src/mem/chunkallocator.h b/src/mem/chunkallocator.h index 3965dbd..88a9bcf 100644 --- a/src/mem/chunkallocator.h +++ b/src/mem/chunkallocator.h @@ -194,6 +194,7 @@ namespace snmalloc if (slab_sizeclass >= NUM_SLAB_SIZES) { // Your address space is not big enough for this allocation! + errno = ENOMEM; return {nullptr, nullptr}; } diff --git a/src/mem/external_alloc.h b/src/mem/external_alloc.h index 13eb2bc..4eecfa9 100644 --- a/src/mem/external_alloc.h +++ b/src/mem/external_alloc.h @@ -56,15 +56,20 @@ namespace snmalloc::external_alloc if constexpr (bits::BITS == 64) { if (size >= 0x10000000000) + { + errno = ENOMEM; return nullptr; + } } if (alignment < sizeof(void*)) alignment = sizeof(void*); void* result; - if (posix_memalign(&result, alignment, size) != 0) + int err = posix_memalign(&result, alignment, size); + if (err != 0) { + errno = err; result = nullptr; } return result; diff --git a/src/mem/localalloc.h b/src/mem/localalloc.h index f9082e2..6fb378e 100644 --- a/src/mem/localalloc.h +++ b/src/mem/localalloc.h @@ -198,7 +198,7 @@ namespace snmalloc if (meta != nullptr) meta->initialise_large(); - if (zero_mem == YesZero) + if (zero_mem == YesZero && chunk.unsafe_ptr() != nullptr) { SharedStateHandle::Pal::template zero( chunk.unsafe_ptr(), size); @@ -427,7 +427,7 @@ namespace snmalloc // would guarantee. void* result = external_alloc::aligned_alloc( natural_alignment(size), round_size(size)); - if constexpr (zero_mem == YesZero) + if (zero_mem == YesZero && result != nullptr) memset(result, 0, size); return result; #else diff --git a/src/override/malloc.cc b/src/override/malloc.cc index a04e1dc..de0eadc 100644 --- a/src/override/malloc.cc +++ b/src/override/malloc.cc @@ -98,6 +98,10 @@ extern "C" { a.dealloc(ptr); } + else + { + errno = ENOMEM; + } return p; } diff --git a/src/pal/pal_windows.h b/src/pal/pal_windows.h index 1667dce..6f88420 100644 --- a/src/pal/pal_windows.h +++ b/src/pal/pal_windows.h @@ -186,13 +186,18 @@ namespace snmalloc void* ret = VirtualAlloc2FromApp( nullptr, nullptr, size, flags, PAGE_READWRITE, ¶m, 1); + if (ret == nullptr) + errno = ENOMEM; return ret; } # endif static void* reserve(size_t size) noexcept { - return VirtualAlloc(nullptr, size, MEM_RESERVE, PAGE_READWRITE); + void* ret = VirtualAlloc(nullptr, size, MEM_RESERVE, PAGE_READWRITE); + if (ret == nullptr) + errno = ENOMEM; + return ret; } /** diff --git a/src/test/func/malloc/malloc.cc b/src/test/func/malloc/malloc.cc index 367be7c..a5b0acb 100644 --- a/src/test/func/malloc/malloc.cc +++ b/src/test/func/malloc/malloc.cc @@ -16,8 +16,9 @@ void check_result(size_t size, size_t align, void* p, int err, bool null) bool failed = false; if (errno != err && err != SUCCESS) { + // Note: successful calls are allowed to spuriously set errno printf("Expected error: %d but got %d\n", err, errno); - failed = true; + abort(); } if (null) @@ -238,6 +239,11 @@ int main(int argc, char** argv) our_free(nullptr); + /* A very large allocation size that we expect to fail. */ + const size_t too_big_size = ((size_t)-1) / 2; + check_result(too_big_size, 1, our_malloc(too_big_size), ENOMEM, true); + errno = SUCCESS; + for (smallsizeclass_t sc = 0; sc < (MAX_SMALL_SIZECLASS_BITS + 4); sc++) { const size_t size = bits::one_at_bit(sc); @@ -252,6 +258,9 @@ int main(int argc, char** argv) our_free(nullptr); + test_calloc(1, too_big_size, ENOMEM, true); + errno = SUCCESS; + for (smallsizeclass_t sc = 0; sc < NUM_SMALL_SIZECLASSES; sc++) { const size_t size = sizeclass_to_size(sc); @@ -275,7 +284,7 @@ int main(int argc, char** argv) const size_t size = sizeclass_to_size(sc); test_realloc(our_malloc(size), size, SUCCESS, false); test_realloc(nullptr, size, SUCCESS, false); - test_realloc(our_malloc(size), ((size_t)-1) / 2, ENOMEM, true); + test_realloc(our_malloc(size), too_big_size, ENOMEM, true); for (smallsizeclass_t sc2 = 0; sc2 < NUM_SMALL_SIZECLASSES; sc2++) { const size_t size2 = sizeclass_to_size(sc2); @@ -289,7 +298,7 @@ int main(int argc, char** argv) const size_t size = bits::one_at_bit(sc); test_realloc(our_malloc(size), size, SUCCESS, false); test_realloc(nullptr, size, SUCCESS, false); - test_realloc(our_malloc(size), ((size_t)-1) / 2, ENOMEM, true); + test_realloc(our_malloc(size), too_big_size, ENOMEM, true); for (smallsizeclass_t sc2 = 0; sc2 < (MAX_SMALL_SIZECLASS_BITS + 4); sc2++) { const size_t size2 = bits::one_at_bit(sc2); @@ -302,7 +311,7 @@ int main(int argc, char** argv) test_realloc(our_malloc(64), 4194304, SUCCESS, false); test_posix_memalign(0, 0, EINVAL, true); - test_posix_memalign(((size_t)-1) / 2, 0, EINVAL, true); + test_posix_memalign(too_big_size, 0, EINVAL, true); test_posix_memalign(OS_PAGE_SIZE, sizeof(uintptr_t) / 2, EINVAL, true); for (size_t align = sizeof(uintptr_t); align < MAX_SMALL_SIZECLASS_SIZE * 8; @@ -316,7 +325,7 @@ int main(int argc, char** argv) test_memalign(size, align, SUCCESS, false); } test_posix_memalign(0, align, SUCCESS, false); - test_posix_memalign(((size_t)-1) / 2, align, ENOMEM, true); + test_posix_memalign(too_big_size, align, ENOMEM, true); test_posix_memalign(0, align + 1, EINVAL, true); } @@ -327,7 +336,7 @@ int main(int argc, char** argv) test_reallocarray(our_malloc(size), 1, size, SUCCESS, false); test_reallocarray(our_malloc(size), 1, 0, SUCCESS, false); test_reallocarray(nullptr, 1, size, SUCCESS, false); - test_reallocarray(our_malloc(size), 1, ((size_t)-1) / 2, ENOMEM, true); + test_reallocarray(our_malloc(size), 1, too_big_size, ENOMEM, true); for (smallsizeclass_t sc2 = 0; sc2 < (MAX_SMALL_SIZECLASS_BITS + 4); sc2++) { const size_t size2 = bits::one_at_bit(sc2); @@ -351,7 +360,7 @@ int main(int argc, char** argv) printf("realloc alloc failed with %zu\n", size); abort(); } - int r = our_reallocarr(&p, 1, ((size_t)-1) / 2); + int r = our_reallocarr(&p, 1, too_big_size); if (r != ENOMEM) { printf("expected failure on allocation\n");