Using `unreachable_unchecked` based on FFI guarantees

According to the docs, unreachable_unchecked should be used "only when you can prove that the code will never call it". It's clear what this means when all the behavior is controlled by your code. Relying on complex behavior of other libraries is more tricky, and it seems to me even more tricky in case your relying on the behavior of code sitting across an FFI border.

In the case that brought me to this question, we have a method being passed as a callback to a C function. That C function "should not" be calling our callback in the case some condition doesn't hold, so by the definition above, it should be valid to use unreachable_unchecked on the else arm of checking that condition. It's also easy to verify by reading the C code, that it indeed shouldn't be calling the callback in that case. However, this relies on the correctness of the C code, assuming that there wasn't any UB in that vendor code that causes it do something unexpected etc. This means, that using unreachable_unchecked couples our UB with that of non-Rust code, which is unfortunate.

So the question is, is it reasonable to use in this case, or should more caution be exerted?

Toy code (original code and discussion here: Changed WrapperState into an enum by amosonn · Pull Request #1 · Michael-F-Bryan/chmlib · GitHub):

Our code:

unsafe extern "C" fn decrement_positive(num: c_int) -> c_int {
    if num > 0 {
        num - 1
    } else {
        // This callback should only be called with positive numbers
        unreachable_unchecked()
    }
}

type CALLBACK = Option<unsafe extern "C" fn(num: c_int) -> c_int>;

extern "C" {
    fn run_with_positive(
        num: c_int,
        c: CALLBACK,
    ) -> c_int;
}

pub fn do_it(num: i32) -> Option<i32> {
    let ret_val = unsafe { run_with_positive(num as c_int, decrement_positive) };
    match ret_val {
        -1 => None,
        val => Some(val as i32),
    }
}

#[test]
fn it_works() {
    assert_eq!(do_it(5), Some(4));
    assert_eq!(do_it(-5), None);
}

"Vendor" code:

typedef int (*CALLBACK)(int num);

int run_with_positive(int num, CALLBACK c) {
    if (num <= 0) {
        return -1;
    }

    return c(num);
}

It's fine to use it if you document to the caller that violating your requirement is UB. It's not "should not", it's "MUST NEVER". It's similar to requiring non-NULL arguments, or even valid pointers in general.

4 Likes

Fun point is that panic unwinding across ffi boundaries is UB so unreachable_unchecked() here is equally unsafe than unreachable!(), though the latter prints some error message before triggering UB.

Personally expressing UB point via document seems acceptable, though not a best choice, because every other C projects already doing so at best. That's the C way. And this is why we prefer to code in Rust! :stuck_out_tongue:

3 Likes

For this use case it's actually okay to use unreachable!() because this unreachable_unchecked() is being called inside std::panic::catch_unwind(). One of the big reasons this came up is because we're using an enum with variants that are a closure to execute, a Box<dyn Any> if a previous call panicked, or a Box<dyn Error> if the closure returned an error.

The reason we were thinking of using unreachable_unchecked() is because this function should never be called if previous calls errored or panicked, so the code looks like if let State::Closure(cb) = state { ... } else { unreachable_unchecked() }. We can verify this by looking at the C code which invokes the function and making sure the function is only accessible by going through a strict public interface.

But using unreachable_unchecked() still feels a bit sketchy and UB-inducing...

1 Like

I don't know if this changes your answer, but the control flow I am asking about is the other way around. Our library code is both the one with the callback and the one calling the extern "C" func, and we wrap this functionality for the user. So the code which has to uphold the requirement is not the user of our library, rather the C code which is maintained elsewhere, and which we are attempting to wrap in a safe way.

I've updated the example code in the original post to make this more clear.

You should treat unreachable_unchecked as an optimization. Is the section you want to use it a performance-critical one? If yes, you may want to do it, but usually the CPU is good enough at optimizing if-statements, that always end up in the same branch. This kind of CPU optimization is called branch prediction and makes the cost of the if-statement you're dealing with almost certainly negligable.

1 Like

Note that, taking a simpler case,

#[no_mangle] pub unsafe extern "C"
fn read (p: *const c_int) -> c_int
{
    let my_ref =
        p   .as_ref()
            .unwrap_or_else(|| ::std::hint::unreachable_unchecked())
    ;
    *my_ref
}

is strictly equivalent to:

#[no_mangle] pub unsafe extern "C"
fn read (p: *const c_int) -> c_int
{
    let my_ref = &*p;
    *my_ref;
}

That is, both functions behave exactly the same.


When a function can trigger UB if it requires some invariant to be upheld when called, then that function must be marked unsafe. This way, any function calling it needs to wrap the call in an unsafe block, hence expressing that it takes responsibility from the required invariant(s) not being upheld.

And as long as there is only Rust code along the chain, this is indeed the case.

In your situation you are feeding a callback and params to C, for it to call the function with the given params. Since the C function is able to call the callback (i.e., because it is an unsafe fn function pointer), then calling that C function must be considered unsafe too (which it will most probably be anyways since it is a foreign function).

So when your other Rust code calls that C function, it needs to use an unsafe block to do it, and it is there that your code must ensure the invariant is upheld (EDIT: it can "ensure" so by relying on the C code guarantees, in hindsight I did not phrase it clearly enough).

That's not always possible. For instance, in the original case for this question, the C code actually calls the callback multiple times. We guarantee of course that the invariant holds in the beginning, and we know it's not going to call it if it ever stops holding, but we can't guarantee that ourselves, because we don't get control during that time - unless of course we check it inside the callback, instead of using unreachable_unchecked, which is exactly the question.

Yeah, I think by asking this question, you're kind of missing the forest for the trees.

If you were providing a pure Rust library, would you be allowed to have a function that called unreachable_unchecked based on an invariant that you can't guarantee? The answer, of course, is "if the function is marked unsafe, then yes." In a pure Rust library, if a function can invoked UB, then you are supposed to mark it unsafe and document the invariant that it relies upon.

Introducing FFI doesn't change anything important. If you mark your callback unsafe extern "C", and document the invariant that needs to be upheld, then you have now created a "proof obligation." You are responsible for proving "if X, then no UB," and your client is responsible for proving "X", and the combination of these two proofs is that the program never invokes UB.

The C header won't have the word "unsafe" in it, but that's because all functions are unsafe in C.

1 Like

This. If the C library guarantees / promises this, then your own unsafe calling the library which calls your unsafe fn is "proven" correct: the responsibility for correctness now lies within the C library and its guarantees.

Taking the OP code example, this is how I'd write it (I'll only be adding "safety" annotations):

