Conversation
7ea304b to
11f94c3
Compare
| if (sentry__atomic_fetch(&g_preloaded)) { | ||
| g_crash_ipc = NULL; | ||
| SENTRY_DEBUG("crash handler deactivated, keeping preloaded handlers"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: The signal stack (g_signal_stack.ss_sp) allocated during preload is never freed on shutdown, causing a memory leak.
Severity: MEDIUM
Suggested Fix
The shutdown logic for the preloaded case should be updated to free the resources it allocated. Specifically, the memory for g_signal_stack.ss_sp should be freed and the signal stack should be disabled before the function returns, even when g_preloaded is true.
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/native/sentry_crash_handler.c#L920-L924
Potential issue: When the crash handler is preloaded by calling
`sentry__crash_handler_preload()`, memory is allocated for a signal stack and assigned
to `g_signal_stack.ss_sp`. However, during shutdown
(`sentry__crash_handler_shutdown()`), an early return is taken if the `g_preloaded` flag
is set. This bypasses the cleanup logic that frees the allocated signal stack. As a
result, the memory for `g_signal_stack.ss_sp` is never deallocated, leading to a memory
leak in applications that use the preload feature and then shut down the handler.
| char *serialized = sentry_envelope_serialize(envelope, &size_out); | ||
| printf("%s", serialized); |
There was a problem hiding this comment.
Bug: The return value of sentry_envelope_serialize is not checked for NULL before being passed to printf, which can cause a crash in out-of-memory situations.
Severity: LOW
Suggested Fix
Add a null-check for the serialized variable after calling sentry_envelope_serialize. If the variable is NULL, handle the error gracefully, for example by skipping the printf call, to prevent a crash.
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/transports/sentry_transport_stdout.c#L11-L12
Potential issue: Under extreme out-of-memory conditions, the function
`sentry_envelope_serialize` can return a `NULL` pointer. This occurs if an internal
1-byte memory allocation for an empty string fails. The code passes this `NULL` return
value directly to `printf` without a check. This results in undefined behavior and is
known to cause a segmentation fault and crash on platforms like Android using the Bionic
libc.
| char *serialized = sentry_envelope_serialize(envelope, &size_out); | ||
| printf("%s", serialized); | ||
| fflush(stdout); | ||
| sentry_free(serialized); |
There was a problem hiding this comment.
Stdout transport null dereference
Medium Severity
When sentry_envelope_serialize fails and returns NULL, print_envelope still passes the result to printf and sentry_free, which is undefined behavior and can crash the process.
Reviewed by Cursor Bugbot for commit 57004b7. Configure here.
| fail: | ||
| if (ipc->init_mutex) { | ||
| sentry__mutex_unlock(ipc->init_mutex); | ||
| } | ||
| if (ipc->shmem) { | ||
| munmap(ipc->shmem, SENTRY_CRASH_SHM_SIZE); | ||
| } | ||
| if (ipc->notify_fd >= 0) { |
There was a problem hiding this comment.
Bug: The native initialization function closes caller-provided file descriptors on failure without notifying the caller, leading to stale file descriptors on the Java side.
Severity: HIGH
Suggested Fix
The callee (sentry__crash_ipc_init_app_with_fds) should not close file descriptors that it does not own. The function should instead return an error code to the caller, sentry_android_crash_daemon_init, which can then propagate the failure status back to the Java layer. The caller, which owns the file descriptors, should be responsible for closing them upon learning of the initialization failure.
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/native/sentry_crash_ipc.c#L117-L124
Potential issue: During native crash daemon initialization via `nativeInitCrashDaemon`,
file descriptors (`notify_fd`, `ready_fd`) are passed from Java to the native layer. If
an error occurs during the subsequent initialization in
`sentry__crash_ipc_init_app_with_fds` (e.g., an `mmap()` failure), the error handling
logic closes these file descriptors. However, the original caller on the Java side is
not notified of this failure or that the descriptors have been closed. This leaves the
caller with stale file descriptors, which can lead to data corruption or crashes if they
are used later, as they might have been recycled by the OS for other resources.
6460208 to
527d4dc
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 4 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 527d4dc. Configure here.
| ipc->shmem->version = SENTRY_CRASH_VERSION; | ||
| sentry__atomic_store(&ipc->shmem->state, SENTRY_CRASH_STATE_READY); | ||
| sentry__atomic_store(&ipc->shmem->sequence, 0); | ||
| } |
There was a problem hiding this comment.
Stale Android shm is reused
High Severity
App-side Android IPC only initializes magic, version, and READY state when it creates a new shm file. An existing leftover mapping is reused as-is, so a prior CRASHED or DONE state can prevent the next session from claiming a crash.
Reviewed by Cursor Bugbot for commit 527d4dc. Configure here.
| = (uint32_t)((app_pid ^ (app_tid & 0xFFFFFFFF)) & 0xFFFFFFFF); | ||
| snprintf(ipc->shm_path, sizeof(ipc->shm_path), "%s/.sentry-shm-%08x", | ||
| ipc->shmem->database_path, id); | ||
| } |
There was a problem hiding this comment.
Daemon unlinks reconstructed shm path
Medium Severity
The Android daemon maps the shm file that sentry_android_crash_daemon_run opened, but then rebuilds ipc->shm_path from database_path and pid^tid. Cleanup unlinks that reconstructed name, not the path Java actually used, so the real shm file leaks and a different .sentry-shm-* file in the database directory can be removed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 527d4dc. Configure here.
| } | ||
| if (ipc->ready_fd >= 0) { | ||
| close(ipc->ready_fd); | ||
| } |
There was a problem hiding this comment.
Init failure closes caller fds
Medium Severity
The Android app IPC fail path closes notify_fd and ready_fd even when those descriptors were supplied by the caller. A failed shm open or mmap in the service-hosted path then invalidates Java-owned eventfds, so later native or JVM use of those numbers can hit the wrong file.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 527d4dc. Configure here.
| #if defined(SENTRY_PLATFORM_ANDROID) | ||
| if (g_android_active_crash_daemon | ||
| && g_android_active_crash_daemon->envelope_path[0]) { | ||
| g_android_active_crash_daemon->options = options; | ||
| g_android_active_crash_daemon->ipc = ipc; | ||
| g_android_active_crash_daemon->log_file = log_file; | ||
| return 0; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Bug: The new early return path in the Android daemon may leak resources allocated within sentry__process_crash, as they are not transferred for deferred cleanup before returning.
Severity: MEDIUM
Suggested Fix
Ensure that all resources allocated within sentry__process_crash are properly cleaned up before the new early return path is taken. This might involve refactoring sentry__process_crash to return allocated resources or performing cleanup within the function before it returns in this specific path.
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/native/sentry_crash_daemon.c#L5343-L5351
Potential issue: In the Android service daemon mode, an early return path has been
added. This path is executed after `sentry__process_crash` completes. While ownership of
`options`, `ipc`, and `log_file` is correctly transferred for later cleanup, any
intermediate resources allocated within the complex `sentry__process_crash` function
that are not explicitly transferred will be leaked. This creates a resource leak risk
when crashes are successfully processed in the Android daemon, potentially leading to
degraded performance over time.
Did we get this right? 👍 / 👎 to inform future reviews.
| sentry_envelope_free(envelope); | ||
| envelope = merged_envelope; | ||
| sentry__path_remove(android_native_envelope_path); | ||
| sentry__path_remove(merged_path); | ||
| sentry__path_free(merged_path); | ||
| } else { | ||
| sentry_envelope_free(envelope); | ||
| envelope = NULL; | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Bug: If make_run_envelope_path fails on Android, the crash envelope is incorrectly freed and the report is silently lost instead of being processed.
Severity: HIGH
Suggested Fix
Modify the logic to handle the case where make_run_envelope_path returns NULL. Instead of freeing the envelope and losing the crash report, the code should proceed to capture the existing envelope without waiting for a tombstone, ensuring the report is not lost.
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/native/sentry_crash_daemon.c#L4770-L4820
Potential issue: On Android, if creating an envelope path fails (e.g., due to an
out-of-memory error or a nil event UUID), the `android_native_envelope_path` variable
will be `NULL`. This `NULL` value is then passed to
`wait_for_android_tombstone_envelope`, which correctly handles it by returning `NULL`.
However, the calling code interprets this `NULL` return as a failure and proceeds to
free the original `envelope`. As a result, the crash report is never sent to the
transport, and the Android daemon service is not notified, causing the crash to be
silently lost.
Did we get this right? 👍 / 👎 to inform future reviews.


Warning
WIP 🚧🔨⏳⛔
This enables the out-of-process native crash daemon on Android, including file-backed IPC, daemon packaging as
libsentry-crash.so, backend-owned daemon path resolution, and preload support for managed runtimes.The NDK integration now selects native crash reporting mode from the Android SDK tombstone merging setting: tombstone-enabled builds avoid minidumps because minidumps take precedence over native envelopes, while tombstone-disabled builds keep native-with-minidump for richer crash data.
This also adds Android daemon support for signal-handler chaining, crash envelope queueing,
libunwindstackunwinding, and focused Android integration coverage.A hidden
SENTRY_TRANSPORT=stdouttransport is added for tests so they can exercise the SDK default transport path without usingSENTRY_TRANSPORT=custom, which remains available for downstream SDKs.