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

Undefined behavior: the library panics across FFI boundary #354

Open
Kixunil opened this issue Jan 4, 2022 · 15 comments · May be fixed by #358
Open

Undefined behavior: the library panics across FFI boundary #354

Kixunil opened this issue Jan 4, 2022 · 15 comments · May be fixed by #358

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 4, 2022

Panicking across FFI boundary is UB but the library does it anyway in rustsecp256k1_v0_4_1_default_illegal_callback_fn. This has to be changed to abort. (infinite loop in non-std, I think)

@real-or-random
Copy link
Collaborator

See #290 and the first bullet in #288 (comment)

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 4, 2022

Forgot it's no longer UB. Relying on UB even with test doesn't sound good to me. Once could build the crate with a slightly different compiler or even in non-test mode could cause issues. We literally can't be sure, that's pretty much what "undefined" means.

@real-or-random
Copy link
Collaborator

Well yeah, it's not ideal for sure but I think we've explored all options in #288 and concluded this is the best. Or what would you suggest? You mentioned abort but it requires std.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 5, 2022

#[cfg(feature = "std")]
{
    std::process::abort();
}
#[cfg(not(feature = "std"))]
{
    loop {}
}

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 5, 2022

Another no_std alternative:

struct PanicOnDrop;

impl Drop for PanicOnDrop {
    fn drop(&mut self) {
        panic!("force abort by double panic");
    }
}

let bomb = PanicOnDrop;
panic!("force abort by double panic");

However not sure how it translates on embedded and similar platforms.

@real-or-random
Copy link
Collaborator

I believe the double panic is indeed a way to access core::intrinsics::abort, which is otherwise not exposed in no_std. That may be a reasonable alternative I think.

I'm personally still too lazy to bother with this but feel free to open a PR.

@elichai
Copy link
Member

elichai commented Jan 5, 2022

I'm not sure there's anything to fix here.
right now panicking across FFI boundries is not UB.
Even when it was compiler devs explicitly allowed it to just pass through to support people doing longjumps, See: https://github.com/rust-lang/project-ffi-unwind/blob/master/periodic-summaries/October-2ndHalf.md#letting-extern-c-unwind and a lot of threads in Zulip about it

@apoelstra
Copy link
Member

I tend to agree with @elichai.

I would ACK/accept a PR to use the double-panic bomb, since I think this behavior is still UB on the verison of rustc that mrustc supports (I think), and I appreciate that my "it's not UB on new stuff, and empirically the UB on old stuff has ok behavior" logic is not very sound. But I don't feel strongly enough about this that I would do the work myself.

Kixunil added a commit to Kixunil/rust-secp256k1 that referenced this issue Jan 5, 2022
Panicking across FFI was UB in older Rust versions and thus because of
MSRV it's safer to avoid it. This replaces the panic with print+abort on
`std` and double panic on no-std.

Closes rust-bitcoin#354
@Kixunil Kixunil linked a pull request Jan 5, 2022 that will close this issue
@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 5, 2022

No problem, submitted the PR.

Kixunil added a commit to Kixunil/rust-secp256k1 that referenced this issue Jan 5, 2022
Panicking across FFI was UB in older Rust versions and thus because of
MSRV it's safer to avoid it. This replaces the panic with print+abort on
`std` and double panic on no-std.

Closes rust-bitcoin#354
Kixunil added a commit to Kixunil/rust-secp256k1 that referenced this issue Jan 5, 2022
Panicking across FFI was UB in older Rust versions and thus because of
MSRV it's safer to avoid it. This replaces the panic with print+abort on
`std` and double panic on no-std.

Closes rust-bitcoin#354
@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 5, 2022

Actually I just found out that panic across FFI is still UB - it got reverted and as far as I can see it wasn't reintroduced since then.

@apoelstra
Copy link
Member

Wow!! Good find.

@real-or-random
Copy link
Collaborator

Oh I just sad in #358 that we should let it go.... But that was before I saw the comment here that the change got reverted. To be honest, I don't know what to do.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 6, 2022

I see one, hopefully reasonable, way forward: have static AtomicPtr<fn(&str) -> !> storing the error handler initialized with null and a public function to set it.
The error handling code will load it and:

  • call it if not null
  • call eprintln!() + abort() on std
  • loop forever in no_std

This behavior will be documented for no_std and consumers will be encouraged to provide their error handler.

@elichai
Copy link
Member

elichai commented Jan 6, 2022

Actually I just found out that panic across FFI is still UB - it got reverted and as far as I can see it wasn't reintroduced since then.

Wait, it's true that it got reverted, but only until extern "C-unwind" will be implemented, which it got implemented here: rust-lang/rust#76570, I'm pretty sure it is defined as abort right now, but let me try to get some official clarification here: rust-lang/rust#52652 (comment)

@apoelstra
Copy link
Member

@elichai even if it is defined as of Rust 1.57 (or whatever) I think that's too far in the future (at least two years before that comes under our MSRV, maybe longer because post-Rust-2018 there's are far fewer essential features we're missing), and we need to handle this properly here.

I like @Kixunil's propasal.

Kixunil added a commit to Kixunil/rust-secp256k1 that referenced this issue Jan 7, 2022
Panicking across FFI was UB in older Rust versions and thus because of
MSRV it's safer to avoid it. This replaces the panic with print+abort on
`std` and double panic on no-std.

Closes rust-bitcoin#354
Kixunil added a commit to Kixunil/rust-secp256k1 that referenced this issue Jan 7, 2022
Panicking across FFI was UB in older Rust versions and thus because of
MSRV it's safer to avoid it. This replaces the panic with print+abort on
`std` and double panic on no-std.

Closes rust-bitcoin#354
Kixunil added a commit to Kixunil/rust-secp256k1 that referenced this issue Jan 7, 2022
Panicking across FFI was UB in older Rust versions and thus because of
MSRV it's safer to avoid it. This replaces the panic with print+abort on
`std` and double panic on no-std.

Closes rust-bitcoin#354
Kixunil added a commit to Kixunil/rust-secp256k1 that referenced this issue Jan 7, 2022
Panicking across FFI was UB in older Rust versions and thus because of
MSRV it's safer to avoid it. This replaces the panic with print+abort on
`std` and double panic on no-std.

Closes rust-bitcoin#354
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 a pull request may close this issue.

4 participants