/// # Safety
///
///   - `num` must be greater than `0`.
unsafe extern "C" fn decrement_positive(num: c_int) -> c_int {
    if num > 0 {
        num - 1
    } else {
        // This callback should only be called with positive numbers
        unreachable_unchecked()
    }
}

type UnsafeCallback = Option<unsafe extern "C" fn(num: c_int) -> c_int>;

extern "C" {
    /// # Safety
    ///
    ///   - `unsafe_cb` must be either NULL or safe to call when `num > 0`
    ///     (since that's what is checked by this function)
    fn run_with_positive(
        num: c_int,
        unsafe_cb: UnsafeCallback,
    ) -> c_int;
}

pub fn do_it(num: i32) -> Option<i32> {
    let ret_val = unsafe {
        // Safety: decrement_positive is indeed safe to call when its input is positive.
        run_with_positive(num as c_int, Some(decrement_positive))
    };
    match ret_val {
        -1 => None,
        val => Some(val as i32),
    }
}

Now, in practice, there are two things to be aware of:

  • is performance critical enough (e.g., hot path function) for the micro-optimization of skipping the check to be worth it? This ought to be benchmarked.

    • you can try having the unreachable part become:

      if cfg!(do_not_trust_vendor_guarantees) {
          unreachable!("Vendor code violated the safety guarantees!");
      } else {
          unsafe { ::std::hint::unreachable_unchecked(); }
      }
      
      [features]
      do_not_trust_vendor_guarantees = []
      
      • The reason behind the "negative" definition of the feature is that features are additive, so if one crate depends on it and enables the feature, and another one does not enable it, then the feature remains enabled. By making the definition negative, the check will in that case remain activated, which is the most conservative choice.

      and then run some micro-benchmarks with and without
      --features do_not_trust_vendor_guarantees

  • since I guess you have control or at least read access over the vendor source code, you can ensure (or at least check) that it does indeed perform the required check before calling the callback, as in the OP example. If that's the case, then all is good :white_check_mark:

    • but for others stumbling upon this thread who are in a similar situation but where they have no access to the source code (e.g., imagine a shared / static library with just a header file), they should know that relying too blindly on an external library may be too dangerous.

As you can see in the code I've shown, all the subtlety lies in the fact that decrement_positive is unsafe (since some inputs can trigger UB), but with a very specific narrow contract, that the C code can fulfill.

So, when requiring to fulfill some invariants that other unsafe code (or FFI code) needs to rely on (for memory safety), then using an unsafe trait is usually the way to go:

use ::libc::c_int;

/// # Safety
///
///   - `call()` mut be sound / safe to call when `input > 0`.
unsafe trait SafeToCallOnPositive {
    unsafe extern "C"
    fn call (input: c_int)
      -> c_int
    ;
}

#[allow(bad_style)]
struct decrement_positive;
unsafe impl SafeToCallOnPositive for decrement_positive {
    unsafe extern "C"
    fn call (num: c_int)
      -> c_int
    {
        if num > 0 {
            num - 1
        } else {
            // This callback should only be called with positive numbers
            ::std::hint::unreachable_unchecked()
        }
    }
}

fn run_with_positive<Function : SafeToCallOnPositive> (
    num: c_int,
    _: Function,
) -> c_int
{
    extern "C" {
        /// Safety: cb must be NULL or safe to call on a positive input.
        fn run_with_positive (
            num: c_int,
            cb: Option<unsafe extern "C" fn (c_int) -> c_int>,
        ) -> c_int;
    }
    unsafe {
        // Safety: `SafeToCallOnPositive` contract guarantees the preconditions
        // of the extern function are met.
        run_with_positive(num, Some(Function::call))
    }
}

pub fn do_it(num: i32) -> Option<i32> {
    let ret_val =
        run_with_positive(num as c_int, decrement_positive)
    ;
    match ret_val {
        -1 => None,
        val => Some(val as i32),
    }
}

Thanks for the detailed explanation! The unsafe trait model seems like a good way to document the required invariants, so they don't "slip away" later.

1 Like

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.