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

More FFI #29

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

More FFI #29

wants to merge 5 commits into from

Conversation

polazarus
Copy link
Contributor

@polazarus polazarus commented Jan 29, 2020

Some typos
Add a rule about not passing stack pointers to foreign language
Improve and fix C API callback-based example

make it thread-safe and also resistant to compiler optimization
(setting the tag to invalid may be optimized away)
@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Jan 30, 2020

I have mixed feelings about this: to me not trusting the FFI language to overflow w.r.t to the given pointer is pushing things too far. Or to put it in another way: why should it be acceptable to send heap pointers to FFI if there is a risk of overflow from FFI? Although harder to exploit, heap overflows are also exploitable.

At that level of mistrust FFI with shared memory should not be used altogether, and instead messages and some form of sandboxing ought to be used.

I mean, an additional protection against overflows can be a nice thing to have (I just don't think that "not giving stack pointers to FFI" is the way to go), for instance, here is a a prototype:

type Canary = [u8; 4];

#[repr(C)] // to ensure ordering
pub
struct Canaried<T> {
    inner: T,
    canary: Canary,
}

impl<T> From<T> for Canaried<T> {
    fn from (inner: T) -> Self
    {
        Self { inner, canary: ::rand::random() }
    }
}
impl<T> Into<T> for Canaried<T> { ... } //

impl<T> Canaried<T> {
    pub
    fn with_mut_ptr<R> (self: &'_ mut Self, f: impl FnOnce(*mut T) -> R) -> R
    {
        use ::std::panic;

        let at_canary: *mut Canary = &mut self.canary;
        let canary = unsafe { at_canary.read_volatile() };
        let ret = panic::catch_unwind(panic::AssertUnwindSafe(|| f(&mut self.inner)));
        if canary != unsafe { at_canary.read_volatile() } {
            ::std::process::abort();
        }
        unsafe {
            at_canary.write_volatile(::rand::random());
        }
        ret.unwrap_or_else(|err| panic::resume_unwind(err))
    }
}

@polazarus
Copy link
Contributor Author

Yeah it's questionable. I think at the very least it should be downgraded to a rec rather that a rule.

I made it to be consistent with a comment -I made a long time ago- about not providing stack pointers to C in the example.

The point of the example is in fact not to ensure stack smashing or overflow but rather how to ensure drop is called.

For the overflow problem I like your example much better!

I think I will revert the new rule, simplify the example and reopen an issue to discuss that...

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

Successfully merging this pull request may close these issues.

2 participants