DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
504 stars 63 forks source link

Unsound: `Class::<T>::from_js_ref()` returns a reference to `T` which can outlife its containing object. #167

Closed DelSkayn closed 1 year ago

DelSkayn commented 1 year ago

Class::<T>::from_js_ref() has the following signature:

fn from_js_ref<'js>(ctx: Ctx<'js>, value: Value<'js>) -> Result<&'js Self>

The returned reference has the lifetime 'js however the actual lifetime depends on the lifetime of Value<'js>. If this value is the only reference to T, T is immediately dropped inside the function and then the reference is returned. Example:

use rquickjs::{bind, class::ClassDef, Class, Context, Runtime};

use test::Test as TestCls;

#[bind(object, public)]
#[quickjs(bare)]
mod test {
    pub struct Test(pub(crate) String);
}

fn main() {
    let rt = Runtime::new().unwrap();
    let ctx = Context::full(&rt).unwrap();

    ctx.with(|ctx| {
        Class::<TestCls>::register(ctx).unwrap();
        let val = Class::instance(ctx, TestCls("hello".to_string()))
            .unwrap()
            .into_value();
        // Val is moved into from_js_ref and then dropped in the function implementation.
        // This also drops `Test`
        let escape = <TestCls as ClassDef>::from_js_ref(ctx, val).unwrap();
        // but `escape` has lifetime 'js so it is still valid.
        println!("{}", escape.0);
    })
}

A more correct signature would be:

fn from_js_ref<'a, 'js>(ctx: Ctx<'js>, value: &'a Value<'js>) -> Result<&'a Self>
DelSkayn commented 1 year ago

This causes some problems with who classes from rust are implemented. Methods use the fact that class implement FromJs for &T where T: ClassDef which is implemented using from_js_ref. However it is unsound to return a reference from FromJs as it takes Value by value and not reference.

I will have to look at some redesign with regards to class methods.