Enable guard pages in CHECK_CLIENT

Change the behaviour to use PROT_NONE for reservations in CHECK_CLIENT
mode.  This means that we only provide access once data is actually
being used.

CMakeLists.txt

@@ -50,13 +50,6 @@ int main() {
 }
 " SNMALLOC_PLATFORM_HAS_GETENTROPY)
 
-if (NOT SNMALLOC_CI_BUILD)
-  option(USE_POSIX_COMMIT_CHECKS "Instrument Posix PAL to check for access to unused blocks of memory." Off)
-else ()
-  # This is enabled in every bit of CI to detect errors.
-  option(USE_POSIX_COMMIT_CHECKS "Instrument Posix PAL to check for access to unused blocks of memory." On)
-endif ()
-
 # Provide as macro so other projects can reuse
 macro(warnings_high)
   if(MSVC)
@@ -169,10 +162,6 @@ if(SNMALLOC_CI_BUILD)
   target_compile_definitions(snmalloc_lib INTERFACE -DSNMALLOC_CI_BUILD)
 endif()
 
-if(USE_POSIX_COMMIT_CHECKS)
-  target_compile_definitions(snmalloc_lib INTERFACE -DUSE_POSIX_COMMIT_CHECKS)
-endif()
-
 if(SNMALLOC_PLATFORM_HAS_GETENTROPY)
   target_compile_definitions(snmalloc_lib INTERFACE -DSNMALLOC_PLATFORM_HAS_GETENTROPY)
 endif()
@@ -277,13 +266,13 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY)
     set(SHARED_FILES src/override/new.cc)
     add_shim(snmallocshim SHARED ${SHARED_FILES})
     add_shim(snmallocshim-checks SHARED ${SHARED_FILES})
-    target_compile_definitions(snmallocshim-checks PRIVATE CHECK_CLIENT)
+    target_compile_definitions(snmallocshim-checks PRIVATE SNMALLOC_CHECK_CLIENT)
   endif()
 
   if(SNMALLOC_RUST_SUPPORT)
     add_shim(snmallocshim-rust STATIC src/override/rust.cc)
     add_shim(snmallocshim-checks-rust STATIC src/override/rust.cc)
-    target_compile_definitions(snmallocshim-checks-rust PRIVATE CHECK_CLIENT)
+    target_compile_definitions(snmallocshim-checks-rust PRIVATE SNMALLOC_CHECK_CLIENT)
   endif()
 
   enable_testing()
@@ -313,14 +302,11 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY)
 
         add_executable(${TESTNAME} ${SRC})
 
-        # For all tests enable commit checking.
-        target_compile_definitions(${TESTNAME} PRIVATE -DUSE_POSIX_COMMIT_CHECKS)
-
         if (${FLAVOUR} STREQUAL "malloc")
           target_compile_definitions(${TESTNAME} PRIVATE SNMALLOC_PASS_THROUGH)
         endif()
         if (${FLAVOUR} STREQUAL "check")
-          target_compile_definitions(${TESTNAME} PRIVATE CHECK_CLIENT)
+          target_compile_definitions(${TESTNAME} PRIVATE SNMALLOC_CHECK_CLIENT)
         endif()
         target_link_libraries(${TESTNAME} snmalloc_lib)
         if (${TEST} MATCHES "release-.*")

src/ds/defines.h

@@ -114,12 +114,6 @@ namespace snmalloc
 #  endif
 #endif
 
-// // The CHECK_CLIENT macro is used to turn on minimal checking of the client
-// // calling the API correctly.
-// #if !defined(NDEBUG) && !defined(CHECK_CLIENT)
-// #  define CHECK_CLIENT
-// #endif
-
 inline SNMALLOC_FAST_PATH void check_client_error(const char* const str)
 {
   //[[clang::musttail]]
@@ -132,7 +126,7 @@ check_client_impl(bool test, const char* const str)
   if (unlikely(!test))
     check_client_error(str);
 }
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
 #  define check_client(test, str) check_client_impl(test, str)
 #else
 #  define check_client(test, str)

src/ds/helpers.h

