Skip to content

Sentry Patches - #1

Open
jan-auer wants to merge 738 commits into
masterfrom
getsentry
Open

jan-auer wants to merge 738 commits into
masterfrom
getsentry

Conversation

@jan-auer

Copy link
Copy Markdown
Member

This PR contains patches for Sentry. Do not merge.

Swatinem pushed a commit that referenced this pull request Sep 10, 2020
__builtin_trap uses ud2 on x86_64, producing a SIGILL. On arm64, it uses
brk #1, producing a SIGTRAP. Test expectations must be adjusted
accordingly.

Bug: crashpad:345
Test: crashpad_snapshot_test MachOImageAnnotationsReader.CrashModuleInitialization, crashpad_util_test ExcServerVariants.*,ExceptionPorts.*
Change-Id: I22e75b7b48b8887031b1d95f1cea8a09733daf49
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2386464
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
supervacuus pushed a commit to supervacuus/crashpad that referenced this pull request Dec 12, 2022
…Body

This bug was found when trying to upgrading the MSAN bots from Ubuntu
18.04 (where this codepath was not hit) to 20.04.  The following MSAN
error is produced when running HTTPTransport/HTTPTransport.*

==3496553==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5616c540ad7d in __is_long buildtools/third_party/libc++/trunk/include/string:1674:33
    getsentry#1 0x5616c540ad7d in size buildtools/third_party/libc++/trunk/include/string:1069:17
    getsentry#2 0x5616c540ad7d in crashpad::(anonymous namespace)::HTTPTransportLibcurl::WriteResponseBody(char*, unsigned long, unsigned long, void*) third_party/crashpad/crashpad/util/net/http_transport_libcurl.cc:528:50
    ...
    SUMMARY: MemorySanitizer: use-of-uninitialized-value buildtools/third_party/libc++/trunk/include/string:1674:33 in __is_long
  ORIGIN: invalid (0). Might be a bug in MemorySanitizer origin tracking.

The memory is initialized in http_transport_test.cc:293, but MSAN gets
confused.  Given the message output by MSAN (ORIGIN: invalid (0).
Might be a bug in MemorySanitizer origin tracking), this appears
to be a bug in MSAN, not crashpad, so this CL suppresses the error.

Bug: chromium: 1260217
Change-Id: I2d6a46e3489816270cc1fee776793ffafe0147e4
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4015160
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
markmentovai and others added 28 commits January 17, 2024 17:45
Change-Id: I0a30b816e2550e7df6d7777c6d27e6104fc2f9fa
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5206711
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
It's expected that iOS intermediate dumps can be written with
missing information, but it's better to try and report as much as
possible rather than drop the incomplete minidump.

Bug: b/284959148
Change-Id: I04110b576a4ee552814234d559c9ba85db0382f0
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4582167
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
This assumption is non-portable and prevents Chromium from using
bounded iterators in libc++.

Bug: chromium: 1519908
Change-Id: Iafe6639ef3bc896d6fa4fb3ceb7ac0b546363017
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5237292
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
This changes fxbug.dev/ URLs from Monorail bug numbers to the new
Fuchsia Issue Tracker numbers.

The migration to the new issue tracker was announced here:
https://groups.google.com/a/fuchsia.dev/g/announce/c/GOYfJozEqmk/m/qsGsaJ7UAAAJ

Bug: 298074672
Change-Id: I5f4b7a26a3f41bf539fa79d15e1a108ea35a5b29
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5246697
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
The change in macOS 14's dyld to insert new modules in the front of
`dyld_all_image_infos` means that if the any images are loaded
during an exception while iterating the modules list, the primary
executable will be missed. Instead, read the modules in reverse
order.


Change-Id: I49f6468173f18ef4bd0f326c84e4b48cfc696cd3
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5249275
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
This change adds a method to update a CrashpadInfo stream.

As part of this change, AddUserDataMinidumpStream() now returns a handle
to the added stream. This handle can be passed to
UpdateUserDataMinidumpStream() if a new version of the stream needs to
be attached to the crash report.

