Merge pull request #557 from bratpiorka/rrudnick_sdl_flags

add more secure compilation flags

Author: lukaszstolarczuk

CMakeLists.txt

@@ -38,8 +38,7 @@ option(UMF_BUILD_BENCHMARKS_MT "Build UMF multithreaded benchmarks" OFF)
 option(UMF_BUILD_EXAMPLES "Build UMF examples" ON)
 option(UMF_BUILD_FUZZTESTS "Build UMF fuzz tests" OFF)
 option(UMF_BUILD_GPU_EXAMPLES "Build UMF GPU examples" OFF)
-option(UMF_DEVELOPER_MODE "Enable developer checks, treats warnings as errors"
-       OFF)
+option(UMF_DEVELOPER_MODE "Enable additional developer checks" OFF)
 option(UMF_FORMAT_CODE_STYLE
        "Add clang, cmake, and black -format-check and -format-apply targets"
        OFF)

README.md

@@ -118,7 +118,7 @@ List of options provided by CMake:
 | UMF_BUILD_EXAMPLES | Build UMF examples | ON/OFF | ON |
 | UMF_BUILD_FUZZTESTS | Build UMF fuzz tests | ON/OFF | OFF |
 | UMF_BUILD_GPU_EXAMPLES | Build UMF GPU examples | ON/OFF | OFF |
-| UMF_DEVELOPER_MODE | Treat warnings as errors and enables additional checks | ON/OFF | OFF |
+| UMF_DEVELOPER_MODE | Enable additional developer checks | ON/OFF | OFF |
 | UMF_FORMAT_CODE_STYLE | Add clang, cmake, and black -format-check and -format-apply targets to make | ON/OFF | OFF |
 | UMF_TESTS_FAIL_ON_SKIP | Treat skips in tests as fail | ON/OFF | OFF |
 | USE_ASAN | Enable AddressSanitizer checks | ON/OFF | OFF |

benchmark/ubench.c

@@ -7,6 +7,12 @@
  *
  */
 
+#include <stdbool.h>
+
+#ifndef _WIN32
+#include <unistd.h>
+#endif
+
 #include <umf/ipc.h>
 #include <umf/memory_pool.h>
 #include <umf/pools/pool_proxy.h>
@@ -22,19 +28,28 @@
 #include <umf/pools/pool_jemalloc.h>
 #endif
 
-#include <stdbool.h>
-
-#ifndef _WIN32
-#include <unistd.h>
-#endif
-
-#include "ubench.h"
 #include "utils_common.h"
 
 #if (defined UMF_BUILD_GPU_TESTS)
 #include "utils_level_zero.h"
 #endif
 
+// NOTE: with strict compilation flags, ubench compilation throws some
+// warnings. We disable them here because we do not want to change the ubench
+// code.
+
+// disable warning 6308:'realloc' might return null pointer: assigning null
+// pointer to 'failed_benchmarks', which is passed as an argument to 'realloc',
+// will cause the original memory block to be leaked.
+// disable warning 6001: Using uninitialized memory
+// '*ubench_state.benchmarks.name'.
+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable : 6308)
+#pragma warning(disable : 6001)
+#endif // _MSC_VER
+
+#include "ubench.h"
 // BENCHMARK CONFIG
 #define N_ITERATIONS 1000
 #define ALLOC_SIZE (util_get_page_size())