@@ -73,7 +73,7 @@ namespace snmalloc
     }
   };
 
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
   template<size_t length, typename T>
   class ModArray
   {

src/mem/allocconfig.h

@@ -80,7 +80,7 @@ namespace snmalloc
   static constexpr size_t MIN_CHUNK_SIZE = bits::one_at_bit(MIN_CHUNK_BITS);
 
   // Minimum number of objects on a slab
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
   static constexpr size_t MIN_OBJECT_COUNT = 13;
 #else
   static constexpr size_t MIN_OBJECT_COUNT = 4;

src/mem/corealloc.h

@@ -146,7 +146,7 @@ namespace snmalloc
       FreeListBuilder<false> b;
       SNMALLOC_ASSERT(b.empty());
 
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
       // Structure to represent the temporary list elements
       struct PreAllocObject
       {
@@ -214,7 +214,7 @@ namespace snmalloc
       meta->free_queue.close(fl, key);
       void* p = finish_alloc_no_zero(fl.take(key), sizeclass);
 
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
       // Check free list is well-formed on platforms with
       // integers as pointers.
       size_t count = 1; // Already taken one above.

src/mem/freelist.h

@@ -90,7 +90,7 @@ namespace snmalloc
       // TODO: Should really use C++20 atomic_ref rather than a union.
       AtomicCapPtr<FreeObject, CBAlloc> atomic_next_object;
     };
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
     // Encoded representation of a back pointer.
     // Hard to fake, and provides consistency on
     // the next pointers.
@@ -104,7 +104,7 @@ namespace snmalloc
     }
 
     /**
-     * Assign next_object and update its prev_encoded if CHECK_CLIENT.
+     * Assign next_object and update its prev_encoded if SNMALLOC_CHECK_CLIENT.
      * Static so that it can be used on reference to a FreeObject.
      *
      * Returns a pointer to the next_object field of the next parameter as an
@@ -116,7 +116,7 @@ namespace snmalloc
       CapPtr<FreeObject, CBAlloc> next,
       FreeListKey& key)
     {
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
       next->prev_encoded =
         signed_prev(address_cast(curr), address_cast(next), key);
 #else
@@ -127,13 +127,13 @@ namespace snmalloc
     }
 
     /**
-     * Assign next_object and update its prev_encoded if CHECK_CLIENT
+     * Assign next_object and update its prev_encoded if SNMALLOC_CHECK_CLIENT
      *
      * Uses the atomic view of next, so can be used in the message queues.
      */
     void atomic_store_next(CapPtr<FreeObject, CBAlloc> next, FreeListKey& key)
     {
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
       next->prev_encoded =
         signed_prev(address_cast(this), address_cast(next), key);
 #else
@@ -152,7 +152,7 @@ namespace snmalloc
     CapPtr<FreeObject, CBAlloc> atomic_read_next(FreeListKey& key)
     {
       auto n = atomic_next_object.load(std::memory_order_acquire);
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
       if (n != nullptr)
       {
         n->check_prev(signed_prev(address_cast(this), address_cast(n), key));
@@ -194,7 +194,7 @@ namespace snmalloc
   class FreeListIter
   {
     CapPtr<FreeObject, CBAlloc> curr{nullptr};
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
     address_t prev{0};
 #endif
 
@@ -203,7 +203,7 @@ namespace snmalloc
       CapPtr<FreeObject, CBAlloc> head, address_t prev_value)
     : curr(head)
     {
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
       prev = prev_value;
 #endif
       UNUSED(prev_value);
@@ -237,7 +237,7 @@ namespace snmalloc
 
       Aal::prefetch(next.unsafe_ptr());
       curr = next;
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
       c->check_prev(prev);
       prev = signed_prev(address_cast(c), address_cast(next), key);
 #else
@@ -251,7 +251,7 @@ namespace snmalloc
   /**
    * Used to build a free list in object space.
    *
-   * Adds signing of pointers in the CHECK_CLIENT mode
+   * Adds signing of pointers in the SNMALLOC_CHECK_CLIENT mode
    *
    * We use the template parameter, so that an enclosing
    * class can make use of the remaining bytes, which may not

src/mem/metaslab.h

@@ -30,7 +30,7 @@ namespace snmalloc
      *
      *  Spare 32bits are used for the fields in MetaslabEnd.
      */
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
     FreeListBuilder<true> free_queue;
 #else
     FreeListBuilder<false> free_queue;
@@ -149,7 +149,7 @@ namespace snmalloc
       auto p = tmp_fl.take(key);
       fast_free_list = tmp_fl;
 
-#ifdef CHECK_CLIENT
+#ifdef SNMALLOC_CHECK_CLIENT
       entropy.refresh_bits();
 #else
       UNUSED(entropy);

src/mem/sizeclasstable.h

@@ -166,7 +166,7 @@ namespace snmalloc
    */
   inline uint16_t threshold_for_waking_slab(sizeclass_t sizeclass)
   {
-    // #ifdef CHECK_CLIENT
+    // #ifdef SNMALLOC_CHECK_CLIENT
     return sizeclass_metadata.waking[sizeclass];
     // #else
     //     UNUSED(sizeclass);

src/pal/pal_apple.h

@@ -113,7 +113,7 @@ namespace snmalloc
     {
       SNMALLOC_ASSERT(is_aligned_block<page_size>(p, size));
 
-#  ifdef USE_POSIX_COMMIT_CHECKS
+#  if defined(SNMALLOC_CHECK_CLIENT) && !defined(NDEBUG)
       memset(p, 0x5a, size);
 #  endif
 
@@ -126,7 +126,7 @@ namespace snmalloc
       while (madvise(p, size, MADV_FREE_REUSABLE) == -1 && errno == EAGAIN)
         ;
 
-#  ifdef USE_POSIX_COMMIT_CHECKS
+#  ifdef SNMALLOC_CHECK_CLIENT
       // This must occur after `MADV_FREE_REUSABLE`.
       //
       // `mach_vm_protect` is observably slower in benchmarks.
@@ -179,7 +179,7 @@ namespace snmalloc
         }
       }
 
-#  ifdef USE_POSIX_COMMIT_CHECKS
+#  ifdef SNMALLOC_CHECK_CLIENT
       // Mark pages as writable for `madvise` below.
       //
       // `mach_vm_protect` is observably slower in benchmarks.
@@ -218,7 +218,7 @@ namespace snmalloc
       // must be initialized to 0 or addr is interepreted as a lower-bound.
       mach_vm_address_t addr = 0;
 
-#  ifdef USE_POSIX_COMMIT_CHECKS
+#  ifdef SNMALLOC_CHECK_CLIENT
       vm_prot_t prot = committed ? VM_PROT_READ | VM_PROT_WRITE : VM_PROT_NONE;
 #  else
       vm_prot_t prot = VM_PROT_READ | VM_PROT_WRITE;

src/pal/pal_bsd.h

@@ -35,11 +35,11 @@ namespace snmalloc
     {
       SNMALLOC_ASSERT(is_aligned_block<OS::page_size>(p, size));
       // Call this Pal to simulate the Windows decommit in CI.
-#ifdef USE_POSIX_COMMIT_CHECKS
+#if defined(SNMALLOC_CHECK_CLIENT) && !defined(NDEBUG)
       memset(p, 0x5a, size);
 #endif
       madvise(p, size, MADV_FREE);
-#ifdef USE_POSIX_COMMIT_CHECKS
+#ifdef SNMALLOC_CHECK_CLIENT
       mprotect(p, size, PROT_NONE);
 #endif
     }

src/pal/pal_bsd_aligned.h

@@ -37,10 +37,16 @@ namespace snmalloc
 
       int log2align = static_cast<int>(bits::next_pow2_bits(size));
 
+#ifdef SNMALLOC_CHECK_CLIENT
+      auto prot = committed ? PROT_READ | PROT_WRITE : PROT_NONE;
+#else
+      auto prot = PROT_READ | PROT_WRITE;
+#endif
+
       void* p = mmap(
         nullptr,
         size,
-        PROT_READ | PROT_WRITE,
+        prot,
         MAP_PRIVATE | MAP_ANONYMOUS | MAP_ALIGNED(log2align),
         -1,
         0);

src/pal/pal_posix.h

@@ -165,10 +165,12 @@ namespace snmalloc
     static void notify_not_using(void* p, size_t size) noexcept
     {
       SNMALLOC_ASSERT(is_aligned_block<OS::page_size>(p, size));
-#ifdef USE_POSIX_COMMIT_CHECKS
+#ifdef SNMALLOC_CHECK_CLIENT
       // Fill memory so that when we switch the pages back on we don't make
       // assumptions on the content.
+#  if !defined(NDEBUG)
       memset(p, 0x5a, size);
+#  endif
       mprotect(p, size, PROT_NONE);
 #else
       UNUSED(p);
@@ -189,7 +191,7 @@ namespace snmalloc
       SNMALLOC_ASSERT(
         is_aligned_block<OS::page_size>(p, size) || (zero_mem == NoZero));
 
-#ifdef USE_POSIX_COMMIT_CHECKS
+#ifdef SNMALLOC_CHECK_CLIENT
       mprotect(p, size, PROT_READ | PROT_WRITE);
 #else
       UNUSED(p);
@@ -260,6 +262,12 @@ namespace snmalloc
       constexpr size_t min_size =
         bits::is64() ? bits::one_at_bit(31) : bits::one_at_bit(27);
 
+#ifdef SNMALLOC_CHECK_CLIENT
+      auto prot = PROT_NONE;
+#else
+      auto prot = PROT_READ | PROT_WRITE;
+#endif
+
       for (size_t size_request = bits::max(size, min_size);
            size_request >= size;
            size_request = size_request / 2)
@@ -267,7 +275,7 @@ namespace snmalloc
         void* p = mmap(
           nullptr,
           size_request,
-          PROT_READ | PROT_WRITE,
+          prot,
           MAP_PRIVATE | MAP_ANONYMOUS | DefaultMMAPFlags<OS>::flags,
           AnonFD<OS>::fd,
           0);