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

Audit context guard soundness #26

Open
nox opened this issue Nov 11, 2018 · 2 comments
Open

Audit context guard soundness #26

nox opened this issue Nov 11, 2018 · 2 comments

Comments

@nox
Copy link

nox commented Nov 11, 2018

How is the soundness of Context::make_current guaranteed? For example, what happens if user calls it for two different contexts and then drops the guard in reverse order?

@darfink
Copy link
Owner

darfink commented Jun 14, 2019

Sorry for the delay.

That's a valid observation. Since only one context may be active at a time (per thread), a probable solution would be to use a thread-local stack instead of each context tracking their predecessor.

EDIT: Although that does not guarantee soundness either, but I believe I have a different solution.

@darfink
Copy link
Owner

darfink commented Jun 15, 2019

I've pondered more about a potential solution and each have their own pros and cons

  • Use a stack-based context logic and associate each guard with an ID:

    Context ID
    Context1 1234
    Context2 5234
    Context3 1236

    Whenever a guard is dropped, it checks whether it is currently active, and if so pops itself from the
    thread-local stack and activates its predecessor. Otherwise, if not currently active, it just removes
    itself from the stack by its ID. This would also require a reentrant mutex for each runtime (unless this validation is deferred to ChakraCore itself).

    Pros Cons
    Simple API Requires reentrant mutex for the runtime
    - Introduces guard inconsistencies*

    *A guard may not ensure that the context it references is actually active.

    let guard1 = context1.make_current()?;
    let guard2 = context2.make_current()?;
    
    // This property would still be associated with `context2`
    let property = Property::new(&guard1, "foo"); 
  • Take advantage of the runtime being Send + Sync, and expose a &mut self for activating a context (where only one context can be activate at a time).

    This approach would rely on the type system to ensure that the runtime is only used from a single thread at a time, whilst also preventing guard inconsistencies.

    Pros Cons
    No guard inconsistencies Arguably a more cumbersome API
    No reentrant mutex -
    let guard1 = runtime.activate_context(&context1);
    
    // The following would not compile since `runtime` is already mutably borrowed
    let guard2 = runtime.activate_context(&context2);

    The downside is of course that the runtime must be passed around whenever a context is used.

    NOTE: An extension to this method may also be to allow nested guards:

    let guard2 = guard1.switch(&context2);
    
    // The following would not compile since `guard1` is mutably borrowed during the `switch`
    let property = Property::new(&guard1, "foo"); 

It may also be worth mentioning that none of these options would solve scenarios where values are intermixed from different contexts:

let object1 = Object::new(&guard1);
let guard2 = guard1.switch(&context2);

// `object1` is associated with `context1`, whilst a value from `context2` is assigned.
object1.set(&guard2, &property, Number::new(&guard2, 10));

This may be solved by each value tracking its own associated context, but I'm not sure it's worth the overhead.

I'd love some input if anyone is interested or has any ideas. I will continue to ponder but I believe using runtime + &mut may be the cleanest and most robust solution.

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

No branches or pull requests

2 participants