Skip to content

fix(logs): move logs flush after on_crash call #1474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JoshuaMoelans merged 1 commit into from
Jan 8, 2026
+16 −13
Merged

fix(logs): move logs flush after on_crash call #1474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/backends/sentry_backend_breakpad.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,6 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor,
event, "level", sentry__value_new_level(SENTRY_LEVEL_FATAL));

SENTRY_WITH_OPTIONS (options) {
// Flush logs in a crash-safe manner before crash handling
if (options->enable_logs) {
sentry__logs_flush_crash_safe();
}

sentry__write_crash_marker(options);

bool should_handle = true;
Expand All @@ -157,6 +152,11 @@ breakpad_backend_callback(const google_breakpad::MinidumpDescriptor &descriptor,
should_handle = !sentry_value_is_null(result);
}

// Flush logs in a crash-safe manner before crash handling
if (options->enable_logs) {
sentry__logs_flush_crash_safe();
}

if (should_handle) {
sentry_envelope_t *envelope = sentry__prepare_event(
options, event, nullptr, !options->on_crash_func, nullptr);
Expand Down
10 changes: 6 additions & 4 deletions src/backends/sentry_backend_crashpad.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,10 +331,6 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
bool should_dump = true;

SENTRY_WITH_OPTIONS (options) {
// Flush logs in a crash-safe manner before crash handling
if (options->enable_logs) {
sentry__logs_flush_crash_safe();
}
auto state = static_cast<crashpad_state_t *>(options->backend->data);
sentry_value_t crash_event
= sentry__value_new_event_with_id(&state->crash_event_id);
Expand All @@ -359,6 +355,12 @@ sentry__crashpad_handler(int signum, siginfo_t *info, ucontext_t *user_context)
crash_event = options->before_send_func(
crash_event, nullptr, options->before_send_data);
}

// Flush logs in a crash-safe manner before crash handling
if (options->enable_logs) {
sentry__logs_flush_crash_safe();
Comment on lines 357 to +361
Copy link

@sentry sentry bot Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The on_crash callback is now called before logs are flushed. If the callback crashes or hangs, all crash telemetry and logs will be lost.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The order of operations within the crash handler has been changed, moving the execution of the user-provided on_crash_func callback to before the sentry__logs_flush_crash_safe() function is called. The on_crash_func is executed within a signal handler context without any timeout or error handling wrapper. If the user's callback code crashes, enters an infinite loop, or otherwise hangs, the execution flow will be interrupted. As a result, the log flushing mechanism will never be reached, leading to the complete loss of all pre-crash logs and preventing the crash report from being sent to Sentry. This is a regression, as logs were previously flushed before executing the potentially unstable user callback.

💡 Suggested Fix

Revert the order of operations in the crash handler. Call sentry__logs_flush_crash_safe() before invoking the user-provided options->on_crash_func. This ensures that logs are safely written to disk before executing potentially faulty user code, preserving critical diagnostic information even if the callback fails.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/backends/sentry_backend_crashpad.cpp#L357-L361

Potential issue: The order of operations within the crash handler has been changed,
moving the execution of the user-provided `on_crash_func` callback to before the
`sentry__logs_flush_crash_safe()` function is called. The `on_crash_func` is executed
within a signal handler context without any timeout or error handling wrapper. If the
user's callback code crashes, enters an infinite loop, or otherwise hangs, the execution
flow will be interrupted. As a result, the log flushing mechanism will never be reached,
leading to the complete loss of all pre-crash logs and preventing the crash report from
being sent to Sentry. This is a regression, as logs were previously flushed before
executing the potentially unstable user callback.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8276555

Copy link
Member Author

@JoshuaMoelans JoshuaMoelans Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while this is true, we explicitly warn users about on_crash being signal-safe, and if the callback crashes/hangs we would lose our crash events anyway (which we consider user error from not providing a proper on_crash function).

}

should_dump = !sentry_value_is_null(crash_event);

if (should_dump) {
Expand Down
9 changes: 5 additions & 4 deletions src/backends/sentry_backend_inproc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,10 +649,6 @@ handle_ucontext(const sentry_ucontext_t *uctx)
// use a signal-safe allocator before we tear down.
sentry__page_allocator_enable();
#endif
// Flush logs in a crash-safe manner before crash handling
if (options->enable_logs) {
sentry__logs_flush_crash_safe();
}

sentry_value_t event = make_signal_event(sig_slot, uctx, strategy);
bool should_handle = true;
Expand All @@ -664,6 +660,11 @@ handle_ucontext(const sentry_ucontext_t *uctx)
should_handle = !sentry_value_is_null(event);
}

// Flush logs in a crash-safe manner before crash handling
if (options->enable_logs) {
sentry__logs_flush_crash_safe();
}

if (should_handle) {
sentry_envelope_t *envelope = sentry__prepare_event(
options, event, NULL, !options->on_crash_func, NULL);
Expand Down
Loading