Return an error instead of aborting on panics in C API

[seanmonstar]

The C API will currently abort after catching a panic, for safety reasons. However, we shouldn't abort, but rather catch it and return an error code.

[seanmonstar]

A tricky part to this is that the various FFI functions have different return types, so we can't just always return some HYPERE_INTERNAL_BUG code.

One way I considered to do this was to have some trait implemented for return types. However, it's slightly scary to me to execute some indirect function outside of the catch_unwind, since it could technically panic also. We could just promise to be careful not to do anything too fancy in them... Another issue is that sometimes the type returned has different meanings per function, and returning -1 or 0 might make sense in different functions.

[Kixunil]

Wouldn't unsafe trait with contract requiring absence of panic solve this?

[seanmonstar]

The trait would need to be unsafe, yes. However, there's a difference between "literally can't happen" and "I promise I won't". 🤷

[Kixunil]

Here's a hack to at least force abort in case it goes wrong:

/// Calls the closure and guarantees to abort if the closure panics
pub fn force_abort_on_panic<R, F: FnOnce() ->R>(fun: F) -> R {
    let mut output = MaybeUninit::uninit();
    // drop impl of RunOnDrop is guaranteed to write the result into output
    // and any panic inside it would turn to abort, so uninitialized value can't be observed
    unsafe {
        drop(RunOnDrop(&mut output, ManuallyDrop::new(fun)));
        output.assume_init()
    }
}

struct RunOnDrop<'a, R, F: FnOnce() ->R>(&'a mut MaybeUninit<R>, ManuallyDrop<F>);

impl<'a, R, F: FnOnce() ->R> Drop for RunOnDrop<'a, R, F> {
    fn drop(&mut self) {
        // The struct is private, and the caller guarantees to initialize it
        // panic in drop always causes abort
        unsafe {
            *self.1 = MaybeUninit::new(self.1.take()());
        }
    }
}

Didn't try to compile it but I guess you get the idea.

[nylanderdev]

Seeing as we might want different error handlers and values for different functions, I suggest we create a new ffi_fn! macro rule instead of using a trait. Something that will allows us to write ffi functions like this, with default values or closures following the function body:

ffi_fn! {
    /// Create a new "empty" body.
    ///
    /// If not configured, this body acts as an empty payload.
    fn hyper_body_new() -> *mut hyper_body {
        Box::into_raw(Box::new(hyper_body(Body::empty())))
    } ? std::ptr::null_mut() // Error value or closure here
}

There are no guarantees that whatever error closure is used won't panic, but at least it will be clearly visible by the function definition. As such, it adds clarity. We might need to repeat ourselves when using commonly used error handling closures, but any verbosity could be minimized by making such closures into constants, like so:

const DANGER_HANDLER: fn(ErrorType) -> usize = |e| { /* ... */ };

ffi_fn! {
    fn danger_func1() -> usize  {
        // ...
    } ? DANGER_HANDLER
}

ffi_fn! {
    fn danger_func2() -> usize  {
        // ...
    } ? DANGER_HANDLER
}

For error values (i.e. non-closures), the rule could look something like this:

($(#[$doc:meta])* fn $name:ident($($arg:ident: $arg_ty:ty),*) -> $ret:ty $body:block ? $err:expr) => {
    $(#[$doc])*
    #[no_mangle]
    pub extern fn $name($($arg: $arg_ty),*) -> $ret {
        use std::panic::{self, AssertUnwindSafe};

        match panic::catch_unwind(AssertUnwindSafe(move || $body)) {
            Ok(v) => v,
            Err(_) => {
                eprintln!("panic unwind caught, returning default");
                $err
            }
        }
    }
};

[seanmonstar]

@nylanderdev That actually sounds very good to me, thanks for the write-up! The trait way seemed to easy to return a value that is "bad" in one context and "good" in another.

Would you want to try adding that? At least to a few functions, we don't have to update all the functions in a single go...

[nylanderdev]

@seanmonstar I've never actually contributed to or used hyper, so I'm not entirely sure which default values would be appropriate, but I could add some nulls for some (all?) pointer-returning ones, to set an example. If that's alright I'll make a PR in the morning and someone can review it extra carefully just to be safe.

[seanmonstar]

Yea, that's fine! Even just having the updated macro and an example of how it works, it'd be easier to audit each function and define a proper "error value" in later PRs. I assume any function that doesn't have it set will just continue to abort for now, and once we've got them all updated, we could remove that default to force all new functions to pick an error value.

[nylanderdev]

Now we can easily catch panics on functions that don't return anything. A different issue is functions that return numbers. In a lot of cases, I imagine -1 would be a good value, however most functions seem to return unsigned values, so how do we want to handle that? We could retype some functions, such as those returning buffer length, so that we can return negative numbers too. It's just a matter of making sure we can still return as many different lengths as we could before and rewriting any header-files, lest we return some really big unsized integer by accident and cause massive memory access violations.

[nylanderdev]

I suppose for hyper_buf_len we could actually just return 0, but point to null in hyper_buf_bytes, like we're already doing. Not all number valued functions might be as easy to fix though.