This method is needed for e.g., allowing Chrome's System Profile to
update, as it contains some data that takes a while to collect.

Bug: crashpad:474
Change-Id: I19e935a6904d8843215582e5606b189479ee338b
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5260024
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Jesse McKenna <jessemckenna@google.com>
If building for chromium, honor the ios_is_app_extension gn variable
that is set per toolchain. When it is defined, the code is built for
an application extension (i.e. -fapplication-extension is passed to
the compiler).

Use CRASHPAD_IS_IOS_APP_EXTENSION build guard to not compile code
that use unavailable extension when ios_is_app_extension is set. If
the variable is not set, then check at runtime whether the API can
be used or not (if the crashpad client uses the same toolchain for
the main application and its application extensions).

This is required to pass -fapplication-extension to the compiler when
building application extensions (which allow catching API that is not
available to application extensions).

Bug: 40120082
Change-Id: I28d545fcfd0f8662430c40ff202b79b0c2b2ff8b
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5286216
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
CrashpadInfo::AddUserDataMinidumpStream() and
UpdateUserDataMinidumpStream() allocate memory for the newly added
streams. This change makes the CrashpadInfo test free that allocated
memory to prevent memory leaks from these tests.

This is intended to fix the ASAN failures seen on crrev.com/c/5285881:
https://ci.chromium.org/ui/p/chromium/builders/try/linux_chromium_asan_rel_ng/1839072/overview

Bug: crashpad:474
Change-Id: I6e030291594d22e316942a58805a177ce448053b
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5292137
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Jesse McKenna <jessemckenna@google.com>
Bug: b/325495632
Change-Id: I19df5b44b76efcdb050344e79bcc2dfd18d8e289
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5299466
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Bug: 325296797
Change-Id: I39f76519c46804ad663172abf91ef582bde135e7
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5310754
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
This adds argv[0] for PLOG(FATAL) calls following a failed posix_spawn
or execve call to make logs more useful.

Bug: chromium:324982367
Change-Id: I179928ec9f791ce5b365b3444aa3bb667f4ec4b3
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5315332
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Chrome on ChromeOS is starting to run into problems where there are more
than 200 annotations, primarily because we use a lot of command-line
switches (40 or more) and commandline-enabled-features as well, each of
which takes up an annotation. It's still rare (100s a day) but will
probably become worse over time as more CrashKey uses are added.

Increase kMaxNumberOfAnnotations to 400.

BUG=296821415

Change-Id: Iba7049014ee3c5ae9c45c4022600eaba50acd403
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5354336
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Ian Barkley-Yeung <iby@chromium.org>
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
There appears to be a change in dyld in macOS 14.3 that iOS 17
accounts for, but older simulators do not. This causes the main
binary to be listed twice when iterating modules, breaking some
tests.

Bug: crbug.com/328282286

Change-Id: I71909fbc13bee6de23b10ffd92a791067f8ea909
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5353754
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
This CL make the iterators implemented by AnnotationList compliant to
the requirements imposed by the C++ standard on input iterators.

Change-Id: I263c94a97f5bcd7edd5ef4d8b65fa28b11876974
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5093147
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Arm's Pointer Authentication uses two keys for signing pointers, A-key
and B-key. Although by default Clang uses the A-key if PAC support is
enabled at compile time, this behaviour might be overridden via compiler
command line.

This CL fixes the check for the B-key being enabled. The key that shall
be used for Pointer Authentication is denoted by bits 0 (A-key) or
1 (B-key) of __ARM_FEATURE_PAC_DEFAULT. Hence, the previous way of
checking by using bits 0 and 2 does not correctly identify the B-key.

Bug: 40608466
Change-Id: Ib2f226baa12a7145fa0b6e486e49d36e6b0a3cd7
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5341090
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
After https://crrev.com/c/5375084, Chromium __libcpp_verbose_abort is
handled differently for official non-dcheck builds. This change fixes
the test expectation for release non-official builds.

