Passing callbacks to C: panic!

help
1

When passing a function pointer to a C function (e.g. as done here), do I need to make sure that if that function is called from C that it never panics?

I'm asking because I found the abort_on_panic crate's documentation:

When calling Rust code from C, it's unsafe to call panic!. Doing so may cause unsafe behavior. But when calling user-defined functions, we sometimes need to enforce these rules.

I also found this side note in another thread here:

Or is this done automatically when/if I declare a function as being extern "C" (as done here)?

2

Generally, if the Rust code can panic, you must use catch_unwind to stop the panic from unwinding into C code.

3 Likes
3

Okay, that is very important to know! I somehow missed this. Maybe because it wasn't explained in the tutorials I looked at for working with callbacks in regard to C. Or I overlooked it. I think it's easy to miss this.

4

In regard to my particular use-case, I have a related question:

I assume I cannot rely on std::cmp::Ord::cmp not panicking in general. However, would it be safe to assume that <[u8] as Ord>::cmp never panics?

5

Technically, unwinding across an extern "C" was/has been indeed UB, even though in practice it was caught and aborted.

After some Rust version which I can't remember at the moment, this was made a bit more official/guaranteed, while waiting for the proper C-unwind initiative (BatmanAoD was a member of it, but I don't know if I should ping them here), so I'm tempted to say it's not UB anymore, but don't quote me on that. cc @LegionMammal978 who has been the one most recently focusing on this aspect in Rust code out there

2 Likes
6

Note that the official docs on catch_unwind currently state:

It is currently undefined behavior to unwind from Rust code into foreign code, so this function is particularly useful when Rust is called from another language (normally C). This can run arbitrary Rust code, capturing a panic and allowing a graceful handling of the error.

So either that info is outdated and would need to be updated, or the fact that it does get caught and aborted is not that "official" yet, I guess. Or there are other ways to call Rust without declaring a function being extern "C"?

Found tracking issue #74990 for feature c_unwind.

Apparently there seem to be a lot of different ABIs. I didn't know that.

Basically the problem is that some C code might not expect the callback doing a longjmp, right? Or is that yet another problem on top of being "undefined behavior to unwind from Rust code into foreign code"?

7

Right now, unwinding through extern "C" is nominally UB but in practice unwinds through C frames as if it were longjmp(), and C++ frames as if it were an exception. If you add #![feature(c_unwind)], then extern "C" will add an abort-on-unwind shim to the function. The plan is to make the latter behavior the default once c_unwind is stabilized, but that may take many months yet. There have been some soft promises made about the former behavior on GitHub and on Zulip, but I haven't been able to track down my old list of links.

2 Likes
8

I just tried it by modifying the example code of sandkiste_lua, and I can confirm how it unwinds through the C functions on my system.

So I read this as:

  • There is currently no practical problem if I don't add a catch_unwind myself.
  • It's believed to be guaranteed at some point to be no problem in the future, but that's no guarantee yet.

I think in practice I have no problem in my original case anyway, because no reasonable implementation of the unsafe method mmtkvdb::Storable::cmp_bytes_unchecked (which is what I invoke in my callback) would allocate or ever have to report an error.

I guess I could add to its "Safety" section a requirement that implementors must ensure that the method doesn't panic, but it's not really a practical issue I guess (both because it's not UB in practice to panic and because it's very unlikely any implementation would ever want to panic). But I'll think about that. I would like to avoid adding an extra, unnecessary catch_unwind, especially if such a "catch" is later added by Rust anyway.

The above case of sandkiste_lua::LuaMachine::callback, however, is much more likely to panic in practice.

Thanks a lot for your feedback and advice/info.

Regarding what I wrote here:

I guess that's entirely unrelated. This is what !UnwindSafe is for, so I don't use any values which are in a weird state because of unwinding (except in Drop handlers, which must be prepared for such a case).

Update:

:thinking: … but what happens if my drop handlers try to clean up an LMDB environment after I longjmped through it? :scream: Maybe LMDB is not prepared to be cleaned up (e.g. using mdb_env_close) in that case and will crash badly.

So perhaps I should tackle that issue and add proper "Safety" requirements that forbid panickingunwinding, so I don't have that problem.

