Skip to content
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.

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

Add panic guards to extern "C" functions #68

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Mar 20, 2023

Prevents undefined behavior in the event that any functions panic.

@MarijnS95
Copy link
Member

MarijnS95 commented Mar 20, 2023

Previously folks used panic_unwind to also print the error/backtrace, is it desired to ignore these nowadays?

@notgull
Copy link
Contributor Author

notgull commented Mar 20, 2023

I'd also be fine with panic_unwind, it's just that from the codebase I get the impression that a panic here is likely a critical bug that can't be recovered from.

Let me try adding it anyways

@rib
Copy link
Collaborator

rib commented Mar 29, 2023

thanks, having something to map panics in extern C functions to an abort sounds good. (at the very least we shouldn't be letting panics unwind across any Java or C FFI boundaries)

I didn't follow what extra protection was provided by the AbortOnPanic struct with the Drop handler when the code uses catch_unwind to catch any panic+unwind and abort? The AbortOnPanic stuff seemed redundant?

Adding the catch_unwind around android_main is probably the most important thing here that we should have done before (at least for most of the other ones they would hopefully only panic due to an internal bug, but a panic from the application will be a common occurrence)

@notgull
Copy link
Contributor Author

notgull commented Mar 29, 2023

I didn't follow what extra protection was provided by the AbortOnPanic struct with the Drop handler when the code uses catch_unwind to catch any panic+unwind and abort? The AbortOnPanic stuff seemed redundant?

This protects undefined behavior from occurring if the panic-handling code panics.

@MarijnS95
Copy link
Member

I'd also be fine with panic_unwind, it's just that from the codebase I get the impression that a panic here is likely a critical bug that can't be recovered from.

Yeah, so keep the abort() but still print a friendly message which won't happen with the original implementation? It seems you're doing that now though.

@rib
Copy link
Collaborator

rib commented Mar 29, 2023

Prevents undefined behavior in the event that any functions panic.

Ah, right, I see. If we're worried that the catch code might panic though, can we consider just using catch_unwind for both cases for readability. The intent for the Drop code wasnt super obvious to me at least and I think the closure scope for a second catch_unwind might have made it more obvious that we want to also handle secondary panics too.

I'm also wondering if we should specifically catch around the application's android_main to distinguish how we deal with an internal panic vs an application panic.

@rib
Copy link
Collaborator

rib commented Apr 24, 2023

For reference here, I rebased your changes @notgull and also updated the implementation of abort_on_panic to use catch_panic for the extra-paranoid check in case we panic while logging the first panic.

I've also put a TODO comment in there to note that actually we probably shouldn't be aborting at all.

I recently raised it as an issue in Winit because it's problematic that Winit's run API would exit the whole process and the same thing applies here I think. An Android Activity doesn't own the whole process because there could be other Services, or even other Activitys running in other threads which we shouldn't be nuking if we've safely caught a panic.

At some point we should probably throw appropriate Java exceptions via JNI instead.

I haven't yet looked at putting a more precise catch around the application's android_main which I think would make sense here too (so we can handle app panics differently to internal panics).

(also re-ordered the separate change to consolidate the logging API, and squashed other related bits together)

@rib rib requested a review from MarijnS95 April 24, 2023 15:20
Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Few suggestions, but generally looks okay.

android-activity/src/util.rs Outdated Show resolved Hide resolved
android-activity/src/util.rs Show resolved Hide resolved
android-activity/src/util.rs Outdated Show resolved Hide resolved
@rib rib merged commit 6559dc8 into rust-mobile:main Apr 27, 2023
@notgull notgull deleted the abort-guards branch April 27, 2023 20:39
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.

3 participants