feat: external crash reporter

[jpnurmi]

When an external crash reporter is registered, instead of posting minidump + attachments straight to Sentry, Crashpad now dumps them into an envelope, spawns and detaches the external crash reporter process, and passes the envelope as an argument.

Required for:

• feat: external crash reporter sentry-native#1303

[jpnurmi]

@sentry review

[seer-by-sentry]

On it! We are reviewing the PR and will provide feedback shortly.

[seer-by-sentry]

PR Description

This pull request introduces a mechanism to launch an external feedback handler after a crash has been processed. This allows for custom post-crash actions, such as gathering additional diagnostic information or notifying users about the crash.

Click to see more

Key Technical Changes

1. Added feedback_handler and feedback_path parameters to the BuildHandlerArgvStrings function and the CrashpadClient::StartHandler method, propagating these parameters through the client and handler code.
2. Introduced a FeedbackReport class in crash_report_database to encapsulate the creation of a feedback envelope file containing crash data and attachments.
3. Implemented LaunchFeedbackHandler methods in CrashReportDatabaseGeneric, CrashReportDatabaseMac, and CrashReportDatabaseWin to launch the external handler process, using platform-specific mechanisms like SpawnSubprocess (Linux/Mac) and CreateProcessW (Windows).
4. Modified the crash exception handlers (Linux, Mac, Windows) to create a FeedbackReport, add attachments and minidump data, and launch the feedback handler after writing the minidump to the database.

Architecture Decisions

The design uses a file-based approach for communication between the crash handler and the feedback handler. The crash handler creates a feedback envelope file containing crash data and attachments, and passes the path to this file as an argument to the feedback handler. This approach decouples the crash handler from the feedback handler, allowing for greater flexibility and extensibility. The FeedbackReport class encapsulates the logic for creating this feedback envelope.

Dependencies and Interactions

This change depends on the util/posix/spawn_subprocess.h utility for launching processes on Linux and Mac, and util/win/command_line.h for Windows command line handling. It interacts with the CrashReportDatabase to store crash reports and launch the feedback handler. It also interacts with the file system to create and manage the feedback envelope file and attachments.

Risk Considerations

1. The external feedback handler could potentially introduce security vulnerabilities if it is not properly vetted. Users should be cautious about the executables they configure as feedback handlers.
2. The feedback handler launch could fail due to various reasons (e.g., executable not found, permissions issues). Robust error handling and logging are important to diagnose such failures.
3. The size of the feedback envelope file could become large if many attachments are included. Consider adding mechanisms to limit the size or number of attachments.
4. The Windows implementation needs to validate the command line length to prevent potential buffer overflows or command line truncation.

Notable Implementation Details

1. The FeedbackReport class creates a JSON-like envelope format for the feedback data. The format includes type, length, attachment_type, and filename for each attachment.
2. The Mac implementation uses the open command to launch .app bundles as feedback handlers.
3. The Windows implementation uses CreateProcessW with DETACHED_PROCESS to launch the feedback handler in a separate process.

[supervacuus]

This makes all sense.

[supervacuus]

There are some original unit tests from Crashpad, but they are unmaintained and excluded from the CMake port of the build system.

We can think about enabling the upstream tests, given that we have added considerable changes to our crashpad fork that no longer maintain compatibility with upstream. Tests allow us to verify whether any of our changes affect (untouched) upstream behavior, and would further provide a basis for isolated unit tests, which is less effort for an envelope writer than going full circle via the consuming integration tests.

However, it isn't necessary to include this in this PR.

[jpnurmi]

msgpack/msgpack-cxx looked nice at first because it's a header-only library, but then I noticed the size:

SLOC    Directory       SLOC-by-Language (Sorted)
77415   msgpack-cxx     cpp=73844,ansic=3571

ludocode/mpack looks more reasonable:

SLOC    Directory       SLOC-by-Language (Sorted)
22304   zlib            ansic=16260,cpp=6020,sh=24
17980   nlohmann-json   cpp=17980
9583    mini_chromium   cpp=8429,python=1154
7840    mpack           ansic=7840
4852    lss             ansic=4852
1839    cpp-httplib     cpp=1839
598     xnu             ansic=598
309     getopt          cpp=309

What about the JSON library? Is nlohmann/json ok or should we look for something slimmer?

[supervacuus]

In general, I would love to cut down dependencies to an absolute minimum. If there is a way in which we can share between the Native SDK and the crashpad repos, I'd vote for it, even if it means to vendor redundantly or create a local fork + submodule.

Even more important than source size is the effect on binary size. C++ code tends to grow binaries dramatically due to the combination of template instantiation, aggressive inlining, and dependencies on the standard library. So, even a slim-looking header-only "modern" C++ library can have severe consequences on binary size. The crashpad_handler is big enough as is.

In addition to that - if it is modern C++ - it can also introduce problems for building and deployment since those libraries might depend on features that are not evenly supported by toolchains used downstream (this is a recurring pain and it applies on a finer-grained level than C++ standard version, because particular features of the standard are not bound to appear as implementations in the same toolchain version).

msgpack/msgpack-cxx looked nice at first because it's a header-only library, but then I noticed the size:

