Not catching panics across FFI boundaries

[SimonSapin]

gtk-rs/cairo#257 added bindings for “user data” owned by cairo objects. Each user data entry has a destructor function pointer that cairo calls when the object is destroyed. This helps solve life time issues, since with reference counting it can be hard to predict when an object will be destroyed exactly.

The bindings are generic and accept user data of any type. They forward destructor function pointer calls to Drop, which potentially panic and unwind. However, until rust-lang/rust#58794 is resolved, unwinding into C is undefined behavior.

A possible fix is using std::panic::catch_unwind in the destructor function. However even if we stash the panic payload somewhere at that point, there is no good place in the code to call resume_unwind. So the best we can do might be to abort the process.

[sdroege]

Abort seems the best, yes.

[SimonSapin]

Or wait until the compiler does this abort for us? I think that’s the expected eventual resolution of rust-lang/rust#58794 (though with an opt-out that I’m not sure is implemented yet.)

[sdroege]

Personally I'd wait for the compiler, I don't want to change everything back yet another time only to revert the change again in a bit.

[sdroege]

Made this issue a bit more generic. This applies to all gtk-rs code currently.

[jhg]

Are there news about if the compiler implement it or not yet?

[sdroege]

There's a whole WG about that: https://rust-lang.github.io/rfcs/2797-project-ffi-unwind.html / https://github.com/rust-lang/project-ffi-unwind . It's not finished yet.

[jhg]

Thanks for the information @sdroege 🤗

[SimonSapin]

The status quo is that unwinding across FFI is Undefined Behavior. Therefore all callbacks called from C should be wrapped into something that uses catch_unwind + abort.

Only if and when that WG propose languages changes that end up implemented, maybe that wrapping will become unnecessary.

[jhg]

@SimonSapin I found rust-lang/rust#58794 is closed and in rust-lang/rust#58760 link to rust-lang/rust#55982 merged PR. Is that also UB when the FFI is Rust function to be used from C or that fix the UB? (even if it's in nightly only yet and it is not unwind but at least is not undefined, it will abort).

And thanks about the information about FFI and unwind.

[sdroege]

Currently any unwinding across extern "C" functions is UB, even if all those functions happens to be implemented in Rust. That's part of what that WG is working on solving.

For example this adds support for an extern "C-unwind" ABI that explicitly allows unwinding (and AFAIU causes unwinding through extern "C" to abort as it should).

[bilelmoussaoui]

@sdroege this should be moved to gtk-rs-core

[piegamesde]

I don't understand one thing in here: the initial issue description is specifically about Cairo "user data". But doesn't the problem also apply in any signal callbacks? I've already panicked in my code more than once and it's never blown up badly, so was it just by luck?

[sdroege]

Yes, it's about any callbacks that go through FFI.