@@ -63,7 +78,7 @@ static void do_benchmark(alloc_t *array, size_t iters, malloc_t malloc_f,
     int i = 0;
     do {
         array[i].ptr = malloc_f(provider, Alloc_size, 0);
-    } while (array[i++].ptr != NULL && i < iters);
+    } while (array[i++].ptr != NULL && i < (int)iters);
 
     while (--i >= 0) {
         free_f(provider, array[i].ptr, Alloc_size);
@@ -110,14 +125,18 @@ UBENCH_EX(simple, glibc_malloc) {
 
 static umf_os_memory_provider_params_t UMF_OS_MEMORY_PROVIDER_PARAMS = {
     /* .protection = */ UMF_PROTECTION_READ | UMF_PROTECTION_WRITE,
-    /* .visibility */ UMF_MEM_MAP_PRIVATE,
+    /* .visibility = */ UMF_MEM_MAP_PRIVATE,
+    /* .shm_name = */ NULL,
 
     // NUMA config
-    /* .nodemask = */ NULL,
-    /* .maxnode = */ 0,
+    /* .numa_list = */ NULL,
+    /* .numa_list_len = */ 0,
+
     /* .numa_mode = */ UMF_NUMA_MODE_DEFAULT,
+    /* .part_size = */ 0,
 
-    // others
+    /* .partitions = */ NULL,
+    /* .partitions_len = */ 0,
 };
 
 static void *w_umfMemoryProviderAlloc(void *provider, size_t size,
@@ -487,3 +506,7 @@ UBENCH_EX(ipc, disjoint_pool_with_level_zero_provider) {
 #endif /* (defined UMF_BUILD_LIBUMF_POOL_DISJOINT && defined UMF_BUILD_LEVEL_ZERO_PROVIDER && defined UMF_BUILD_GPU_TESTS) */
 
 UBENCH_MAIN()
+
+#if defined(_MSC_VER)
+#pragma warning(pop)
+#endif // _MSC_VER

cmake/helpers.cmake

@@ -92,17 +92,20 @@ function(add_umf_target_compile_options name)
             ${name}
             PRIVATE -fPIC
                     -Wall
+                    -Wextra
+                    -Werror
                     -Wpedantic
                     -Wempty-body
                     -Wunused-parameter
+                    -Wformat
+                    -Wformat-security
                     $<$<CXX_COMPILER_ID:GNU>:-fdiagnostics-color=auto>)
         if(CMAKE_BUILD_TYPE STREQUAL "Release")
             target_compile_definitions(${name} PRIVATE -D_FORTIFY_SOURCE=2)
         endif()
         if(UMF_DEVELOPER_MODE)
-            target_compile_options(
-                ${name} PRIVATE -Werror -fno-omit-frame-pointer
-                                -fstack-protector-strong)
+            target_compile_options(${name} PRIVATE -fno-omit-frame-pointer
+                                                   -fstack-protector-strong)
         endif()
         if(USE_GCOV)
             if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
@@ -113,12 +116,26 @@ function(add_umf_target_compile_options name)
     elseif(MSVC)
         target_compile_options(
             ${name}
-            PRIVATE $<$<CXX_COMPILER_ID:MSVC>:/MP> # clang-cl.exe does not
-                                                   # support /MP
-                    /W3 /MD$<$<CONFIG:Debug>:d> /GS)
-
-        if(UMF_DEVELOPER_MODE)
-            target_compile_options(${name} PRIVATE /WX /GS)
+            PRIVATE /MD$<$<CONFIG:Debug>:d>
+                    $<$<CONFIG:Release>:/sdl>
+                    /analyze
+                    /DYNAMICBASE
+                    /W4
+                    /WX
+                    /Gy
+                    /GS
+                    # disable warning 6326: Potential comparison of a constant
+                    # with another constant
+                    /wd6326
+                    # disable 4200 warning: nonstandard extension used:
+                    # zero-sized array in struct/union
+                    /wd4200)
+        if(${CMAKE_C_COMPILER_ID} MATCHES "MSVC")
+            target_compile_options(
+                ${name}
+                PRIVATE # below flags are not recognized by Clang
+                        /MP $<$<CONFIG:Release>:/LTCG>
+                        $<$<CONFIG:Release>:/NXCOMPAT>)
         endif()
     endif()
 endfunction()

examples/basic/ipc_ipcapi_consumer.c

@@ -135,13 +135,14 @@ int main(int argc, char *argv[]) {
     memset(recv_buffer, 0, RECV_BUFF_SIZE);
 
     // receive a size of the IPC handle from the producer's
-    ssize_t len = recv(producer_socket, recv_buffer, RECV_BUFF_SIZE, 0);
-    if (len < 0) {
+    ssize_t recv_len = recv(producer_socket, recv_buffer, RECV_BUFF_SIZE, 0);
+    if (recv_len < 0) {
         fprintf(
             stderr,
             "[consumer] ERROR: receiving a size of the IPC handle failed\n");
         goto err_close_producer_socket;
     }
+    size_t len = (size_t)recv_len;
 
     size_t size_IPC_handle = *(size_t *)recv_buffer;
 
@@ -151,11 +152,13 @@ int main(int argc, char *argv[]) {
             len, size_IPC_handle);
 
     // send received size to the producer as a confirmation
-    len = send(producer_socket, &size_IPC_handle, sizeof(size_IPC_handle), 0);
-    if (len < 0) {
+    recv_len =
+        send(producer_socket, &size_IPC_handle, sizeof(size_IPC_handle), 0);
+    if (recv_len < 0) {
         fprintf(stderr, "[consumer] ERROR: sending confirmation failed\n");
         goto err_close_producer_socket;
     }
+    len = (size_t)recv_len;
 
     fprintf(stderr,
             "[consumer] Sent a confirmation to the producer (%zu bytes)\n",
@@ -169,11 +172,13 @@ int main(int argc, char *argv[]) {
     }
 
     // receive the IPC handle from the producer's
-    len = recv(producer_socket, IPC_handle, size_IPC_handle, 0);
-    if (len < 0) {
+    recv_len = recv(producer_socket, IPC_handle, size_IPC_handle, 0);
+    if (recv_len < 0) {
         fprintf(stderr, "[consumer] ERROR: receiving the IPC handle failed\n");
         goto err_free_IPC_handle;
     }
+    len = (size_t)recv_len;
+
     if (len < size_IPC_handle) {
         fprintf(stderr,
                 "[consumer] ERROR: receiving the IPC handle failed - received "

examples/basic/ipc_level_zero.c

@@ -20,10 +20,11 @@ int create_level_zero_pool(ze_context_handle_t context,
                            ze_device_handle_t device,
                            umf_memory_pool_handle_t *pool) {
     // setup params
-    level_zero_memory_provider_params_t params = {0};
+    level_zero_memory_provider_params_t params;
     params.level_zero_context_handle = context;
     params.level_zero_device_handle = device;
     params.memory_type = UMF_MEMORY_TYPE_DEVICE;
+
     // create Level Zero provider
     umf_memory_provider_handle_t provider = 0;
     umf_result_t umf_result = umfMemoryProviderCreate(

include/umf/proxy_lib_new_delete.h

@@ -48,6 +48,14 @@ SOFTWARE.
 #include <stdlib.h>
 #endif // _WIN32
 
+// disable warning 28251: "Inconsistent annotation for 'new': this instance
+// has no annotations." because different Win SDKs use slightly different
+// definitions of new
+#if defined(_MSC_VER)
+#pragma warning(push)
+#pragma warning(disable : 28251)
+#endif // _MSC_VER
+
 static inline void *internal_aligned_alloc(size_t alignment, size_t size) {
 #ifdef _WIN32
     return _aligned_malloc(size, alignment);
@@ -75,10 +83,18 @@ void operator delete(void *p, const std::nothrow_t &) noexcept { free(p); }
 void operator delete[](void *p, const std::nothrow_t &) noexcept { free(p); }
 
 decl_new(n) void *operator new(std::size_t n) noexcept(false) {
-    return malloc(n);
+    void *ptr = malloc(n);
+    if (ptr == nullptr) {
+        throw std::bad_alloc();
+    }
+    return ptr;
 }
 decl_new(n) void *operator new[](std::size_t n) noexcept(false) {
-    return malloc(n);
+    void *ptr = malloc(n);
+    if (ptr == nullptr) {
+        throw std::bad_alloc();
+    }
+    return ptr;
 }
 
 decl_new_nothrow(n) void *operator new(std::size_t n,
@@ -134,10 +150,18 @@ void operator delete[](void *p, std::align_val_t al,
 }
 
 void *operator new(std::size_t n, std::align_val_t al) noexcept(false) {
-    return internal_aligned_alloc(static_cast<size_t>(al), n);
+    void *ptr = internal_aligned_alloc(static_cast<size_t>(al), n);
+    if (ptr == nullptr) {
+        throw std::bad_alloc();
+    }
+    return ptr;
 }
 void *operator new[](std::size_t n, std::align_val_t al) noexcept(false) {
-    return internal_aligned_alloc(static_cast<size_t>(al), n);
+    void *ptr = internal_aligned_alloc(static_cast<size_t>(al), n);
+    if (ptr == nullptr) {
+        throw std::bad_alloc();
+    }
+    return ptr;
 }
 void *operator new(std::size_t n, std::align_val_t al,
                    const std::nothrow_t &) noexcept {
@@ -147,6 +171,11 @@ void *operator new[](std::size_t n, std::align_val_t al,
                      const std::nothrow_t &) noexcept {
     return internal_aligned_alloc(static_cast<size_t>(al), n);
 }
+
+#if defined(_MSC_VER)
+#pragma warning(pop)
+#endif // _MSC_VER
+
 #endif // (__cplusplus > 201402L || defined(__cpp_aligned_new))
 #endif // defined(__cplusplus)
 

src/base_alloc/base_alloc.c

@@ -219,6 +219,12 @@ void *umf_ba_alloc(umf_ba_pool_t *pool) {
     // we'll mark the memory as undefined
     utils_annotate_memory_defined(chunk, sizeof(*chunk));
 
+    // check if the free list is not empty
+    if (pool->metadata.free_list == NULL) {
+        LOG_ERR("base_alloc: Free list should not be empty before new alloc");
+        return NULL;
+    }
+
     pool->metadata.free_list = pool->metadata.free_list->next;
     pool->metadata.n_allocs++;
 #ifndef NDEBUG

src/base_alloc/base_alloc_linear.c

@@ -197,9 +197,9 @@ int umf_ba_linear_free(umf_ba_linear_pool_t *pool, void *ptr) {
         if ((pool->metadata.pool_n_allocs == 0) && pool->next_pool &&
             (pool->metadata.pool_size > page_size)) {
             // we can free the first (main) pool except of the first page containing the metadata
-            void *ptr = (char *)pool + page_size;
+            void *pool_ptr = (char *)pool + page_size;
             size_t size = pool->metadata.pool_size - page_size;
-            ba_os_free(ptr, size);
+            ba_os_free(pool_ptr, size);
             // update pool_size
             pool->metadata.pool_size = page_size;
         }
@@ -222,9 +222,9 @@ int umf_ba_linear_free(umf_ba_linear_pool_t *pool, void *ptr) {
                 assert(prev_pool->next_pool == next_pool);
                 prev_pool->next_pool = next_pool->next_pool;
                 _DEBUG_EXECUTE(pool->metadata.n_pools--);
-                void *ptr = next_pool;
+                void *next_pool_ptr = next_pool;
                 size_t size = next_pool->pool_size;
-                ba_os_free(ptr, size);
+                ba_os_free(next_pool_ptr, size);
             }
             _DEBUG_EXECUTE(ba_debug_checks(pool));
             util_mutex_unlock(&pool->metadata.lock);

src/base_alloc/base_alloc_windows.c

@@ -16,7 +16,10 @@ void *ba_os_alloc(size_t size) {
     return VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE);
 }
 
-void ba_os_free(void *ptr, size_t size) { VirtualFree(ptr, 0, MEM_RELEASE); }
+void ba_os_free(void *ptr, size_t size) {
+    (void)size; // unused
+    VirtualFree(ptr, 0, MEM_RELEASE);
+}
 
 static void _ba_os_init_page_size(void) {
     SYSTEM_INFO SystemInfo;