Bug: 330168249
Change-Id: Iceb6d327f9e93fd366cc07abe27eefd1adf06472
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5378380
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Previously, Crashpad would only capture iOS thread context memory regions by iterating the task_threads->thread_get_state's. For Mach
exception this worked as intended.  However, for signal exceptions this
missed the registers from the actual signal context. This change
correctly captures these regions and stores them in the exception
snapshot.

Change-Id: I494e753a25c2687e61b5183ed0135f520ca8bf52
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5380505
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
This should give crashpad a newer xcode and msvc that support C++20.

These changes need to land separately from the C++20 usage, as they do
not get applied until after landing, so can't affect the CQ from inside
the CQ.

Bug: 40284755
Change-Id: I3ae72befa008bfb37bac882de0986c5bcf9de079
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5425460
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Also enables C++20 as span now depends on it.

Roll buildtools to grab a newer libc++ that supports C++20.

Explicitly capture `this` in lambdas in cpp-httplib as the implicit
capture through `=` is deprecated and causes an error in C++20.

Update the MacOS version to "Mac-13|Mac-14" which is the current
value of `os.MAC_DEFAULT` in Chromium infra in order to have
C++20 support in the std library on iOS.

Moves iOS tests to run on iPhone 13 and includes a mini_chromium roll
to fix Xcode 14.3 egtests.

Bug: 40284755
Change-Id: Ic078f07d12473f2aaed5e84df0f0a7fb7b8c35c3
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5443384
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
There were two issues with the iOS implementation of CrashpadClient
which I reported in https://crbug.com/crashpad/481:

1) TSAN found a data race in ResetForTesting() when it modified the
ScopedMachReceiveRight while the Mach exception port thread was
reading it

2) The Mach port connected to the exception server was never deallocated

This CL fixes both issues.

Change-Id: I5bd4f79ae6d0eccca954d663be7a36f8ceb0a0e8
Bug: https://crbug.com/crashpad/481
Bug: b:332305593
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5410301
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
base/sys_byteorder.h is going away. Instead, use the byte conversions
in base::numerics to convert from a byte array in big endian to an
integer. This avoids putting big endian data into integer types at all.

mini_chromium was rolled and crashpad updated to work with newer
mac/windows toolchains in order to support C++20 in
f9cee5c.

Bug: 40284755
Change-Id: If690847b7aa54b0216e73ec297eae3d0bca2fa57
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5402184
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
process_info_test.cc

Bug: crashpad: 326459035,326458915,326459055
Change-Id: Ifb91297b6097aa81a9d5c883b2c284e9fdd512a8
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5463361
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Arthur Wang <wuwang@chromium.org>
This DCHECK() was not correct. When dumping a process with CET
enabled the cetumsr and cetussp registers are not available in
the context obtained for the exception record. All contexts to be
written to a minidump must have the same context format so those
registers will be present for captured threads. It is therefore ok for
the context to expect extended xsave registers but for them to be
zero in some cases.

Bug: 337665168
Change-Id: If7e5f40fe8eda6799b034991cb87e89437cb4821
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5507588
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Alex Gough <ajgo@chromium.org>
Replacing std::is_pod usage as per the following compilation error:
```
../../util/misc/uuid.cc:44:20: error: 'is_pod<crashpad::UUID>' is deprecated: use 'is_standard_layout && is_trivial' instead [-Werror,-Wdeprecated-declarations]
static_assert(std::is_pod<UUID>::value, "UUID must be POD");
                   ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/type_traits:818:5: note: 'is_pod<crashpad::UUID>' has been explicitly marked deprecated here
    _GLIBCXX20_DEPRECATED_SUGGEST("is_standard_layout && is_trivial")

```

Bug: None
Change-Id: I1d61ee12261877f7f1f84f0ea15d262d22959766
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/5472885
Commit-Queue: Andrew Williams <awillia@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
@sentry

sentry Bot commented Jul 21, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 15375497

Runtime attachment IPC can race on Windows: the handler writes a response and
then disconnects the named pipe before the client has read it. Under stress this
can abort the crashing process in Crashpad with ERROR_PIPE_NOT_CONNECTED or
"!IsSocketHandle(file)", preventing normal crash handling.
@sentry

sentry Bot commented Jul 22, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 15408543

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread CMakeLists.txt
* feat: Add attachment update IPC

Allow clients to send replacement attachment contents to the handler with
variable-length IPC messages. The handler writes the updated bytes to the
attachment path while keeping attachment registration unchanged.

Co-Authored-By: OpenAI Codex <noreply@openai.com>

* Drop IPC on Unix - simple file operations are fast enough

* Clean up Windows

* don't wait for write - defeats the purpose

* cap payload size

* add owner checks to the new IPC methods

* fix typo

* add clarifying comments

* serialize

* file lock

* flush response

* attachments_lock_

* restrict

* restrict more

* lock

* startup vs. user attachments

* use base::span to prevent copies

* restore original removal behavior to fix integration tests

* length checks

---------

Co-authored-by: OpenAI Codex <noreply@openai.com>
@sentry

sentry Bot commented Jul 24, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 15463964

* chore(deps): bump mini_chromium

* Bump mini_chromium
@sentry

sentry Bot commented Jul 27, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 15500343

@sentry

sentry Bot commented Aug 3, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 15670005

)