SLOC    Directory       SLOC-by-Language (Sorted)
77415   msgpack-cxx     cpp=73844,ansic=3571

ludocode/mpack looks more reasonable:

SLOC    Directory       SLOC-by-Language (Sorted)
22304   zlib            ansic=16260,cpp=6020,sh=24
17980   nlohmann-json   cpp=17980
9583    mini_chromium   cpp=8429,python=1154
7840    mpack           ansic=7840
4852    lss             ansic=4852
1839    cpp-httplib     cpp=1839
598     xnu             ansic=598
309     getopt          cpp=309

Concretely, to the above, ludocode/mpack is the MsgPack library we vendor here: https://github.com/getsentry/sentry-native/tree/master/vendor. So, yes, I would be delighted if we use it 😅 . It is fine to vendor it in crashpad redundantly.

What about the JSON library? Is nlohmann/json ok, or should we look for something slimmer?

I wonder if we could somehow reuse the existing capabilities of the Native SDK here instead of introducing yet another full-featured JSON implementation, of which we only use a small portion.

For instance, just thinking out loud: what if you embed MsgPack encoded files from the crashpad_handler like you did before (or not even embed just "bundle"), and do the entire decoding of MsgPack and conversion back to JSON when you load it in the feedback UI (by using a Native SDK exposed function that is aware of message-pack encoded attachments)?

This way, you could eliminate the need to introduce all these details to the crashpad_handler and use it solely for what it is intended: handling crash snapshots and uploading reports outside the monitored application. There is significant value in keeping the payloads beyond the minidump opaque for the crashpad_handler.

[jpnurmi]

Sharing code between sentry-native and crashpad would be useful! I still remember well the duplicate screenshotting code. 😓

While I hesitated to make Crashpad aware of the __sentry-foo attachments, this way the external crash reporter can submit the envelope straight to Sentry without any processing. The Sentry Crash Reporter reference GUI might not use the Native SDK, by the way. Thanks to the process separation, we can choose any technology. Currently, the strongest candidate is .NET, because the .NET SDK can AOT-compile self-contained, easy-to-deploy static executables out of the box.

[jpnurmi]

Hmm, if we had IPC on macOS, we could pass breadcrumbs one by one instead of writing them into files... 🤔

[jpnurmi]

Static Crashpad builds with MSVC 2022 (-DCMAKE_BUILD_TYPE=RelWithDebInfo -DBUILD_SHARED_LIBS=OFF -DSENTRY_BACKEND=crashpad)

master

jpnurmi@surface MINGW64 ~/Projects/sentry/sentry-native (master)
$ ls -lah build/master/RelWithDebInfo/
total 49M
drwxr-xr-x 1 jpnurmi 197610    0 Aug 13 10:49 ./
drwxr-xr-x 1 jpnurmi 197610    0 Aug 13 10:49 ../
-rwxr-xr-x 1 jpnurmi 197610 1.2M Aug 13 10:49 crashpad_handler.exe*
-rw-r--r-- 1 jpnurmi 197610  13M Aug 13 10:49 crashpad_handler.pdb
-rwxr-xr-x 1 jpnurmi 197610  38K Aug 13 10:47 crashpad_wer.dll*
-rw-r--r-- 1 jpnurmi 197610 604K Aug 13 10:47 crashpad_wer.pdb
-rw-r--r-- 1 jpnurmi 197610  42K Aug 13 10:49 minidump.dmp
-rw-r--r-- 1 jpnurmi 197610 2.4M Aug 13 10:49 sentry.lib
-rw-r--r-- 1 jpnurmi 197610 868K Aug 13 10:49 sentry.pdb
-rwxr-xr-x 1 jpnurmi 197610 812K Aug 13 10:49 sentry_example.exe*
-rw-r--r-- 1 jpnurmi 197610 6.8M Aug 13 10:49 sentry_example.pdb
-rwxr-xr-x 1 jpnurmi 197610 760K Aug 13 10:48 sentry_fuzz_json.exe*
-rw-r--r-- 1 jpnurmi 197610 6.9M Aug 13 10:48 sentry_fuzz_json.pdb
-rwxr-xr-x 1 jpnurmi 197610 806K Aug 13 10:49 sentry_screenshot.exe*
-rw-r--r-- 1 jpnurmi 197610 6.9M Aug 13 10:49 sentry_screenshot.pdb
-rwxr-xr-x 1 jpnurmi 197610 1.1M Aug 13 10:48 sentry_test_unit.exe*
-rw-r--r-- 1 jpnurmi 197610 7.5M Aug 13 10:48 sentry_test_unit.pdb
-rw-r--r-- 1 jpnurmi 197610  273 Aug 13 10:49 view-hierarchy.json

feat/feedback-handler

