Skip to content

feat: make the crashpad handler path configurable - #2853

Closed
mollstam wants to merge 2 commits into
getsentry:mainfrom
Keepsake-Games:expose-handler-path
Closed

mollstam wants to merge 2 commits into
getsentry:mainfrom
Keepsake-Games:expose-handler-path

Conversation

@mollstam

Copy link
Copy Markdown

sentry-unity never calls sentry_options_set_handler_path, so sentry-native falls back to looking for crashpad_handler.exe next to the game executable.

Player use eg MelonLoader to load mods into the game, it hooks by placing itself as version.dll in game root directory which causes the Unity runtime to load it on launch (it's looking for version.dll in the Windows system dir but local copy takes precedence). This is naturally risky code and may crash the game, at which point the Sentry crashpad triggers. If crashpad_handler.exe is in the game root directory (which it currently has to be) it will also load the injected version.dll into the handler itself. It causes crashpad to crash leaving the process suspended and no crash report delivered.

Binding handler_path lets the handler be staged outside the game root. Moving ours to <Game>_Data/Plugins/x86_64/ restored native crash capture on modded installs.

Not sure about the changelog additions etc, let me know if you need anything changed. 🙇🏻

sentry-native resolves its out-of-process crash handler next to the running
executable unless handler_path is set, and the bridge never set it. Add
SentryUnityOptions.NativeHandlerPath and pass it through, using the wide
variant on Windows the way database_path already does.

@bitsandfoxes bitsandfoxes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution! Other than those two minor tweaks this looks like it's good to go!

#if !SENTRY_NATIVE_SWITCH
// Left unset, sentry-native looks for the handler next to the running executable.
// Switch does not run an out-of-process handler, so the option does not apply there.
if (options.NativeHandlerPath is not null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (options.NativeHandlerPath is not null)
if (!string.IsNullOrWhiteSpace(options.NativeHandlerPath))

Ideally, we'd actually check for the file to be there. But that would require a change to the underlying submodule so it's fine for now.

Comment thread CHANGELOG.md Outdated
Co-authored-by: Stefan Jandl <reg@bitfox.at>
@bitsandfoxes

Copy link
Copy Markdown
Contributor

Thanks a lot for the contribution and the detailed write-up, @mollstam! Since the branch doesn't allow maintainer edits and needed a rebase plus a couple of fixes (Cdecl on the new P/Invokes after #2847, changelog placement), we carried your commit over with authorship intact in #2881. Closing this in favor of that one.

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.

2 participants