Don't panic across FFI #358

Kixunil · 2022-01-05T15:58:58Z

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 #354

apoelstra

ACK 5deccc1

Pretty cool that this works with the existing abort/panic tests.

apoelstra · 2022-01-05T16:07:23Z

Oh :) it doesn't, it's just that my local test script doesn't exercise the abort tests. Oops.

Kixunil · 2022-01-05T16:08:54Z

Yeah, hopefully this fixes it (letting CI test that).

real-or-random

ACK ffac2d6

Do we have a no_std test in CI? I assumed we have but apparently no?

bjorn3 · 2022-01-05T20:25:00Z

secp256k1-sys/src/lib.rs

+        }
+
+        let _bomb = PanicOnDrop(&msg);
+        panic!("[libsecp256k1] {}", &msg)


I think this is still UB. The unwinder works in two passes. In the first pass it looks for a catch landing pad. In the second pass it actually unwinds. The double panic only happens on the second pass, but on the first pass I think optimizations due to the nounwind attribute may already cause UB. One optimization I can imagine is that LLVM sees that for example rustsecp256k1_v0_4_1_default_illegal_callback_fn can't unwind and then propagates this info to ffi_abort which then results in the landingpad for PanicOnDrop being removed.

That makes sense, great that you pointed it out!

I will wrap it in catch_unwind then and put loop {} at the end just in case.

That should indeed fix the UB I think.