Also, if I understand it right, safe code can always ignore the fact that some value ends up in a bad way, e.g. by using AssertUnwindSafe, so I must ensure myself that I properly "poison" such a value (or abort on panic).

1 Like
9

Adding your own abort-on-panic shim can be pretty lightweight; it doesn't even need catch_unwind(). It just needs a drop guard:

#[no_mangle]
extern "C" fn example(callback: fn()) {
    struct AbortOnDrop;
    impl Drop for AbortOnDrop {
        fn drop(&mut self) {
            std::process::abort();
        }
    }
    let guard = AbortOnDrop;
    callback();
    std::mem::forget(guard);
}

catch_unwind() is only necessary if you want to stop the panic and continue execution on the current thread, or if you want to soundly cross an FFI boundary before resuming the panic. (In general, I would not expect C libraries to be longjmp-safe except by accident.)

2 Likes
10

I think I won't use the abort-on-panic shim in case of mmtkvdb (as I really want the compare function to be fast) and rather solve this by documenting that the comparison method (of the unsafe trait) must not panic, but I'll keep that in mind. I might use it for sandkiste_lua, but I think it is safe to do a longjmp through Lua (it's what Lua does too, the only issue might be a number of values on the Lua stack which could cause a stack overflow when not accounted for :thinking:).

11

You can make it even nicer, without the forget, by checking std::thread::panicking() in drop.

1 Like
12

Like this?

struct AbortOnPanic;
impl Drop for AbortOnPanic {
    fn drop(&mut self) {
        if std::thread::panicking() {
            std::process::abort();
        }
    }
}

fn main() {
    {
        let _guard = AbortOnPanic;
        panic!("This versus...")
    }
    //panic!("...that");
}

(Playground)

13

Yes

14

I wonder if it has slightly more runtime overhead.

15

Having thought about this again, I guess it's not safe to longjmp from a C (or Rust) function which has been passed to Lua with lua_pushcfunction. There is no reason to believe why it should be safe.

The other way around, Lua also avoids longjmping through C, by the way. (edit: But in this case it's because it is a "continuation", i.e. Lua wants/needs to jump back to where it jumped from. It's for coroutines.)

I find all this jumping across FFI boundaries pretty confusing. :face_with_spiral_eyes:

16

That indeed has an overhead which can be seen in Godbolt: the compiler cannot optimize out the checks to the global and local panic counts. Also, it has the side effect that if example() is itself called from a destructor during an unwinding panic, then it will abort even if callback() does not panic.

1 Like
17

I tend to go for the following solution(s):

  • In mmtkvdb, where I want to be fast, I will demand that the function used by the callback must not panic (guaranteed through an already existing unsafe trait, where implementors must guarantee that in future).
  • In sandkiste_lua, I will use @LegionMammal978's drop guard to abort the process when unwinding from a Rust closure (that has been invoked by Lua). Interestingly, I also need a guard for prohibiting unwinding when the Rust closure gets dropped (as this also happens through a callback). I considered to demand that all Rust closures must be UnwindSafe and catch the panic with catch_unwind and then report it to the Lua machine, but apparently Rust closures easily end up non-unwind-safe (Playground). This might become painful. So better to just abort on unwinding panic, I guess (which allows non-unwind-safe closures being passed to the API).
18

I wrote it a bit nicer (regarding its use):

fn abort_on_unwind<F: FnOnce() -> R, R>(func: F) -> R {
    struct AbortOnDrop;
    impl Drop for AbortOnDrop {
        fn drop(&mut self) {
            std::process::abort();
        }
    }
    let guard = AbortOnDrop;
    let result = func();
    std::mem::forget(guard);
    result
}

fn main() {
    let _ = std::panic::catch_unwind(|| {
        abort_on_unwind(|| {
            panic!();
        });
    });
    println!("Won't get here.")
}

(Playground)

I wonder if it would be nice to have such an abort_on_unwind in the standard library. It seems to be a pretty fundamental thing. I guess there also exists at least one crate offering this functionality (but I wouldn't want to pull in an extra dependency for that).

19

Here is the relevant section of the RFC: