fix: Run through clang-analyzer and fail on warnings (#277)

* correct some issues found by clang-analyzer (dead stores, one of them a bug in the page allocator)
* fix other warnings on clang
* turn of warnings-as-errors on CI for clang and msvc

CMakeLists.txt

@@ -218,6 +218,16 @@ if(MSVC)
 		set(CMAKE_ASM_MASM_FLAGS "${CMAKE_ASM_MASM_FLAGS} /safeseh")
 	endif()
 
+	# using `/Wall` is not feasible, as it spews tons of warnings from windows headers
+	target_compile_options(sentry PRIVATE $<BUILD_INTERFACE:/W4>)
+	# ignore all warnings for mpack
+	set_source_files_properties(
+		"${PROJECT_SOURCE_DIR}/vendor/mpack.c"
+		PROPERTIES
+		COMPILE_FLAGS
+		"/W0"
+	)
+
 	# set static runtime if enabled
 	if(SENTRY_BUILD_RUNTIMESTATIC)
 		set_property(TARGET sentry PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")

azure-pipelines.yml

@@ -29,8 +29,12 @@ stages:
               vmImage: "ubuntu-latest"
               CC: clang-9
               CXX: clang++-9
+              ERROR_ON_WARNINGS: 1
+              # TODO: install `clang-tools`
+              #SCAN_BUILD: 1
             macOS:
               vmImage: "macOs-latest"
+              ERROR_ON_WARNINGS: 1
             Windows (VS2017, 32bit):
               vmImage: "vs2017-win2016"
               TEST_X86: 1
@@ -51,7 +55,7 @@ stages:
             Android (API 29 NDK 21):
               vmImage: "macOs-latest"
               ANDROID_API: 29
-              ANDROID_NDK: 21.0.6113669
+              ANDROID_NDK: 21.2.6472646
         pool:
           vmImage: $(vmImage)
         steps:

examples/example.c

@@ -33,6 +33,16 @@ has_arg(int argc, char **argv, const char *arg)
     return false;
 }
 
+static void *invalid_mem = (void *)1;
+
+static void
+trigger_crash()
+{
+    // Triggers a segfault by writing to `NULL`. We actually do a `1 - 1` to
+    // defeat static analyzers which would warn for the trivial case.
+    memset((char *)invalid_mem - 1, 1, 100);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -131,7 +141,7 @@ main(int argc, char **argv)
     }
 
     if (has_arg(argc, argv, "crash")) {
-        memset((char *)0x0, 1, 100);
+        trigger_crash();
     }
 
     if (has_arg(argc, argv, "capture-event")) {

src/backends/sentry_backend_crashpad.cpp

@@ -13,21 +13,26 @@ extern "C" {
 #include <map>
 #include <vector>
 
-#ifdef __GNUC__
+#if defined(__GNUC__)
 #    pragma GCC diagnostic push
 #    pragma GCC diagnostic ignored "-Wunused-parameter"
 #    pragma GCC diagnostic ignored "-Wgnu-zero-variadic-macro-arguments"
 #    pragma GCC diagnostic ignored "-Wfour-char-constants"
 #    pragma GCC diagnostic ignored "-Wgnu-include-next"
+#elif defined(_MSC_VER)
+#    pragma warning(push)
+#    pragma warning(disable : 4100) // unreferenced formal parameter
 #endif
 
 #include "client/crash_report_database.h"
 #include "client/crashpad_client.h"
 #include "client/crashpad_info.h"
 #include "client/settings.h"
 
-#ifdef __GNUC__
+#if defined(__GNUC__)
 #    pragma GCC diagnostic pop
+#elif defined(_MSC_VER)
+#    pragma warning(pop)
 #endif
 
 extern "C" {

src/modulefinder/sentry_modulefinder_linux.c

@@ -310,8 +310,8 @@ sentry__procmaps_module_to_value(const sentry_module_t *module)
 {
     sentry_value_t mod_val = sentry_value_new_object();
     sentry_value_set_by_key(mod_val, "type", sentry_value_new_string("elf"));
-    sentry_value_set_by_key(
-        mod_val, "image_addr", sentry__value_new_addr((uint64_t)module->start));
+    sentry_value_set_by_key(mod_val, "image_addr",
+        sentry__value_new_addr((uint64_t)(size_t)module->start));
     sentry_value_set_by_key(mod_val, "image_size",
         sentry_value_new_int32(
             (int32_t)((size_t)module->end - (size_t)module->start)));

src/path/sentry_path_windows.c

@@ -340,7 +340,6 @@ sentry__path_remove(const sentry_path_t *path)
         }
         return 1;
     }
-    return 1;
 }
 
 int

src/sentry_core.h

@@ -14,8 +14,10 @@
 #    define MUST_USE
 #endif
 
-#ifdef __GNUC__
+#if defined(__GNUC__)
 #    define UNUSED(x) UNUSED_##x __attribute__((__unused__))
+#elif defined(_MSC_VER)
+#    define UNUSED(x) UNUSED_##x __pragma(warning(suppress : 4100))
 #else
 #    define UNUSED(x) UNUSED_##x
 #endif

src/sentry_database.c

@@ -156,8 +156,11 @@ sentry__process_old_runs(const sentry_options_t *options)
         }
 
         sentry_path_t *lockfile = sentry__path_append_str(run_dir, ".lock");
-        sentry_filelock_t *lock;
-        if (!lockfile || !(lock = sentry__filelock_new(lockfile))) {
+        if (!lockfile) {
+            continue;
+        }
+        sentry_filelock_t *lock = sentry__filelock_new(lockfile);
+        if (!lock) {
             continue;
         }
         bool did_lock = sentry__filelock_try_lock(lock);

src/sentry_json.c

@@ -360,7 +360,7 @@ decode_string_inplace(char *buf)
             input += 4;
 
             if (sentry__is_lead_surrogate(uchar)) {
-                uint16_t lead = uchar;
+                uint16_t lead = (uint16_t)uchar;
                 if (input[0] != '\\' || input[1] != 'u') {
                     return false;
                 }

src/sentry_logger.c

@@ -1,9 +1,9 @@
-#include <stdio.h>
-#include <string.h>
-
 #include "sentry_logger.h"
 #include "sentry_options.h"
 
+#include <stdio.h>
+#include <string.h>
+
 #if defined(SENTRY_PLATFORM_ANDROID)
 
 #    include <android/log.h>
@@ -43,11 +43,19 @@ sentry__logger_defaultlogger(
     const char *prefix = "[sentry] ";
     const char *priority = sentry__logger_describe(level);
 
-    char *format = sentry_malloc(
-        strlen(prefix) + strlen(priority) + strlen(message) + 1);
-    strcpy(format, prefix);
-    strcat(format, priority);
-    strcat(format, message);
+    size_t len = strlen(prefix) + strlen(priority) + strlen(message) + 1;
+    char *format = sentry_malloc(len);
+
+    // Some compilers/tools, such as MSVC warn for using `strcpy` and friends.
+    // However, there are not really any good alternatives:
+    // * `strcpy_s` is only available on Windows
+    // * `strlcpy` is apparently a BSD thing
+    // * `strscpy` is a linux kernel thing
+    // * `strncpy` is not really a good choice either, but tools do not mark it
+    //   as "unsafe".
+    strncpy(format, prefix, strlen(prefix));
+    strncat(format, priority, strlen(priority));
+    strncat(format, message, strlen(message));
 
     vfprintf(stderr, format, args);
 

src/sentry_logger.h

@@ -1,4 +1,4 @@
-#include "sentry.h"
+#include "sentry_boot.h"
 
 void sentry__logger_defaultlogger(
     sentry_level_t level, const char *message, va_list args);

src/sentry_scope.c

@@ -175,13 +175,13 @@ sentry__symbolize_frame(const sentry_frame_info_t *info, void *data)
         && sentry_value_is_null(
             sentry_value_get_by_key(frame, "symbol_addr"))) {
         sentry_value_set_by_key(frame, "symbol_addr",
-            sentry__value_new_addr((uint64_t)info->symbol_addr));
+            sentry__value_new_addr((uint64_t)(size_t)info->symbol_addr));
     }
 
     if (info->load_addr
         && sentry_value_is_null(sentry_value_get_by_key(frame, "image_addr"))) {
         sentry_value_set_by_key(frame, "image_addr",
-            sentry__value_new_addr((uint64_t)info->load_addr));
+            sentry__value_new_addr((uint64_t)(size_t)info->load_addr));
     }
 }
 
