ref(on_crash): follow-up to make on_crash releasable

.github/workflows/ci.yml

@@ -136,7 +136,7 @@ jobs:
 
       - name: Expose llvm PATH for Mac
         if: ${{ runner.os == 'macOS' }}
-        run: echo $(brew --prefix llvm@13)/bin >> $GITHUB_PATH
+        run: echo $(brew --prefix llvm@14)/bin >> $GITHUB_PATH
 
       - name: Installing Android SDK Dependencies
         if: ${{ env['ANDROID_API'] }}

CHANGELOG.md

@@ -2,12 +2,26 @@
 
 ## Unreleased
 
+**Features**
+
+- Provide `on_crash()` callback to allow clients to act on detected crashes
+  ([#724](https://github.com/getsentry/sentry-native/pull/724), 
+   [#734](https://github.com/getsentry/sentry-native/pull/734))
+
 **Fixes**
 
+- Make Windows ModuleFinder more resilient to missing Debug Info 
+  ([#732](https://github.com/getsentry/sentry-native/pull/732))
 - Aligned pre-send event processing in `sentry_capture_event()` with the 
   [cross-SDK session filter order](https://develop.sentry.dev/sdk/sessions/#filter-order) 
   ([#729](https://github.com/getsentry/sentry-native/pull/729))
 
+**Thank you**:
+
+Features, fixes and improvements in this release have been contributed by:
+
+- [@espkk](https://github.com/espkk)
+
 ## 0.4.18
 
 **Features**:

examples/example.c

@@ -23,6 +23,51 @@
 #    define sleep_s(SECONDS) sleep(SECONDS)
 #endif
 
+static sentry_value_t
+before_send_callback(sentry_value_t event, void *hint, void *closure)
+{
+    (void)hint;
+    (void)closure;
+
+    // make our mark on the event
+    sentry_value_set_by_key(
+        event, "adapted_by", sentry_value_new_string("before_send"));
+
+    // tell the backend to proceed with the event
+    return event;
+}
+
+static sentry_value_t
+discarding_before_send_callback(sentry_value_t event, void *hint, void *closure)
+{
+    (void)hint;
+    (void)closure;
+
+    // discard event and signal backend to stop further processing
+    sentry_value_decref(event);
+    return sentry_value_new_null();
+}
+
+static int
+discarding_on_crash_callback(const sentry_ucontext_t *uctx, void *closure)
+{
+    (void)uctx;
+    (void)closure;
+
+    // tell the backend to discard the event
+    return 0;
+}
+
+static int
+on_crash_callback(const sentry_ucontext_t *uctx, void *closure)
+{
+    (void)uctx;
+    (void)closure;
+
+    // tell the backend to retain the event
+    return 1;
+}
+
 static void
 print_envelope(sentry_envelope_t *envelope, void *unused_state)
 {
@@ -105,6 +150,24 @@ main(int argc, char **argv)
         sentry_options_set_max_spans(options, 5);
     }
 
+    if (has_arg(argc, argv, "before-send")) {
+        sentry_options_set_before_send(options, before_send_callback, NULL);
+    }
+
+    if (has_arg(argc, argv, "discarding-before-send")) {
+        sentry_options_set_before_send(
+            options, discarding_before_send_callback, NULL);
+    }
+
+    if (has_arg(argc, argv, "discarding-on-crash")) {
+        sentry_options_set_on_crash(
+            options, discarding_on_crash_callback, NULL);
+    }
+
+    if (has_arg(argc, argv, "on-crash")) {
+        sentry_options_set_on_crash(options, on_crash_callback, NULL);
+    }
+
     sentry_init(options);
 
     if (!has_arg(argc, argv, "no-setup")) {

include/sentry.h

@@ -86,7 +86,6 @@ extern "C" {
 
 #include <inttypes.h>
 #include <stdarg.h>
-#include <stdbool.h>
 #include <stddef.h>
 
 /* context type dependencies */
@@ -771,6 +770,10 @@ SENTRY_API void sentry_options_set_transport(
  * call `sentry_value_decref` on the provided event, and return a
  * `sentry_value_new_null()` instead.
  *
+ * If you have set an `on_crash` callback (even one that only returns 1),
+ * `before_send` will no longer be invoked for crash-events. This allows you to
+ * better differentiate between crashes and all other events.
+ *
  * This function may be invoked inside of a signal handler and must be safe for
  * that purpose, see https://man7.org/linux/man-pages/man7/signal-safety.7.html.
  * On Windows, it may be called from inside of a `UnhandledExceptionFilter`, see
@@ -804,13 +807,17 @@ SENTRY_API void sentry_options_set_before_send(
  *
  * If the callback returns false outgoing crash report will be discarded.
  *
+ * If this callback is set in the options, a concurrently enabled `before_send`
+ * callback will no longer be called in the crash case. This allows for better
+ * differentiation between crashes and other events.
+ *
  * This function may be invoked inside of a signal handler and must be safe for
  * that purpose, see https://man7.org/linux/man-pages/man7/signal-safety.7.html.
  * On Windows, it may be called from inside of a `UnhandledExceptionFilter`, see
  * the documentation on SEH (structured exception handling) for more information
  * https://docs.microsoft.com/en-us/windows/win32/debug/structured-exception-handling
  */
-typedef bool (*sentry_crash_function_t)(
+typedef int (*sentry_crash_function_t)(
     const sentry_ucontext_t *uctx, void *closure);
 
 /**

src/backends/sentry_backend_breakpad.cpp

@@ -113,8 +113,8 @@ sentry__breakpad_backend_callback(
 
         if (should_handle) {
             sentry_value_t event = sentry_value_new_event();
-            sentry_envelope_t *envelope
-                = sentry__prepare_event(options, event, NULL);
+            sentry_envelope_t *envelope = sentry__prepare_event(
+                options, event, nullptr, !options->on_crash_func);
             // the event we just prepared is empty,
             // so no error is recorded for it
             sentry__record_errors_on_current_session(1);

src/backends/sentry_backend_crashpad.cpp

@@ -43,6 +43,7 @@ extern "C" {
 extern "C" {
 
 #ifdef SENTRY_PLATFORM_LINUX
+#    include <unistd.h>
 #    define SIGNAL_STACK_SIZE 65536
 static stack_t g_signal_stack;
 
@@ -133,10 +134,9 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
 #    endif
     SENTRY_DEBUG("flushing session and queue before crashpad handler");
 
-    bool should_handle = true;
+    bool should_dump = true;
 
     SENTRY_WITH_OPTIONS (options) {
-        sentry__write_crash_marker(options);
 
         if (options->on_crash_func) {
             sentry_ucontext_t uctx;
@@ -149,17 +149,19 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
 #    endif
 
             SENTRY_TRACE("invoking `on_crash` hook");
-            should_handle
-                = options->on_crash_func(&uctx, options->on_crash_data);
+            should_dump = options->on_crash_func(&uctx, options->on_crash_data);
         } else if (options->before_send_func) {
             sentry_value_t event = sentry_value_new_event();
             SENTRY_TRACE("invoking `before_send` hook");
             event = options->before_send_func(
                 event, nullptr, options->before_send_data);
+            should_dump = !sentry_value_is_null(event);
             sentry_value_decref(event);
         }
 
-        if (should_handle) {
+        if (should_dump) {
+            sentry__write_crash_marker(options);
+
             sentry__record_errors_on_current_session(1);
             sentry_session_t *session = sentry__end_current_session_with_status(
                 SENTRY_SESSION_STATUS_CRASHED);
@@ -175,18 +177,52 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
                 sentry_transport_free(disk_transport);
             }
         } else {
-            SENTRY_TRACE("event was discarded by the `on_crash` hook");
+            SENTRY_TRACE("event was discarded");
         }
-
         sentry__transport_dump_queue(options->transport, options->run);
     }
 
     SENTRY_DEBUG("handing control over to crashpad");
 #    ifndef SENTRY_PLATFORM_WINDOWS
     sentry__leave_signal_handler();
 #    endif
-    // further handling can be skipped via on_crash hook
-    return !should_handle;
+
+    // If we __don't__ want a minidump produced by crashpad we need to either
+    // exit or longjmp at this point. The crashpad client handler which calls
+    // back here (SetFirstChanceExceptionHandler) does the same if the
+    // application is not shutdown via the crashpad_handler process.
+    //
+    // If we would return `true` here without changing any of the global signal-
+    // handling state or rectifying the cause of the signal, this would turn
+    // into a signal-handler/exception-filter loop, because some
+    // signals/exceptions (like SIGSEGV) are unrecoverable.
+    //
+    // Ideally the SetFirstChanceExceptionHandler would accept more than a
+    // boolean to differentiate between:
+    //
+    // * we accept our fate and want a minidump (currently returning `false`)
+    // * we accept our fate and don't want a minidump (currently not available)
+    // * we rectified the situation, so crashpads signal-handler can simply
+    //   return, thereby letting the not-rectified signal-cause trigger a
+    //   signal-handler/exception-filter again, which probably leads to us
+    //   (currently returning `true`)
+    //
+    // TODO(supervacuus):
+    // * we need integration tests for more signal/exception types not only
+    //   for unmapped memory access (which is the current crash in example.c).
+    // * we should adapt the SetFirstChanceExceptionHandler interface in
+    // crashpad
+    if (!should_dump) {
+#    ifdef SENTRY_PLATFORM_WINDOWS
+        // TerminateProcess(GetCurrentProcess(), kTerminationCodeCrashNoDump);
+        TerminateProcess(GetCurrentProcess(), 1);
+#    else
+        _exit(EXIT_FAILURE);
+#    endif
+    }
+
+    // we did not "handle" the signal, so crashpad should do that.
+    return false;
 }
 #endif
 

src/backends/sentry_backend_inproc.c

@@ -535,8 +535,8 @@ handle_ucontext(const sentry_ucontext_t *uctx)
         }
 
         if (should_handle) {
-            sentry_envelope_t *envelope
-                = sentry__prepare_event(options, event, NULL);
+            sentry_envelope_t *envelope = sentry__prepare_event(
+                options, event, NULL, !options->on_crash_func);
             // TODO(tracing): Revisit when investigating transaction flushing
             // during hard crashes.
 

src/sentry_core.c

@@ -411,7 +411,7 @@ sentry__capture_event(sentry_value_t event)
         if (sentry__event_is_transaction(event)) {
             envelope = sentry__prepare_transaction(options, event, &event_id);
         } else {
-            envelope = sentry__prepare_event(options, event, &event_id);
+            envelope = sentry__prepare_event(options, event, &event_id, true);
         }
         if (envelope) {
             if (options->session) {
@@ -460,7 +460,7 @@ sentry__should_send_transaction(sentry_value_t tx_cxt)
 
 sentry_envelope_t *
 sentry__prepare_event(const sentry_options_t *options, sentry_value_t event,
-    sentry_uuid_t *event_id)
+    sentry_uuid_t *event_id, bool invoke_before_send)
 {
     sentry_envelope_t *envelope = NULL;
 
@@ -477,7 +477,7 @@ sentry__prepare_event(const sentry_options_t *options, sentry_value_t event,
         sentry__scope_apply_to_event(scope, options, event, mode);
     }
 
-    if (options->before_send_func) {
+    if (options->before_send_func && invoke_before_send) {
         SENTRY_TRACE("invoking `before_send` hook");
         event
             = options->before_send_func(event, NULL, options->before_send_data);

src/sentry_core.h

@@ -40,9 +40,8 @@ bool sentry__event_is_transaction(sentry_value_t event);
  * being passed in is not a transaction.
  *
  * More specifically, it will do the following things:
- * - sample the event, possibly discarding it,
  * - apply the scope to it,
- * - call the before_send hook on it,
+ * - call the before_send hook on it (if invoke_before_send == true),
  * - add the event to a new envelope,
  * - record errors on the current session,
  * - add any attachments to the envelope as well
@@ -51,7 +50,7 @@ bool sentry__event_is_transaction(sentry_value_t event);
  * `event_id` out-parameter.
  */
 sentry_envelope_t *sentry__prepare_event(const sentry_options_t *options,
-    sentry_value_t event, sentry_uuid_t *event_id);
+    sentry_value_t event, sentry_uuid_t *event_id, bool invoke_before_send);
 
 /**
  * Sends a sentry event, regardless of its type.

tests/assertions.py

@@ -1,11 +1,11 @@
 import datetime
 import email
 import gzip
-import sys
 import platform
 import re
-from .conditions import is_android
+import sys
 
+from .conditions import is_android
 
 VERSION_RE = re.compile(r"(\d+\.\d+\.\d+)(?:[-\.]?)(.*)")
 
@@ -194,6 +194,25 @@ def assert_crash(envelope):
     assert_stacktrace(envelope, inside_exception=True, check_size=False)
 
 
+def assert_crash_timestamp(has_files, tmp_path):
+    # The crash file should survive a `sentry_init` and should still be there
+    # even after restarts.
+    if has_files:
+        with open("{}/.sentry-native/last_crash".format(tmp_path)) as f:
+            crash_timestamp = f.read()
+        assert_timestamp(crash_timestamp)
+
+
+def assert_before_send(envelope):
+    event = envelope.get_event()
+    assert_matches(event, {"adapted_by": "before_send"})
+
+
+def assert_no_before_send(envelope):
+    event = envelope.get_event()
+    assert ("adapted_by", "before_send") not in event.items()
+
+
 def assert_crashpad_upload(req):
     multipart = gzip.decompress(req.get_data())
     msg = email.message_from_bytes(bytes(str(req.headers), encoding="utf8") + multipart)