From cd70a7856b06d20fbebdc53c563c03364367fa4a Mon Sep 17 00:00:00 2001 From: David Chisnall Date: Thu, 5 Aug 2021 16:09:37 +0100 Subject: [PATCH] Fix fallout from the merge. - CI merge issues: - The malloc shim libraries are renamed. - CMake gets very unhappy if you don't enable the C language and tries to link with the C compiler instead of the C++ compiler if you do enable it. - The Ubuntu packages for QEMU install a `binfmt_misc` activator for PowerPC64 little-endian, but set the page size to 4 KiB. We then tried to run the tests (which expect 64 KiB pages) and became very confused when `mmap` returned 4 KiB-aligned memory. - Test failures: - Fix all of the issues UBsan found. - Underflow in `pointer_offset` when used to add negative offsets. - `CoreAlloc`'s `LocalState` accessed on a null `CoreAlloc` pointer. - Out of bounds access in the sizeclass list on attempts to access more memory than fits in the VA space. - - There was an integer overflow in `AddressSpace` that could cause it to try to allocate a zero-sized object, get a null pointer, and then try to do something with 0 - {size of the real allocation}. - The malloc tests weren't setting `errno` to 0 before doing calling `malloc`, which should set `errno` on failure, and then checking that `errno` was 0. - Don't call `PAL::error` on PAL allocation failure, return `nullptr`. The PALs were inconsistent about that and the new code expects to be able to report address-space exhaustion. - The malloc checks can behave differently with 0-sized allocations on different platforms but were very fragile about their expectations. - The malloc test didn't report failure for all of the ways that it could fail and so was spuriously passing on some platforms. - The perf test for external pointer is currently very slow on Windows. The number of loops have been reduced and a timeout added for the Windows CI runs. - The logic to capture `errno` across calls was using `decltype(errno)`, which on some platforms where `errno` is a macro evaluated to `int&` and so they captured a reference rather than the value and failed to reset `errno`. - The Apple PAL can set `errno` on `notify_using` if it's called with memory that was not previously passed to `notify_not_using` but was not adequately protected against this and so would sometimes cause `malloc` to set `errno` to `EINVAL`. --- .github/workflows/main.yml | 16 +++++++++++----- src/backend/address_space.h | 7 ++++++- src/ds/address.h | 3 ++- src/mem/localalloc.h | 7 +++++-- src/mem/slaballocator.h | 7 +++++++ src/pal/pal_apple.h | 3 ++- src/pal/pal_bsd_aligned.h | 2 +- src/pal/pal_posix.h | 3 ++- src/pal/pal_windows.h | 4 ---- src/test/func/malloc/malloc.cc | 15 ++++++++++----- .../perf/external_pointer/externalpointer.cc | 4 +++- 11 files changed, 49 insertions(+), 22 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 9c63a70..feabade 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -50,7 +50,7 @@ jobs: - os: ubuntu-latest variant: Clang 10 libstdc++ (Build only) dependencies: "sudo apt install ninja-build" - extra-cmake-flags: "-DCMAKE_CXX_COMPILER=clang++-10 -DCMAKE_CXX_FLAGS=-stdlib=libstdc++" + extra-cmake-flags: "-DCMAKE_CXX_COMPILER=clang++-10 -DCMAKE_C_COMPILER=clang-10 -DCMAKE_CXX_FLAGS=-stdlib=libstdc++" build-only: yes # Don't abort runners if a single one fails fail-fast: false @@ -78,13 +78,11 @@ jobs: if: ${{ matrix.self-host }} working-directory: ${{github.workspace}}/build run: | - sudo cp libsnmallocshim.so libsnmallocshim-16mib.so libsnmallocshim-oe.so /usr/local/lib/ + sudo cp libsnmallocshim.so libsnmallocshim-checks.so /usr/local/lib/ ninja clean LD_PRELOAD=/usr/local/lib/libsnmallocshim.so ninja ninja clean - LD_PRELOAD=/usr/local/lib/libsnmallocshim-16mib.so ninja - ninja clean - LD_PRELOAD=/usr/local/lib/libsnmallocshim-oe.so ninja + LD_PRELOAD=/usr/local/lib/libsnmallocshim-checks.so ninja qemu-crossbuild: strategy: @@ -117,6 +115,13 @@ jobs: sudo add-apt-repository "deb http://apt.llvm.org/focal/ llvm-toolchain-focal-13 main" sudo apt update sudo apt install libstdc++-9-dev-${{ matrix.arch.name }}-cross qemu-user ninja-build clang-13 lld-13 + # The default PowerPC qemu configuration uses the wrong page size. + # Wrap it in a script that fixes this. + sudo update-binfmts --disable qemu-ppc64le + sudo sh -c 'echo ":qemu-ppc64le:M:0:\x7f\x45\x4c\x46\x02\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x15\x00:\xff\xff\xff\xff\xff\xff\xff\xfc\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\x00:`pwd`/ppc64.sh:" > /proc/sys/fs/binfmt_misc/register' + echo '#!/bin/sh' > ppc64.sh + echo '/usr/bin/qemu-ppc64le -p 65536 $@' >> ppc64.sh + chmod +x ppc64.sh - name: Configure run: > RTLD_NAME=${{ matrix.arch.rtld }} @@ -179,6 +184,7 @@ jobs: - name: Test working-directory: ${{ github.workspace }}/build run: ctest -j 2 --interactive-debug-mode 0 --output-on-failure -C ${{ matrix.build-type }} + timeout-minutes: 20 # Job to run clang-format and report errors diff --git a/src/backend/address_space.h b/src/backend/address_space.h index abccd83..fbb8be9 100644 --- a/src/backend/address_space.h +++ b/src/backend/address_space.h @@ -96,7 +96,12 @@ namespace snmalloc else if constexpr (!pal_supports) { // Need at least 2 times the space to guarantee alignment. - size_t needed_size = size * 2; + bool overflow; + size_t needed_size = bits::umul(size, 2, overflow); + if (overflow) + { + return nullptr; + } // Magic number (27) for over-allocating a block of memory // These should be further refined based on experiments. constexpr size_t min_size = bits::one_at_bit(27); diff --git a/src/ds/address.h b/src/ds/address.h index 15fc47b..bfa1ac5 100644 --- a/src/ds/address.h +++ b/src/ds/address.h @@ -31,7 +31,8 @@ namespace snmalloc inline U* pointer_offset(T* base, size_t diff) { SNMALLOC_ASSERT(base != nullptr); /* Avoid UB */ - return reinterpret_cast(reinterpret_cast(base) + diff); + return reinterpret_cast( + reinterpret_cast(base) + static_cast(diff)); } template diff --git a/src/mem/localalloc.h b/src/mem/localalloc.h index ad58071..0d9e747 100644 --- a/src/mem/localalloc.h +++ b/src/mem/localalloc.h @@ -512,8 +512,11 @@ namespace snmalloc ChunkRecord* slab_record = reinterpret_cast(entry.get_metaslab()); slab_record->chunk = CapPtr(p); - ChunkAllocator::dealloc( - core_alloc->get_backend_local_state(), slab_record, slab_sizeclass); + check_init([&](CoreAlloc* core_alloc) { + ChunkAllocator::dealloc( + core_alloc->get_backend_local_state(), slab_record, slab_sizeclass); + return nullptr; + }); return; } diff --git a/src/mem/slaballocator.h b/src/mem/slaballocator.h index 13dc161..367509b 100644 --- a/src/mem/slaballocator.h +++ b/src/mem/slaballocator.h @@ -83,6 +83,13 @@ namespace snmalloc { ChunkAllocatorState& state = SharedStateHandle::get_slab_allocator_state(&local_state); + + if (slab_sizeclass >= NUM_SLAB_SIZES) + { + // Your address space is not big enough for this allocation! + return {nullptr, nullptr}; + } + // Pop a slab auto chunk_record = state.chunk_stack[slab_sizeclass].pop(); diff --git a/src/pal/pal_apple.h b/src/pal/pal_apple.h index 0c24fa9..a36e10c 100644 --- a/src/pal/pal_apple.h +++ b/src/pal/pal_apple.h @@ -160,6 +160,7 @@ namespace snmalloc template static void notify_using(void* p, size_t size) noexcept { + KeepErrno e; SNMALLOC_ASSERT( is_aligned_block(p, size) || (zero_mem == NoZero)); @@ -239,7 +240,7 @@ namespace snmalloc if (unlikely(kr != KERN_SUCCESS)) { - error("Failed to allocate memory\n"); + return nullptr; } return reinterpret_cast(addr); diff --git a/src/pal/pal_bsd_aligned.h b/src/pal/pal_bsd_aligned.h index c636e50..592b842 100644 --- a/src/pal/pal_bsd_aligned.h +++ b/src/pal/pal_bsd_aligned.h @@ -52,7 +52,7 @@ namespace snmalloc 0); if (p == MAP_FAILED) - PALBSD::error("Out of memory"); + return nullptr; return p; } diff --git a/src/pal/pal_posix.h b/src/pal/pal_posix.h index 8f9e2f7..ce932fd 100644 --- a/src/pal/pal_posix.h +++ b/src/pal/pal_posix.h @@ -99,12 +99,13 @@ namespace snmalloc static const int fd = T::anonymous_memory_fd; }; + protected: /** * A RAII class to capture and restore errno */ class KeepErrno { - decltype(errno) cached_errno; + int cached_errno; public: KeepErrno() : cached_errno(errno) {} diff --git a/src/pal/pal_windows.h b/src/pal/pal_windows.h index 6fe3564..98654d1 100644 --- a/src/pal/pal_windows.h +++ b/src/pal/pal_windows.h @@ -177,10 +177,6 @@ namespace snmalloc void* ret = VirtualAlloc2FromApp( nullptr, nullptr, size, flags, PAGE_READWRITE, ¶m, 1); - if (ret == nullptr) - { - error("Failed to allocate memory\n"); - } return ret; } # endif diff --git a/src/test/func/malloc/malloc.cc b/src/test/func/malloc/malloc.cc index c8fc74a..dfd92e2 100644 --- a/src/test/func/malloc/malloc.cc +++ b/src/test/func/malloc/malloc.cc @@ -41,7 +41,7 @@ void check_result(size_t size, size_t align, void* p, int err, bool null) #else const auto exact_size = align == 1; #endif - if (exact_size && (alloc_size != expected_size)) + if (exact_size && (alloc_size != expected_size) && (size != 0)) { printf( "Usable size is %zu, but required to be %zu.\n", @@ -79,7 +79,10 @@ void check_result(size_t size, size_t align, void* p, int err, bool null) } if (failed) + { printf("check_result failed! %p", p); + abort(); + } our_free(p); } @@ -149,7 +152,9 @@ int main(int argc, char** argv) { const size_t size = bits::one_at_bit(sc); printf("malloc: %zu\n", size); + errno = 0; check_result(size, 1, our_malloc(size), SUCCESS, false); + errno = 0; check_result(size + 1, 1, our_malloc(size + 1), SUCCESS, false); } @@ -180,7 +185,7 @@ int main(int argc, char** argv) test_realloc(our_malloc(size), size, SUCCESS, false); test_realloc(our_malloc(size), 0, SUCCESS, true); test_realloc(nullptr, size, SUCCESS, false); - test_realloc(our_malloc(size), (size_t)-1, ENOMEM, true); + test_realloc(our_malloc(size), ((size_t)-1) / 2, ENOMEM, true); for (sizeclass_t sc2 = 0; sc2 < NUM_SIZECLASSES; sc2++) { const size_t size2 = sizeclass_to_size(sc2); @@ -195,7 +200,7 @@ int main(int argc, char** argv) test_realloc(our_malloc(size), size, SUCCESS, false); test_realloc(our_malloc(size), 0, SUCCESS, true); test_realloc(nullptr, size, SUCCESS, false); - test_realloc(our_malloc(size), (size_t)-1, ENOMEM, true); + test_realloc(our_malloc(size), ((size_t)-1) / 2, ENOMEM, true); for (sizeclass_t sc2 = 0; sc2 < (MAX_SIZECLASS_BITS + 4); sc2++) { const size_t size2 = bits::one_at_bit(sc2); @@ -208,7 +213,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, 0, EINVAL, true); + test_posix_memalign(((size_t)-1) / 2, 0, EINVAL, true); test_posix_memalign(OS_PAGE_SIZE, sizeof(uintptr_t) / 2, EINVAL, true); for (size_t align = sizeof(uintptr_t); align < MAX_SIZECLASS_SIZE * 8; @@ -222,7 +227,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, align, ENOMEM, true); + test_posix_memalign(((size_t)-1) / 2, align, ENOMEM, true); test_posix_memalign(0, align + 1, EINVAL, true); } diff --git a/src/test/perf/external_pointer/externalpointer.cc b/src/test/perf/external_pointer/externalpointer.cc index 90aa1ba..6a8616d 100644 --- a/src/test/perf/external_pointer/externalpointer.cc +++ b/src/test/perf/external_pointer/externalpointer.cc @@ -53,7 +53,9 @@ namespace test void test_external_pointer(xoroshiro::p128r64& r) { auto& alloc = ThreadAlloc::get(); -#ifdef NDEBUG + // This is very slow on Windows at the moment. Until this is fixed, help + // CI terminate. +#if defined(NDEBUG) && !defined(_MSC_VER) static constexpr size_t iterations = 10000000; #else # ifdef _MSC_VER