@@ -204,8 +204,8 @@ sentry__symbolize_stacktrace(sentry_value_t stacktrace)
         }
 
         // The addr is saved as a hex-number inside the value.
-        uint64_t addr
-            = (uint64_t)strtoll(sentry_value_as_string(addr_value), NULL, 0);
+        size_t addr
+            = (size_t)strtoll(sentry_value_as_string(addr_value), NULL, 0);
         if (!addr) {
             continue;
         }

src/sentry_string.c

@@ -152,8 +152,8 @@ sentry__string_to_wstr(const char *s)
 size_t
 sentry__unichar_to_utf8(uint32_t c, char *buf)
 {
-    size_t i, len;
-    int first;
+    size_t i, len = 1;
+    uint32_t first;
 
     if (c < 0x80) {
         first = 0;
@@ -172,9 +172,9 @@ sentry__unichar_to_utf8(uint32_t c, char *buf)
     }
 
     for (i = len - 1; i > 0; --i) {
-        buf[i] = (c & 0x3f) | 0x80;
+        buf[i] = (char)(c & 0x3f) | 0x80;
         c >>= 6;
     }
-    buf[0] = c | first;
+    buf[0] = (char)(c | first);
     return len;
 }

src/sentry_sync.c

@@ -64,7 +64,12 @@ shutdown_task(void *data)
 #    define UNSIGNED_MINGW
 #endif
 