--wait-for-upload (#121) makes the handler hold the dump-done signal until it
has finished with the report, so that a client whose process lifetime bounds
the handler's - a container's pid 1, or a systemd unit - does not exit while
the report is still in flight.

The client half was never extended to match. RequestCrashDumpHandler waits for
that signal with a hardcoded five second sigtimedwait and discards the result,
so once the handler needs longer the client stops waiting, reraises, and dies
anyway. For a pid 1 client the kernel then tears down the pid namespace and
SIGKILLs the handler mid-write, losing the whole report rather than just its
upload. Five seconds is easy to exceed: a process with a few hundred threads
writing its minidump to a network filesystem takes tens of seconds, and an
upload only adds to that. The failure is silent - a timed-out wait is
indistinguishable from a completed one.

Linux is the outlier here. The Windows client sleeps INFINITE after signalling
the handler, and macOS waits with kMachMessageTimeoutWaitIndefinitely; only
Linux caps the wait, and it does so without saying anything.

Plumb wait_for_upload through to the client. When it is set:

- SignalCrashDump waits for as long as the handler is still holding its end of
  the socket, polling for a peer hangup so that a handler which dies without
  signalling cannot hang the crashing process.
- WaitForDumpDone, which any other thread taking a fatal signal blocks on,
  drops its own five second cap. That cap is measured against the dumping
  thread, not the handler, so leaving it would let a second crashing thread
  reraise and tear the process down while the first thread's dump was still
  being written - which is exactly the report this change exists to keep. It
  rechecks dump_done_futex_ rather than the return value, because FUTEX_WAIT
  is documented to wake spuriously.

Waiters now depend on the dumping thread reaching WakeThreads(), and a
first-chance handler returning true previously returned without getting there,
leaving them to wait for a dump that was never coming. Wake them on that path.
That was already worth fixing: before this change they stalled for the full
five seconds instead.

Default behaviour is unchanged: without --wait-for-upload both waits keep
their existing five second timeouts.
@sentry

sentry Bot commented Aug 3, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 15670450

Check the database upload setting before launching an external crash reporter.
Leave the report pending when uploads are disabled so it can be processed later.
@sentry

sentry Bot commented Aug 11, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 15852443

* fix(win): accept minimal PDB70 CodeView records

Validate the serialized record length instead of the padded C++ struct
size so records with an empty PDB filename are accepted.

* fix(win): validate CodeView PDB name terminator

Reject malformed PDB70 records without a trailing NUL before
constructing a string from the filename field.

* continue
@sentry

sentry Bot commented Aug 20, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 16079151

Resolve final attachments from the `__sentry-attachments` manifest file
if present. This makes it possible to modify attachments during crash
handling via `hint` - IPC is not usable at that point. The manifest is
only written if crash-time modifications are detected, and in that
case it overrides/replaces the entire list of attachments to support
all operations including clearing the attachments.
@sentry

sentry Bot commented Sep 21, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 16916388

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

mpack_node_type(path_node) != mpack_type_str ||
mpack_node_strlen(path_node) == 0) {
mpack_tree_destroy(&attachment);
return attachments;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Manifest parse error drops attachments

Medium Severity

A later invalid or truncated MessagePack object in __sentry-attachments makes ResolveAttachments throw away paths already parsed and return the original list. Those files are not in the original list, so a torn append or a single bad entry drops every manifest attachment from the crash report.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 000e3ad. Configure here.

@sentry

sentry Bot commented Sep 29, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 17160445

* feat: Support optional stack capture size limit

* style: Preserve existing brace formatting

* style: Match existing Crashpad formatting

* Reuse CrashpadInfo padding for stack capture size
@sentry

sentry Bot commented Sep 30, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 17203667

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread snapshot/linux/thread_snapshot_linux.cc
`CaptureMemoryDelegateLinux` excludes the full stack even when
`max_stack_capture_size` truncates the snapshot. With indirect memory
gathering enabled, referenced memory in the discarded portion is omitted.

Pass a `ProcessReaderLinux::Thread` copy with `stack_region_size` set to
the captured size so the exclusion matches the snapshot. This preserves
the reader's thread metadata and captures extra memory within its budget.

Original change: #172
Reported by Cursor Bugbot while reviewing `44144ca6` in PR #1:
#1 (comment)
@sentry

sentry Bot commented Oct 3, 2026

Copy link
Copy Markdown

🚧 Skipped: PR exceeds review size limit.

Please split into smaller PRs and re-run.
Reference ID: 17312017

* ref(win): Expose RegisterHandlers for handler reinstallation

Expose `CrashpadClient::RegisterHandlers()` so sentry-native can restore
the Windows crash handlers after another runtime replaces them, such as
Unity installing its own handlers.

Without access to this method, `sentry_reinstall_backend()` shuts down
and starts the backend again. This launches another handler process and
reloads breadcrumbs into the existing ring, duplicating them. Exposing
the registration method lets the SDK preserve its client, handler
process, IPC pipe, and breadcrumb state.

Ref: getsentry/sentry-native#2182

* fix(win): Buffer attachment updates until a report is built

The attachment IPC round trip waits for acknowledgment, which is sent
before the request's disk write. The write then continues on the pipe
service thread, keeping that pipe instance occupied. Later breadcrumb
or scope updates can block on disk I/O from earlier requests when both
instances are busy. Acknowledging before storing also allows consecutive
updates to reach the delegate out of order, so a truncating write can
erase a later append.

Buffer each writable file under `attachments_lock_` before acknowledging
its update. Writes replace pending content and appends extend it. Flush
the buffers while the client is suspended, before resolving attachments
or reading the external crash reporter envelope. Both crash dumps and
`DumpWithoutCrash` use this path.

Append-only updates extend preloaded file contents, while truncation
keeps breadcrumb buffers bounded as the ring rotates. The handler kept
across sentry-native backend reinstall preserves these pending updates.

Ref: getsentry/sentry-native#2182

* fix(win): Flush pending attachments before building the envelope

The external crash reporter path deletes the database report, so the
envelope is the only artifact. Flush again under the envelope lock so an
update stored during the dump is included. The map is empty after the
first flush, so only late arrivals are written.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 6 total unresolved issues (including 5 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 493151b. Configure here.

}
}
pending_attachments_.clear();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Failed writes drop pending attachment data

Medium Severity

WritePendingAttachments always clears pending_attachments_ after the loop, including when Open or Write fails. A truncate Open can already wipe the on-disk file before Write fails, so both the previous file contents and the buffered payload are gone and never reach the crash report.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 493151b. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.