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

Support an alternative calling convention which can take references #52

Merged
merged 3 commits into from
Sep 11, 2020

Conversation

udoprog
Copy link
Collaborator

@udoprog udoprog commented Sep 11, 2020

This moves the existing Vm::call function to be Vm::execute since it returns an execution and replaces the existing call with call and call_async. And allow call and call_async to take references.

Complete test case showcasing the functionality:

#[derive(Debug, Default, Any)]
struct Foo {
    value: i64,
}

impl Foo {
    fn add_assign(&mut self, value: i64) {
        self.value += value;
    }
}

#[test]
fn vm_test_references() {
    let mut module = Module::new(Item::empty());
    module.ty(&["Foo"]).build::<Foo>().unwrap();
    module
        .inst_fn(runestick::ADD_ASSIGN, Foo::add_assign)
        .unwrap();

    let mut context = Context::with_default_modules().unwrap();
    context.install(&module).unwrap();

    let context = Arc::new(context);

    let mut sources = Sources::new();

    sources.insert_default(Source::new(
        "test",
        r#"
        fn main(number) {
            number += 1;
        }
        "#,
    ));

    let unit = rune::load_sources(
        &context,
        &Options::default(),
        &mut sources,
        &mut Warnings::disabled(),
    )
    .unwrap();

    let vm = Vm::new(context, Arc::new(unit));

    let mut foo = Foo::default();
    assert_eq!(foo.value, 0);
    let output = vm.call(&["main"], (&mut foo,)).unwrap();
    assert_eq!(foo.value, 1);
    assert!(matches!(output, Value::Unit));
}

These functions take a new trait as arguments called GuardedArgs, which has a new unsafe function. Calling this function is able to encode arguments to the stack by wrapping them in Any which now has an expanded vtable to support wrapping references.

Now the contract of unsafe_into_stack says the following:

/// Encode arguments onto a stack.
///
/// # Safety
///
/// This is implemented for and allows encoding references on the stack.
/// The returned guard must be dropped before any used references are
/// invalidated.
  • So what happens is that references can be converted into Shared<Any> + a guard, and once this guard is dropped the Shared<Any> will be "taken", preventing it from being used anywhere else every again.
  • The drop implementation for an Any created from a point prevents the type from being de-allocated since it's actually storing a raw pointer now.

A major caveat however, is that we have to introduce a soundness panic. If a program right now were to hold ownership of a Shared<Any> containing a pointer, at the point the guard is dropped it will panic because it can't take exclusive ownership of the type. If we didn't panic, after the function call was completed other parts of the system might be holding onto the reference (through OwnedRef<T> and OwnedMut<T>).

This can be shown using this test case:

Edit test case has been updated to showcase that we now instead error if we try to take an owned ref:

#[test]
fn vm_test_references_error() {
    fn take_it(this: Shared<AnyObj>) -> Result<(), VmError> {
        // NB: this will error, since this is a reference.
        let _ = this.owned_ref()?;
        Ok(())
    }

    let mut module = Module::new(Item::empty());
    module.function(&["take_it"], take_it).unwrap();

    let mut context = Context::with_default_modules().unwrap();
    context.install(&module).unwrap();

    let context = Arc::new(context);

    let mut sources = Sources::new();

    sources.insert_default(Source::new(
        "test",
        r#"fn main(number) { take_it(number) }"#,
    ));

    let unit = rune::load_sources(
        &context,
        &Options::default(),
        &mut sources,
        &mut Warnings::disabled(),
    )
    .unwrap();

    let vm = Vm::new(context, Arc::new(unit));

    let mut foo = Foo::default();
    assert_eq!(foo.value, 0);

    // this should error, because we're returning an owned ref`.
    assert!(vm.call(&["main"], (&mut foo,)).is_err());
}

Todo

  • [*] Preventing taking owned refs/muts if a Shared<Any> if it has been created from a reference to avoid the soundness panic.

@udoprog udoprog added the enhancement New feature or request label Sep 11, 2020
@udoprog
Copy link
Collaborator Author

udoprog commented Sep 11, 2020

Hiding Shared::owned_mut and Shared::owned_ref should make it impossible to trigger the soundness panic externally, since there's no way to hold onto a borrow or ref.

@udoprog
Copy link
Collaborator Author

udoprog commented Sep 11, 2020

So there's three more improvements that came out of this:

  • We no longer make use of std::any::Any for deciding things that can be punted into an AnyObj. This prevents us from punting OwnedMut<T> into an AnyObj and by extension preventing us from constructing a value out of it (because it doesn't implement runestick::Any).
  • Other internal types (like String) can no longer be punted into Value::Any due to the above, because they do not implement runestick::Any. This could otherwise cause interoperability problem since they are exclusively typechecked depending on which Value variant they have (Value::String for strings).
  • We now smuggle one more bit of information in Access to prevent OwnedMut<T> and OwnedRef<T> from being publicly created from references without causing an error. The soundness panic is still there to guarantee safety, but it should not be possible to trigger in safe code.

@udoprog udoprog merged commit 57206ef into master Sep 11, 2020
@udoprog udoprog deleted the calls-with-references branch September 11, 2020 16:32
@udoprog udoprog added the changelog Issue has been added to the changelog label Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Issue has been added to the changelog enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant