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

Handle panicking in "extern" fns more safely #7

Closed
Lej77 opened this issue Nov 8, 2021 · 2 comments
Closed

Handle panicking in "extern" fns more safely #7

Lej77 opened this issue Nov 8, 2021 · 2 comments
Labels
bug Something isn't working

Comments

@Lej77
Copy link

Lej77 commented Nov 8, 2021

I was looking through the source code of this crate after reading the article Plugins in Rust: Reducing the Pain with Dependencies | NullDeref and I noticed that currently panicking is only handled for the LocalBorrowingFfiFuture::poll method and even that could be better since the Box that the is caught could panic when it is dropped, see Footgun with catch_unwind when catching panic-on-drop types · Issue #86027 · rust-lang/rust. This crate should probably do something similar to the preposed drop_unwind fn or to the AbortBomb in abi_stable.

All of the following extern "C" functions should probably handle panics since they call arbitrary user provided functions:

@oxalica oxalica added the bug Something isn't working label Nov 8, 2021
@Lej77
Copy link
Author

Lej77 commented Nov 10, 2021

The new code looks good and I can't find any safety issues with it.

I do think that the abort message should be Waker::clone and not Waker::drop at line 194 for the owned vtable clone method.

Also in the poll_fn function, is there a reason to not call Pin::new_unchecked and (*context_ptr).with_context inside the catch_unwind closure (seems like that would be safer if that code was ever changed to introduce a panic somewhere even if they can't panic currently).

@oxalica
Copy link
Owner

oxalica commented Nov 11, 2021

I do think that the abort message should be Waker::clone and not Waker::drop at line 194 for the owned vtable clone method.

Also in the poll_fn function, is there a reason to not call Pin::new_unchecked and (*context_ptr).with_context inside the catch_unwind closure (seems like that would be safer if that code was ever changed to introduce a panic somewhere even if they can't panic currently).

Thanks for the review. They are both fixed in b81ef7f now. Documentations are also updated on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants