Rhai is unsound (use after free)

[devzf]

Consider the following example:

fn main() {
    let global_name: std::rc::Rc<std::cell::RefCell<&'static str>> = Default::default();
    let global_name2 = global_name.clone();
    {
        let mut engine = rhai::Engine::new();

        engine.register_fn("hello", move |name: &'static str| {
            *global_name.borrow_mut() = name;
        });

        engine.eval::<()>(r#"hello("some name")"#).unwrap();
    }

    println!("{:?}", &*global_name2.borrow());
}

rhai v1.19.0

[schungx]

I'm outside right now but on the surface it looks legit... Why do you say therr is OB?

[schungx]

Ah I see it now. The 'static is throwing it off...

Sneaky...

I must have a way to distinguish between &str and &'static str...

[schungx]

So far I have not been able to find a way to avoid the user passing in &'static str... short of disallowing &str altogether which will seriously break code.

That's because, to the Rust compiler, lifetimes do not form part of a type's ID. Therefore, it merrily thinks that &'static str and &str are the same type.

[Cerber-Ursi]

Do you have any reason to allow registered functions take any non-'static value at all? It seems that this use-after-free is possible with any borrowed value, not just &str.

[schungx]

Well, non-'static lifetimes are OK because Rhai doesn't return any lifetime. That's why passing in a normal &str is absolutely OK because within the Rust function there is nothing you can do to abuse that reference with a lifetime -- you cannot store it in anything provided by Rhai because all the functions are + 'static.

Therefore, I'm free to assume that, as long as the reference passed into the function outlives the function call, everything is OK.

Except that if that reference is 'static, then suddenly it satisfies the Rhai function requirements and you can now store those references inside Rhai variables. That's what caused the error.

It is only with &str because that's specially treated by Rhai for special handling... it gets converted to a reference inside an ImmutableString for convenience. Other references I suppose are simply never passed anything because there is no data in Rhai that is stored with references (except for the special treatment for ImmutableString). For any other type the function is simply never called.

[udoprog]

FYI, this seems like the same issue in rune-rs/rune#601

I'm mentioning it here so you can consider the same remedy which is to bind local references using HRTBs.

[schungx]

Thanks @udoprog for the tip. I have looked into the PR and I'm not sure what you mean by HRTB since I don't seem to find them in the code. It seems to be disallowing reference parameters altogether...

Any pointer is appreciated!

Also, great job on Rune which seems to keep getting better!

[udoprog]

This is the relevant commit: https://github.com/rune-rs/rune/pull/601/commits/ce81e690dc7e2efaf852408218f55fb4337e0ada

Here's a playground which might make it clearer: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0edc3aa181f03772f7c48a9c41985630

Of particular interest is an implementation like where T: Fn(&A), A: ?Sized + FromValueRef2, which if FromValueRef2 is implemented for u32 is not satisfied for &'static u32. This is the HRTB.

Note that this causes an issue in the number of implementations that have to be present, since it's a permutation between owned, & references and &mut references. This is dealt with a generated macro here: https://github.com/rune-rs/rune/blob/b8f49258173ab4ece091ebfe023141f07583a2ef/crates/rune/src/function/macros.rs