Unfortunately I just found that catch_unwind is unavailable without std, looks like the only way is to loop forever. :(

Right, forgot about that. Does this library ever get used in places where there is no libc or would it be possible to call abort() from libc? I believe that is what std::process::abort() does on most platforms.

Or maybe write a C function that uses a compiler intrinsic to abort and then call this function?

Yeah, using C function is probably the best way, especially since this crate already contains lot of C.

@bjorn3 Yes, this library is widely used in wasm32-unknown-unknown
We thought about writing a small c function that calls __builtin_trap but that precludes MSVC: #288 (comment)

This brings me back to this: #354 (comment)

(side note, Thank you for taking the time to look at this and respond here! @bjorn3 )

I think we should let it go.

apoelstra · 2022-01-05T22:41:37Z

IIRC we cannot use the C abort because it is not available in wasm.

bjorn3's comments

Kixunil · 2022-01-07T11:57:42Z

Rebased and implemented #354 (comment)

Kixunil · 2022-01-07T11:58:29Z

Oh, forgot to suggest the static trick (see test) in the doc but have to run right now, will fix it soon.

secp256k1-sys/src/lib.rs

Kixunil · 2022-01-07T15:38:20Z

Done.

apoelstra

ACK 96ddf07

I would like another ACK, maybe from @elichai, before merging.

Thank you for the thorough comment. I think the one about "you should check the docs about abort if you are using no_std" might be unnecessary, since in the expected case it should be impossible to ever call this abort handler.

secp256k1-sys/src/lib.rs

Kixunil · 2022-01-07T18:02:41Z

I was hoping "libsecp256k1 may want to abort in case of invalid inputs. These are definitely bugs." was clear that it shouldn't happen in practice but if you think it's not clear enough I can try reword it.

I don't like leaving this, even unlikely, case without calling it out, especially because e.g. embedded platforms usually have some kind of trap/reset instruction and may even have an interface for debug printing the message (seen that on STM32).
IMO it's pretty low effort for potentially avoiding annoying situation.

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 · 2022-01-07T18:04:58Z

Fixed that comment.

apoelstra

ACK 4aabbbd

I think the comments are fine, thanks for explaining.

I'm unsure that these pointer-casting methods are actually needed, but I can't convince myself that they're not, so I'll let them be. (I think as *const _ as *mut _ should have the same guarantees. But I'm not sure.)

Kixunil · 2022-01-07T22:24:07Z

I opened a discussion about this and even Ralf Jung agrees which to me is a very strong indication it's a good idea. :) The other conversion wasn't strictly required but I like keeping it as documentation.

apoelstra · 2022-01-07T23:32:00Z

Nice :)

I continue to want one more concept ACK on this before merging.

RCasatta · 2022-01-10T12:28:01Z

secp256k1-sys/src/lib.rs

+
+/// Ensures that types both sides of cast stay in sync and only the constness changes.
+///
+/// This elliminates the risk that if we change the type signature of abort handler the cast


s/elliminates/eliminates

Fixed in separate commit to make review easier.

Did you also check soundness? We need more soundness reviewers.

honestly, I haven't the knowledge for a soundness review on this

apoelstra

ACK 540c783

apoelstra · 2022-01-24T18:11:08Z

cc @elichai think we can merge this?

elichai · 2022-01-24T21:21:36Z

I'd prefer a bit more time to think of alternatives if this isn't urgent.
For the future, I'm trying to get core::intrinsics::abort stabilized

The reasons I'm not excited about this idea:

Setting an abort handler is unergonomic and feels like no user will ever use that (they probably won't even know it exists).
looping forever hangs the CPU and doesn't provide any feedback to the user.
loop{} honestly scares me a little bit (see LLVM loop optimization can make safe programs crash rust-lang/rust#28728)

Some half-baked alternatives suggestions:

Can we maybe get upstream to change their definitions such that the calling function is promised to return if the callback returns? then either they promise to return 0/-1 in that case, or we can set a global error atomic here (although that's gonna infect the whole codebase unless we use a thread local which we don't have without stdlib)
Maybe a stack-overflow is better than an infinite loop? (Is that a security risk? also scary)
inline assembly should be stable in rust 1.59, so we can maintain a list of abort instructions(we can copy off of glibc and it should be a one time thing), but I can see why some people won't like it, and also it's a problem with our MSRV.

(FYI, it looks like abort is definitely a hard problem: https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/abort.c)

bjorn3 · 2022-01-25T08:28:23Z

Maybe a stack-overflow is better than an infinite loop? (Is that a security risk? also scary)

Not all OSes use a stack guard page. On those that don't a stack overflow may smash the heap.

Kixunil · 2022-01-25T09:23:28Z

I'm not excited about that either. The reason I proposed it is it seemed the least bad option. At the time I thought loop {} was long fixed but now that I look at it again, it was only fixed somewhat recently.

they probably won't even know it exists

There's a bold warning about it in the docs. If someone doesn't read the docs at all, especially for security-critical software, they will have to suffer consequences. There's only so much we can do against stupidity.

looping forever hangs the CPU and doesn't provide any feedback to the user

Non-issue for std builds, no-std presumably don't have an OS, which means they're running bare metal - this problem is literally unsolvable in a library.

loop{} honestly scares me

Started to scare me now that I learned it wasn't long ago that it was fixed.

Can we maybe get upstream to change their definitions such that the calling function is promised to return if the callback returns?

It already does, the return values are just unspecified. We could set an atomic variable and check it after each call or use &mut context and have it on the stack but that's annoying. Or use setjmp/longjmp. I agree maybe we should make it just return a specified error value.

Maybe a stack-overflow is better than an infinite loop?

I believe the opposite.

inline assembly should be stable in rust 1.59

Yes, MSRV may be less of a problem for embedded folks who have to use a recent Rust version anyway.

Now I can see only these solutions:

Make the function a symbol the user has to provide, failing compilation on no_std if it's missing.
Bump MSRV for no_std to high-enough value where Rust can support this
Convince upstream to return defined error values
Do some other ugly hack

Out of these, failing compilation on no_std looks best to me at least for now. In 5 years we will bump MSRV and make it even better.
Note that I also think keeping the ability to set the handler like I implemented is a useful feature so I prefer to not discard the code entirely.

apoelstra · 2022-01-25T13:09:05Z

I still think @Kixunil's solution is the least bad option.

The idea of smashing the stack is tempting ... the Rust developers do take pains to make this safe even on architectures that don't help but I agree that this feels like a bad and dangerous idea. I also agree that using loop {} is quite disconcerting. We could replace that with a couple nested loops which update an atomic value 2^96 times, say, followed by a panic! that we know will never actually be executed.

I also don't want to fail compilation. These functions are really supposed to be impossible to hit, I don't want to inconvenience users for their sake.

Kixunil · 2022-01-25T13:18:47Z

Actually, we can just perform relaxed load from an atomic with panic if it's some value which we will make sure never happens but the compiler should be unable to prove it. Actually, volatile read is even better than atomic.

apoelstra previously approved these changes Jan 5, 2022

View reviewed changes

Kixunil dismissed apoelstra’s stale review via 6ff72f8 January 5, 2022 16:07

Kixunil force-pushed the fix-ffi-panic-ub branch from 5deccc1 to 6ff72f8 Compare January 5, 2022 16:07

Kixunil force-pushed the fix-ffi-panic-ub branch from 6ff72f8 to ffac2d6 Compare January 5, 2022 16:13

real-or-random previously approved these changes Jan 5, 2022

View reviewed changes

bjorn3 reviewed Jan 5, 2022

View reviewed changes

real-or-random mentioned this pull request Jan 6, 2022

Undefined behavior: the library panics across FFI boundary #354

Open

Kixunil force-pushed the fix-ffi-panic-ub branch from ffac2d6 to 40919c2 Compare January 7, 2022 11:56

Kixunil marked this pull request as draft January 7, 2022 11:58

elichai reviewed Jan 7, 2022

View reviewed changes

secp256k1-sys/src/lib.rs Outdated Show resolved Hide resolved

Kixunil force-pushed the fix-ffi-panic-ub branch from 40919c2 to 0f351a7 Compare January 7, 2022 15:38

Kixunil marked this pull request as ready for review January 7, 2022 15:38

Kixunil force-pushed the fix-ffi-panic-ub branch from 0f351a7 to 96ddf07 Compare January 7, 2022 16:20

apoelstra previously approved these changes Jan 7, 2022

View reviewed changes

Kixunil commented Jan 7, 2022

View reviewed changes

secp256k1-sys/src/lib.rs Outdated Show resolved Hide resolved

Don't panic across FFI

4aabbbd

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 dismissed apoelstra’s stale review via 4aabbbd January 7, 2022 18:04

Kixunil force-pushed the fix-ffi-panic-ub branch from 96ddf07 to 4aabbbd Compare January 7, 2022 18:04

apoelstra previously approved these changes Jan 7, 2022

View reviewed changes

RCasatta reviewed Jan 10, 2022

View reviewed changes

Fixed typo

540c783

Kixunil dismissed apoelstra’s stale review via 540c783 January 10, 2022 14:28

apoelstra approved these changes Jan 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't panic across FFI #358

Don't panic across FFI #358

Kixunil commented Jan 5, 2022

apoelstra left a comment

apoelstra commented Jan 5, 2022

Kixunil commented Jan 5, 2022

real-or-random left a comment

bjorn3 Jan 5, 2022 •

edited

Loading

Kixunil Jan 5, 2022

bjorn3 Jan 5, 2022

Kixunil Jan 5, 2022

bjorn3 Jan 5, 2022

bjorn3 Jan 5, 2022

Kixunil Jan 5, 2022

elichai Jan 6, 2022

elichai Jan 6, 2022

real-or-random Jan 6, 2022

apoelstra commented Jan 5, 2022

Kixunil commented Jan 7, 2022

Kixunil commented Jan 7, 2022

Kixunil commented Jan 7, 2022

apoelstra left a comment

Kixunil commented Jan 7, 2022

Kixunil commented Jan 7, 2022

apoelstra left a comment

Kixunil commented Jan 7, 2022

apoelstra commented Jan 7, 2022

RCasatta Jan 10, 2022

Kixunil Jan 10, 2022 •

edited

Loading

RCasatta Jan 10, 2022

apoelstra left a comment

apoelstra commented Jan 24, 2022

elichai commented Jan 24, 2022 •

edited

Loading

bjorn3 commented Jan 25, 2022

Kixunil commented Jan 25, 2022

apoelstra commented Jan 25, 2022

Kixunil commented Jan 25, 2022 •

edited

Loading

Don't panic across FFI #358

Are you sure you want to change the base?

Don't panic across FFI #358

Conversation

Kixunil commented Jan 5, 2022

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 5, 2022

Kixunil commented Jan 5, 2022

real-or-random left a comment

Choose a reason for hiding this comment

bjorn3 Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra commented Jan 5, 2022

Kixunil commented Jan 7, 2022

Kixunil commented Jan 7, 2022

Kixunil commented Jan 7, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil commented Jan 7, 2022

Kixunil commented Jan 7, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil commented Jan 7, 2022

apoelstra commented Jan 7, 2022

Choose a reason for hiding this comment

Kixunil Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 24, 2022

elichai commented Jan 24, 2022 • edited Loading

bjorn3 commented Jan 25, 2022

Kixunil commented Jan 25, 2022

apoelstra commented Jan 25, 2022

Kixunil commented Jan 25, 2022 • edited Loading

bjorn3 Jan 5, 2022 •

edited

Loading

Kixunil Jan 10, 2022 •

edited

Loading

elichai commented Jan 24, 2022 •

edited

Loading

Kixunil commented Jan 25, 2022 •

edited

Loading