jpnurmi@surface MINGW64 ~/Projects/sentry/sentry-native (feat/feedback-handler)
$ ls -lah build/feedback-handler/RelWithDebInfo/
total 84M
drwxr-xr-x 1 jpnurmi 197610    0 Aug 13 10:52 ./
drwxr-xr-x 1 jpnurmi 197610    0 Aug 13 10:52 ../
-rwxr-xr-x 1 jpnurmi 197610 1.6M Aug 13 10:52 crashpad_handler.exe*
-rw-r--r-- 1 jpnurmi 197610  17M Aug 13 10:52 crashpad_handler.pdb
-rwxr-xr-x 1 jpnurmi 197610  38K Aug 13 10:51 crashpad_wer.dll*
-rw-r--r-- 1 jpnurmi 197610 604K Aug 13 10:51 crashpad_wer.pdb
-rw-r--r-- 1 jpnurmi 197610  42K Aug 13 10:52 minidump.dmp
-rw-r--r-- 1 jpnurmi 197610 2.4M Aug 13 10:52 sentry.lib
-rw-r--r-- 1 jpnurmi 197610 868K Aug 13 10:52 sentry.pdb
-rwxr-xr-x 1 jpnurmi 197610 1.1M Aug 13 10:52 sentry_crash_reporter.exe*
-rw-r--r-- 1 jpnurmi 197610  11M Aug 13 10:52 sentry_crash_reporter.pdb
-rwxr-xr-x 1 jpnurmi 197610 1.2M Aug 13 10:52 sentry_example.exe*
-rw-r--r-- 1 jpnurmi 197610  11M Aug 13 10:52 sentry_example.pdb
-rwxr-xr-x 1 jpnurmi 197610 1.1M Aug 13 10:52 sentry_fuzz_json.exe*
-rw-r--r-- 1 jpnurmi 197610  12M Aug 13 10:52 sentry_fuzz_json.pdb
-rwxr-xr-x 1 jpnurmi 197610 1.2M Aug 13 10:52 sentry_screenshot.exe*
-rw-r--r-- 1 jpnurmi 197610  12M Aug 13 10:52 sentry_screenshot.pdb
-rwxr-xr-x 1 jpnurmi 197610 1.4M Aug 13 10:52 sentry_test_unit.exe*
-rw-r--r-- 1 jpnurmi 197610  12M Aug 13 10:52 sentry_test_unit.pdb
-rw-r--r-- 1 jpnurmi 197610  273 Aug 13 10:52 view-hierarchy.json

[jpnurmi]

The sizes look reasonable with nlohmann-json replaced by manual JSON formatting:

jpnurmi@surface MINGW64 ~/Projects/sentry/sentry-native (feat/feedback-handler)
$ ls -lah build/manual-json/RelWithDebInfo/
total 58M
drwxr-xr-x 1 jpnurmi 197610    0 Aug 13 12:50 ./
drwxr-xr-x 1 jpnurmi 197610    0 Aug 13 12:50 ../
-rwxr-xr-x 1 jpnurmi 197610 1.3M Aug 13 12:50 crashpad_handler.exe*
-rw-r--r-- 1 jpnurmi 197610  13M Aug 13 12:50 crashpad_handler.pdb
-rwxr-xr-x 1 jpnurmi 197610  38K Aug 13 12:48 crashpad_wer.dll*
-rw-r--r-- 1 jpnurmi 197610 604K Aug 13 12:48 crashpad_wer.pdb
-rw-r--r-- 1 jpnurmi 197610  42K Aug 13 12:50 minidump.dmp
-rw-r--r-- 1 jpnurmi 197610 2.4M Aug 13 12:50 sentry.lib
-rw-r--r-- 1 jpnurmi 197610 868K Aug 13 12:50 sentry.pdb
-rwxr-xr-x 1 jpnurmi 197610 782K Aug 13 12:50 sentry_crash_reporter.exe*
-rw-r--r-- 1 jpnurmi 197610 7.0M Aug 13 12:50 sentry_crash_reporter.pdb
-rwxr-xr-x 1 jpnurmi 197610 842K Aug 13 12:50 sentry_example.exe*
-rw-r--r-- 1 jpnurmi 197610 7.0M Aug 13 12:50 sentry_example.pdb
-rwxr-xr-x 1 jpnurmi 197610 787K Aug 13 12:49 sentry_fuzz_json.exe*
-rw-r--r-- 1 jpnurmi 197610 7.0M Aug 13 12:49 sentry_fuzz_json.pdb
-rwxr-xr-x 1 jpnurmi 197610 836K Aug 13 12:50 sentry_screenshot.exe*
-rw-r--r-- 1 jpnurmi 197610 7.0M Aug 13 12:50 sentry_screenshot.pdb
-rwxr-xr-x 1 jpnurmi 197610 1.1M Aug 13 12:49 sentry_test_unit.exe*
-rw-r--r-- 1 jpnurmi 197610 7.7M Aug 13 12:49 sentry_test_unit.pdb
-rw-r--r-- 1 jpnurmi 197610  273 Aug 13 12:50 view-hierarchy.j

[jpnurmi]

Hey @supervacuus, is it ok to merge this? I think it's ready to go, but there were significant changes after the approval. I ended up dropping nlohmann/json in favor of manual JSON formatting.

I agree that keeping crashpad unaware of the msg-packed attachments would be nice in principle, but passing them unprocessed to the external crash reporter would force the external crash reporter to special-case crashpad envelopes and do non-trivial pre-processing before being able to submit envelopes to Sentry.