-static UNSIGNED_MINGW int THREAD_FUNCTION_API
+// pthreads use `void *` return types, whereas windows uses `DWORD`
+#ifdef SENTRY_PLATFORM_WINDOWS
+static UNSIGNED_MINGW DWORD THREAD_FUNCTION_API
+#else
+static void *
+#endif
 worker_thread(void *data)
 {
     sentry_bgworker_t *bgw = data;

src/sentry_sync.h

@@ -2,6 +2,7 @@
 #define SENTRY_SYNC_H_INCLUDED
 
 #include "sentry_boot.h"
+#include "sentry_core.h"
 
 #include <assert.h>
 #include <stdio.h>
@@ -121,7 +122,8 @@ struct sentry__winmutex_s {
 };
 
 static inline BOOL CALLBACK
-sentry__winmutex_init(PINIT_ONCE InitOnce, PVOID cs, PVOID *lpContext)
+sentry__winmutex_init(
+    PINIT_ONCE UNUSED(InitOnce), PVOID cs, PVOID *UNUSED(lpContext))
 {
     InitializeCriticalSection(cs);
     return TRUE;
@@ -226,9 +228,7 @@ typedef pthread_cond_t sentry_cond_t;
         } while (0)
 #    define sentry__cond_wake pthread_cond_signal
 #    define sentry__thread_spawn(ThreadId, Func, Data)                         \
-        (pthread_create(ThreadId, NULL, (void *(*)(void *))Func, Data) == 0    \
-                ? 0                                                            \
-                : 1)
+        (pthread_create(ThreadId, NULL, Func, Data) == 0 ? 0 : 1)
 #    define sentry__thread_join(ThreadId) pthread_join(ThreadId, NULL)
 #    define sentry__threadid_equal pthread_equal
 #    define sentry__current_thread pthread_self
@@ -254,8 +254,8 @@ sentry__cond_wait_timeout(
         *(Mutex) = tmp;                                                        \
     } while (0)
 
-static inline int
-sentry__atomic_fetch_and_add(volatile int *val, int diff)
+static inline long
+sentry__atomic_fetch_and_add(volatile long *val, long diff)
 {
 #ifdef SENTRY_PLATFORM_WINDOWS
     return InterlockedExchangeAdd(val, diff);
@@ -264,8 +264,8 @@ sentry__atomic_fetch_and_add(volatile int *val, int diff)
 #endif
 }
 
-static inline int
-sentry__atomic_fetch(volatile int *val)
+static inline long
+sentry__atomic_fetch(volatile long *val)
 {
     return sentry__atomic_fetch_and_add(val, 0);
 }

src/sentry_unix_pageallocator.c

@@ -10,6 +10,8 @@
 #    define MAP_ANONYMOUS MAP_ANON
 #endif
 
+#define ALIGN 8
+
 struct page_header;
 struct page_header {
     struct page_header *next;
@@ -81,10 +83,15 @@ sentry__page_allocator_alloc(size_t size)
         return NULL;
     }
 
+    // make sure the requested size is correctly aligned
+    size_t diff = size % ALIGN;
+    size = size + ALIGN - diff;
+
     char *rv = NULL;
 
     sentry__spinlock_lock(&g_lock);
 
+    // current page is large enough
     if (g_alloc->current_page
         && g_alloc->page_size - g_alloc->page_offset >= size) {
         rv = g_alloc->current_page + g_alloc->page_offset;
@@ -93,21 +100,25 @@ sentry__page_allocator_alloc(size_t size)
             g_alloc->page_offset = 0;
             g_alloc->current_page = NULL;
         }
-    }
-
-    size_t pages = (size + sizeof(struct page_header) + g_alloc->page_size - 1)
-        / g_alloc->page_size;
-    rv = get_pages(pages);
-
-    if (rv) {
-        g_alloc->page_offset = (g_alloc->page_size
-                                   - (g_alloc->page_size * pages
-                                       - (size + sizeof(struct page_header))))
-            % g_alloc->page_size;
-        g_alloc->current_page = g_alloc->page_offset
-            ? rv + g_alloc->page_size * (pages - 1)
-            : NULL;
-        rv += sizeof(struct page_header);
+    } else {
+        // allocate new pages
+
+        size_t requested_size = size + sizeof(struct page_header);
+        size_t pages
+            = (requested_size + g_alloc->page_size - 1) / g_alloc->page_size;
+        size_t actual_size = g_alloc->page_size * pages;
+
+        rv = get_pages(pages);
+
+        if (rv) {
+            size_t diff = actual_size - requested_size;
+            g_alloc->page_offset
+                = (g_alloc->page_size - diff) % g_alloc->page_size;
+            g_alloc->current_page = g_alloc->page_offset
+                ? rv + g_alloc->page_size * (pages - 1)
+                : NULL;
+            rv += sizeof(struct page_header);
+        }
     }
 
     sentry__spinlock_unlock(&g_lock);

src/sentry_utils.c

@@ -161,7 +161,6 @@ sentry__url_parse(sentry_url_t *url_out, const char *url)
         tmp = ptr;
         SKIP_WHILE_NOT(tmp, 0);
         url_out->fragment = sentry__string_clonen(ptr, tmp - ptr);
-        ptr = tmp;
     }
 
     if (url_out->port == 0) {

src/sentry_uuid.c

@@ -33,7 +33,7 @@ sentry_uuid_from_string(const char *str)
     size_t len = strlen(str);
     size_t pos = 0;
     bool is_nibble = true;
-    char nibble;
+    char nibble = 0;
 
     for (i = 0; i < len && pos < 16; i++) {
         char c = str[i];