diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index bed0e70..8383e8d 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -18,6 +18,13 @@ jobs: # Build each combination of OS and release/debug variants os: [ "ubuntu-latest", "ubuntu-18.04", "macos-11", "macos-10.15", "freebsd-12.2", "freebsd-13.0" ] build-type: [ Release, Debug ] + # Extra cmake flags. GitHub Actions matrix overloads `include` to mean + # 'add extra things to a job' and 'add jobs'. You can add extra things + # to a job by specifying things that exist in a job created from the + # matrix definition and adding things. You can specify extra jobs by + # specifying properties that don't match existing jobs. We use + # `cmake-flags` to add cmake flags to all jobs matching a pattern and + # `extra-cmake-flags` to specify a new job with custom CMake flags. extra-cmake-flags: [ "" ] # Modify the complete matrix include: @@ -36,16 +43,23 @@ jobs: - os: "macos-10.15" dependencies: "rm -f /usr/local/bin/2to3 ; brew update && brew install ninja" # Skip the tests for the FreeBSD release builds + # Also build-test the checked memcpy implementation while doing these. + # It is run-tested on Linux and should be the same everywhere. - os: "freebsd-13.0" build-type: Release build-only: yes + cmake-flags: "-DSNMALLOC_MEMCPY_BOUNDS=ON -DSNMALLOC_CHECK_LOADS=ON" - os: "freebsd-12.2" build-type: Debug build-only: yes - # Add the self-host build + cmake-flags: "-DSNMALLOC_MEMCPY_BOUNDS=ON -DSNMALLOC_CHECK_LOADS=ON" + # Add the self-host build, using the bounds-checked memcpy in + # maximally paranoid mode (checking loads and stores) - os: "ubuntu-latest" build-type: Debug self-host: true + extra-cmake-flags: "-DSNMALLOC_MEMCPY_BOUNDS=ON -DSNMALLOC_CHECK_LOADS=ON" + dependencies: "sudo apt install ninja-build" # Extra build to check using pthread library for destructing local state. - os: "ubuntu-latest" variant: "Ubuntu (with pthread destructors)." diff --git a/CMakeLists.txt b/CMakeLists.txt index 8d0d613..534331e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -19,6 +19,8 @@ option(SNMALLOC_USE_CXX17 "Build as C++17 for legacy support." OFF) # Options that apply only if we're not building the header-only library cmake_dependent_option(SNMALLOC_RUST_SUPPORT "Build static library for rust" OFF "NOT SNMALLOC_HEADER_ONLY_LIBRARY" OFF) cmake_dependent_option(SNMALLOC_STATIC_LIBRARY "Build static libraries" ON "NOT SNMALLOC_HEADER_ONLY_LIBRARY" OFF) +cmake_dependent_option(SNMALLOC_MEMCPY_BOUNDS "Perform bounds checks on memcpy with heap objects" OFF "NOT SNMALLOC_HEADER_ONLY_LIBRARY" OFF) +cmake_dependent_option(SNMALLOC_CHECK_LOADS "Perform bounds checks on the source argument to memcpy with heap objects" OFF "SNMALLOC_MEMCPY_BOUNDS" OFF) cmake_dependent_option(SNMALLOC_OPTIMISE_FOR_CURRENT_MACHINE "Compile for current machine architecture" Off "NOT SNMALLOC_HEADER_ONLY_LIBRARY" OFF) if (NOT SNMALLOC_HEADER_ONLY_LIBRARY) # Pick a sensible default for the thread cleanup mechanism @@ -291,20 +293,27 @@ if(NOT SNMALLOC_HEADER_ONLY_LIBRARY) target_link_options(${name} PRIVATE $<$:-Wl,--icf=all -fuse-ld=lld>) endif() + target_compile_definitions(${name} PRIVATE + SNMALLOC_CHECK_LOADS=$,true,false>) + install(TARGETS ${name} EXPORT snmallocConfig) endfunction() + set(SHIM_FILES src/override/new.cc) + if (SNMALLOC_MEMCPY_BOUNDS) + list(APPEND SHIM_FILES src/override/memcpy.cc) + endif () + if (SNMALLOC_STATIC_LIBRARY) - add_shim(snmallocshim-static STATIC src/override/new.cc) + add_shim(snmallocshim-static STATIC ${SHIM_FILES}) target_compile_definitions(snmallocshim-static PRIVATE SNMALLOC_STATIC_LIBRARY_PREFIX=${SNMALLOC_STATIC_LIBRARY_PREFIX}) endif () if(NOT WIN32) - set(SHARED_FILES src/override/new.cc) - add_shim(snmallocshim SHARED ${SHARED_FILES}) - add_shim(snmallocshim-checks SHARED ${SHARED_FILES}) + add_shim(snmallocshim SHARED ${SHIM_FILES}) + add_shim(snmallocshim-checks SHARED ${SHIM_FILES}) target_compile_definitions(snmallocshim-checks PRIVATE SNMALLOC_CHECK_CLIENT) endif() diff --git a/src/backend/commonconfig.h b/src/backend/commonconfig.h index 689a139..2e11879 100644 --- a/src/backend/commonconfig.h +++ b/src/backend/commonconfig.h @@ -115,4 +115,26 @@ namespace snmalloc &unused_remote != fake_large_remote, "Compilation should ensure these are different"); }; + + /** + * SFINAE helper. Matched only if `T` implements `is_initialised`. Calls + * it if it exists. + */ + template + SNMALLOC_FAST_PATH auto call_is_initialised(T*, int) + -> decltype(T::is_initialised()) + { + return T::is_initialised(); + } + + /** + * SFINAE helper. Matched only if `T` does not implement `is_initialised`. + * Unconditionally returns true if invoked. + */ + template + SNMALLOC_FAST_PATH auto call_is_initialised(T*, long) + { + return true; + } + } // namespace snmalloc diff --git a/src/backend/pagemap.h b/src/backend/pagemap.h index eae4703..7a794c1 100644 --- a/src/backend/pagemap.h +++ b/src/backend/pagemap.h @@ -244,7 +244,14 @@ namespace snmalloc } // This means external pointer on Windows will be slow. - if constexpr (potentially_out_of_range && !pal_supports) + // TODO: With SNMALLOC_CHECK_CLIENT we should map that region read-only, + // not no-access, but this requires a PAL change. + if constexpr ( + potentially_out_of_range +#ifndef SNMALLOC_CHECK_CLIENT + && !pal_supports +#endif + ) { register_range(p, 1); } diff --git a/src/ds/address.h b/src/ds/address.h index bfa1ac5..81739ac 100644 --- a/src/ds/address.h +++ b/src/ds/address.h @@ -231,11 +231,11 @@ namespace snmalloc * expected to point to the base of some (sub)allocation into which cursor * points; would-be negative answers trip an assertion in debug builds. */ - inline size_t pointer_diff(void* base, void* cursor) + inline size_t pointer_diff(const void* base, const void* cursor) { SNMALLOC_ASSERT(cursor >= base); return static_cast( - static_cast(cursor) - static_cast(base)); + static_cast(cursor) - static_cast(base)); } template< diff --git a/src/ds/defines.h b/src/ds/defines.h index d8c4955..b0aab14 100644 --- a/src/ds/defines.h +++ b/src/ds/defines.h @@ -15,6 +15,7 @@ # define SNMALLOC_PURE # define SNMALLOC_COLD # define SNMALLOC_REQUIRE_CONSTINIT +# define SNMALLOC_UNUSED_FUNCTION #else # define likely(x) __builtin_expect(!!(x), 1) # define unlikely(x) __builtin_expect(!!(x), 0) @@ -25,6 +26,7 @@ # define SNMALLOC_FAST_PATH_LAMBDA SNMALLOC_FAST_PATH # define SNMALLOC_PURE __attribute__((const)) # define SNMALLOC_COLD __attribute__((cold)) +# define SNMALLOC_UNUSED_FUNCTION __attribute((unused)) # ifdef __clang__ # define SNMALLOC_REQUIRE_CONSTINIT \ [[clang::require_constant_initialization]] diff --git a/src/mem/localalloc.h b/src/mem/localalloc.h index 36f4544..761754e 100644 --- a/src/mem/localalloc.h +++ b/src/mem/localalloc.h @@ -287,27 +287,6 @@ namespace snmalloc return local_cache.remote_allocator; } - /** - * SFINAE helper. Matched only if `T` implements `is_initialised`. Calls - * it if it exists. - */ - template - SNMALLOC_FAST_PATH auto call_is_initialised(T*, int) - -> decltype(T::is_initialised()) - { - return T::is_initialised(); - } - - /** - * SFINAE helper. Matched only if `T` does not implement `is_initialised`. - * Unconditionally returns true if invoked. - */ - template - SNMALLOC_FAST_PATH auto call_is_initialised(T*, long) - { - return true; - } - /** * Call `SharedStateHandle::is_initialised()` if it is implemented, * unconditionally returns true otherwise. @@ -639,9 +618,9 @@ namespace snmalloc if constexpr (location == Start) return start; else if constexpr (location == End) - return pointer_offset(start, rsize); - else return pointer_offset(start, rsize - 1); + else + return pointer_offset(start, rsize); } #else UNUSED(p_raw); @@ -649,7 +628,7 @@ namespace snmalloc if constexpr ((location == End) || (location == OnePastEnd)) // We don't know the End, so return MAX_PTR - return pointer_offset(nullptr, UINTPTR_MAX); + return reinterpret_cast(UINTPTR_MAX); else // We don't know the Start, so return MIN_PTR return nullptr; diff --git a/src/override/malloc.cc b/src/override/malloc.cc index 0a9ad36..6db61ae 100644 --- a/src/override/malloc.cc +++ b/src/override/malloc.cc @@ -1,35 +1,10 @@ -// Core implementation of snmalloc independent of the configuration mode -#include "../snmalloc_core.h" - -#ifndef SNMALLOC_PROVIDE_OWN_CONFIG -# include "../backend/globalconfig.h" -// The default configuration for snmalloc is used if alternative not defined -namespace snmalloc -{ - using Alloc = snmalloc::LocalAllocator; -} // namespace snmalloc -#endif - -// User facing API surface, needs to know what `Alloc` is. -#include "../snmalloc_front.h" +#include "override.h" #include #include using namespace snmalloc; -#ifndef SNMALLOC_EXPORT -# define SNMALLOC_EXPORT -#endif -#ifdef SNMALLOC_STATIC_LIBRARY_PREFIX -# define __SN_CONCAT(a, b) a##b -# define __SN_EVALUATE(a, b) __SN_CONCAT(a, b) -# define SNMALLOC_NAME_MANGLE(a) \ - __SN_EVALUATE(SNMALLOC_STATIC_LIBRARY_PREFIX, a) -#elif !defined(SNMALLOC_NAME_MANGLE) -# define SNMALLOC_NAME_MANGLE(a) a -#endif - #ifndef MALLOC_USABLE_SIZE_QUALIFIER # define MALLOC_USABLE_SIZE_QUALIFIER #endif diff --git a/src/override/memcpy.cc b/src/override/memcpy.cc new file mode 100644 index 0000000..4d41bf3 --- /dev/null +++ b/src/override/memcpy.cc @@ -0,0 +1,232 @@ +#include "override.h" + +#include +#include +#include +#if __has_include() +# include +#endif + +using namespace snmalloc; + +// glibc lacks snprintf_l +#ifdef __linux__ +# 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_l(buf, size, 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 +# ifdef NDEBUG + false +# else + true +# endif +#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 +# ifdef NDEBUG + true +# else + false +# endif +#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 + * into a register and store them. Note that this is at the C abstract + * machine level: the compiler may spill temporaries to the stack, just not + * to the source or destination object. + */ + static constexpr size_t LargestRegisterSize = +#ifdef __AVX__ + 32 +#elif defined(__SSE__) + 16 +#else + sizeof(uint64_t) +#endif + ; + + /** + * Copy a single element of a specified size. Uses a compiler builtin that + * expands to a single load and store. + */ + template + SNMALLOC_FAST_PATH inline void copy_one(void* dst, const void* src) + { +#if __has_builtin(__builtin_memcpy_inline) + __builtin_memcpy_inline(dst, src, Size); +#else + // Define a structure of size `Size` that has alignment 1 and a default + // copy-assignment operator. We can then copy the data as this type. The + // compiler knows the exact width and so will generate the correct wide + // instruction for us (clang 10 and gcc 12 both generate movups for the + // 16-byte version of this when targeting SSE. + struct Block + { + char data[Size]; + }; + auto* d = static_cast(dst); + auto* s = static_cast(src); + *d = *s; +#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 (unlikely( + pointer_diff(ptr, alloc.external_pointer(p)) < len)) + { + if constexpr (FailFast) + { + UNUSED(ptr); + UNUSED(len); + UNUSED(msg); + __builtin_trap(); + } + else + { + crashWithMessage(p, len, msg, alloc); + } + } + } + else + { + UNUSED(ptr); + UNUSED(len); + UNUSED(msg); + } + } + + /** + * Copy a block using the specified size. This copies as many complete + * chunks of size `Size` as are possible from `len`. + */ + template + SNMALLOC_FAST_PATH inline void + block_copy(void* dst, const void* src, size_t len) + { + for (size_t i = 0; (i + Size) <= len; i += Size) + { + copy_one(pointer_offset(dst, i), pointer_offset(src, i)); + } + } + + /** + * Perform an overlapping copy of the end. This will copy one (potentially + * unaligned) `T` from the end of the source to the end of the destination. + * This may overlap other bits of the copy. + */ + template + SNMALLOC_FAST_PATH inline void + copy_end(void* dst, const void* src, size_t len) + { + copy_one( + pointer_offset(dst, len - Size), pointer_offset(src, len - Size)); + } + + /** + * Predicate indicating whether the source and destination are sufficiently + * aligned to be copied as aligned chunks of `Size` bytes. + */ + template + SNMALLOC_FAST_PATH bool is_aligned_memcpy(void* dst, const void* src) + { + return (pointer_align_down(const_cast(src)) == src) && + (pointer_align_down(dst) == dst); + } +} + +extern "C" +{ + /** + * Snmalloc checked memcpy. + */ + SNMALLOC_EXPORT void* + SNMALLOC_NAME_MANGLE(memcpy)(void* dst, const void* src, size_t len) + { + // 0 is a very common size for memcpy and we don't need to do external + // pointer checks if we hit it. It's also the fastest case, to encourage + // the compiler to favour the other cases. + if (unlikely(len == 0)) + { + return dst; + } + // Check the bounds of the arguments. + check_bounds( + dst, len, "memcpy with destination out of bounds of heap allocation"); + check_bounds( + src, len, "memcpy with source out of bounds of heap allocation"); + // If this is a small size, do byte-by-byte copies. + if (len < LargestRegisterSize) + { + block_copy<1>(dst, src, len); + return dst; + } + block_copy(dst, src, len); + copy_end(dst, src, len); + return dst; + } +} diff --git a/src/override/override.h b/src/override/override.h new file mode 100644 index 0000000..a9aedbe --- /dev/null +++ b/src/override/override.h @@ -0,0 +1,28 @@ +#pragma once + +// Core implementation of snmalloc independent of the configuration mode +#include "../snmalloc_core.h" + +#ifndef SNMALLOC_PROVIDE_OWN_CONFIG +# include "../backend/globalconfig.h" +// The default configuration for snmalloc is used if alternative not defined +namespace snmalloc +{ + using Alloc = snmalloc::LocalAllocator; +} // namespace snmalloc +#endif + +// User facing API surface, needs to know what `Alloc` is. +#include "../snmalloc_front.h" + +#ifndef SNMALLOC_EXPORT +# define SNMALLOC_EXPORT +#endif +#ifdef SNMALLOC_STATIC_LIBRARY_PREFIX +# define __SN_CONCAT(a, b) a##b +# define __SN_EVALUATE(a, b) __SN_CONCAT(a, b) +# define SNMALLOC_NAME_MANGLE(a) \ + __SN_EVALUATE(SNMALLOC_STATIC_LIBRARY_PREFIX, a) +#elif !defined(SNMALLOC_NAME_MANGLE) +# define SNMALLOC_NAME_MANGLE(a) a +#endif diff --git a/src/pal/pal_posix.h b/src/pal/pal_posix.h index b087c92..93b9518 100644 --- a/src/pal/pal_posix.h +++ b/src/pal/pal_posix.h @@ -134,7 +134,7 @@ namespace snmalloc static void print_stack_trace() { -#ifdef BACKTRACE_HEADER +#ifdef SNMALLOC_BACKTRACE_HEADER constexpr int SIZE = 1024; void* buffer[SIZE]; auto nptrs = backtrace(buffer, SIZE); diff --git a/src/test/func/memcpy/func-memcpy.cc b/src/test/func/memcpy/func-memcpy.cc new file mode 100644 index 0000000..1727d38 --- /dev/null +++ b/src/test/func/memcpy/func-memcpy.cc @@ -0,0 +1,155 @@ +// Windows doesn't like changing the linkage spec of abort. +#if defined(_MSC_VER) +int main() +{ + return 0; +} +#else +// QEMU user mode does not support the code that generates backtraces and so we +// also need to skip this test if we are doing a debug build and targeting +// QEMU. +# if defined(SNMALLOC_QEMU_WORKAROUND) && defined(SNMALLOC_BACKTRACE_HEADER) +# undef SNMALLOC_BACKTRACE_HEADER +# endif +# ifdef SNMALLOC_STATIC_LIBRARY_PREFIX +# undef SNMALLOC_STATIC_LIBRARY_PREFIX +# endif +# ifdef SNMALLOC_FAIL_FAST +# undef SNMALLOC_FAIL_FAST +# endif +# define SNMALLOC_FAIL_FAST false +# define SNMALLOC_STATIC_LIBRARY_PREFIX my_ +# include "ds/defines.h" +# ifndef SNMALLOC_PASS_THROUGH +# include "override/malloc.cc" +# else +# define my_malloc(x) malloc(x) +# define my_free(x) free(x) +# endif +# include "override/memcpy.cc" + +# include +# include +# include +# include +# include + +/** + * Jump buffer used to jump out of `abort()` for recoverable errors. + */ +static std::jmp_buf jmp; + +/** + * Flag indicating whether `jmp` is valid. If this is set then calls to + * `abort` will jump to the jump buffer, rather than exiting. + */ +static bool can_longjmp; + +/** + * Replacement for the C standard `abort` that returns to the `setjmp` call for + * recoverable errors. + */ +extern "C" void abort() +{ + if (can_longjmp) + { + longjmp(jmp, 1); + } + exit(-1); +} + +/** + * Check that memcpy works in correct use. This allocates a pair of buffers, + * fills one with a well-known pattern, and then copies subsets of this at + * one-byte increments to a target. This gives us unaligned starts. + */ +void check_size(size_t size) +{ + auto* s = reinterpret_cast(my_malloc(size + 1)); + auto* d = reinterpret_cast(my_malloc(size + 1)); + d[size] = 0; + s[size] = 255; + for (size_t start = 0; start < size; start++) + { + unsigned char* src = s + start; + unsigned char* dst = d + start; + size_t sz = (size - start); + for (size_t i = 0; i < sz; ++i) + { + src[i] = static_cast(i); + } + for (size_t i = 0; i < sz; ++i) + { + dst[i] = 0; + } + my_memcpy(dst, src, sz); + for (size_t i = 0; i < sz; ++i) + { + if (dst[i] != static_cast(i)) + { + fprintf( + stderr, + "Testing size %zd %hhx == %hhx\n", + sz, + static_cast(i), + dst[i]); + } + SNMALLOC_CHECK(dst[i] == (unsigned char)i); + } + SNMALLOC_CHECK(d[size] == 0); + } + my_free(s); + my_free(d); +} + +void check_bounds(size_t size, size_t out_of_bounds) +{ + auto* s = reinterpret_cast(my_malloc(size)); + auto* d = reinterpret_cast(my_malloc(size)); + for (size_t i = 0; i < size; ++i) + { + s[i] = static_cast(i); + } + for (size_t i = 0; i < size; ++i) + { + d[i] = 0; + } + bool bounds_error = false; + can_longjmp = true; + if (setjmp(jmp) == 0) + { + my_memcpy(d, s, size + out_of_bounds); + } + else + { + bounds_error = true; + } + can_longjmp = false; + SNMALLOC_CHECK(bounds_error == (out_of_bounds > 0)); + my_free(s); + my_free(d); +} + +int main() +{ + // Skip the checks that expect bounds checks to fail when we are not the + // malloc implementation. +# if !defined(SNMALLOC_PASS_THROUGH) + // Some sizes to check for out-of-bounds access + std::initializer_list sizes = {16, 1024, 2 * 1024 * 1024}; + for (auto sz : sizes) + { + // Check in bounds + check_bounds(sz, 0); + // Check one byte out + check_bounds(sz, 1); + // Check one object out of bounds + check_bounds(sz, sz); + } +# endif + for (size_t x = 0; x < 2048; x++) + { + check_size(x); + } +} +#endif