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

FFI is not prod-safe until c_unwind RFC stabilizes and we can use it #113

Open
ryan-johnson-databricks opened this issue Jan 31, 2024 · 3 comments

Comments

@ryan-johnson-databricks
Copy link
Contributor

ryan-johnson-databricks commented Jan 31, 2024

For FFI to really be prod-ready, we need the rust c_unwind RFC to stabilize (see rust-lang/rust#115285 and rust-lang/rust#116088), and then we must update FFI code to take advantage of it. Otherwise, exceptions thrown from C++ code, or panic! originating in rust code, have undefined behavior when unwinding across an FFI boundary.

Context

C++ and Rust take completely different approaches to exception handling.

Rust generally prefers methods to return an error Result when something expected and recoverable goes wrong, and to panic! if the problem is unexpected or unrecoverable. In turn, panic! may be implemented by stack unwinding, if the runtime is configured with panic=unwind.

Meanwhile, C++ relies heavily on exceptions, and on stack unwinding to run destructors as uncaught exceptions propagate. Exceptions can be thrown from just about anywhere (including the standard library) unless extreme care is taken to avoid them.

Because kernel-rs relies heavily on callbacks, and often on double-trampoline callbacks, we must expect cases where an exception thrown from C++ might propagate uncaught through rust code and back into C++ where it is caught and handled. Or for a rust panic to propagate uncaught through C++ code and back to rust code protected by a catch_unwind block.

Unfortunately, any kind of stack unwinding across an FFI boundary (whether C++ -> Rust or Rust -> C++) is undefined behavior today in Rust. The c_unwind RFC proposes to address this by allowing exceptions thrown by extern "c-unwind" fn to safely propagate -- uncaught -- through rust code and back into C++, and for panics in extern "rust-unwind" fn to safely propagate -- also uncaught -- through C++ code and back into rust. Attempting to catch a foreign exception would still produce undefined behavior, but we don't need that capability.

@ryan-johnson-databricks ryan-johnson-databricks changed the title FFI is not prod-safe until c_unwind stabilizes and we can use it FFI is not prod-safe until c_unwind RFC stabilizes and we can use it Jan 31, 2024
@scovich
Copy link
Collaborator

scovich commented Jun 23, 2024

The stabilization PR rust-lang/rust#116088 finally merged and rust-lang/rust#74990 has closed. Now we just need to wait for the next rust release to ship. Among other things, this means duckdb-delta can just throw an exception directly when kernel asks it to allocate an error, which would simplify a lot of FFI code.

@nicklan
Copy link
Collaborator

nicklan commented Jul 8, 2024

As a subtask we sould protect all engine entry points with catch_unwind calls, so panics don't escape into engine. We need the c-unwind protection because otherwise a panic that propagates from kernel to engine and back would be UB. (from here)

@scovich
Copy link
Collaborator

scovich commented Nov 7, 2024

One potential issue: The beta catch_unwind (1.84) docs state:

Although unwinding into Rust code with a foreign exception (e.g. an exception thrown from C++ code, or a panic! in Rust code compiled or linked with a different runtime) via an appropriate ABI (e.g. "C-unwind") is permitted, catching such an exception using this function will have one of two behaviors, and it is unspecified which will occur:

  • The process aborts, after executing all destructors of f and the functions it called.
  • The function returns a Result::Err containing an opaque type.

Thus, if we use c-unwind callbacks (so foreign exceptions can propagate from C++ through kernel and back to C++), then we cannot safely use catch_unwind in the kernel code.

Note: The c-unwind stuff is still propagating through the rust docs ecosystem; today's catch_unwind (1.82) states:

Also note that unwinding into Rust code with a foreign exception (e.g. an exception thrown from C++ code) is undefined behavior.

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

No branches or pull requests

3 participants