1466118 - Audit/fix assertSameCompartment calls for same-compartment realms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

These should still hold, but some of them should be changed to assert same-realm instead, a stronger invariant. We should try to come up with a nice API for this.

Especially scripts will usually be same-realm so it would be nice if we could default to realm asserts for those somehow, but I haven't really thought about it  yet.

(Also, I've always wanted to s/assertSameCompartment/AssertSameCompartment/ - not trivial because js::AssertSameCompartment exists as friend API...)

Comment 1

[Jan de Mooij [:jandem]]

Thinking about it more, assertSameCompartment is already a misnomer, because for strings it checks the zone and for atoms/symbols it checks whether their mark bit is set in the current Zone, completely unrelated to compartments.

One option is to do something like this:

class ContextAsserts
{
  private:
    JSContext* cx_;

  public:
    explicit ContextAsserts(cx);

    ContextAsserts& checkRealm(realm);
    ContextAsserts& checkCompartment(compartment);
    ContextAsserts& checkZone(zone);
    ContextAsserts& checkAtom(atom or symbol);

    // These forward to one of the above.
    ContextAsserts& checkRealm(script);
    ContextAsserts& checkCompartment(object);
    ContextAsserts& checkRealm(object); // maybe

    // These forward to one of the above, depending on what's in them.
    ContextAsserts& check(string);
    ContextAsserts& check(Value);
    ContextAsserts& check(jsid);

    ... maybe some templated variants that accept multiple arguments, for convenience ...
};

Then for a typical SetProperty with obj/id/val arguments, we can do:

  ContextAsserts(cx).checkCompartment(obj).check(id, val);

Another option is to also add check(obj) and have it default to checkCompartment(obj), so then it's just:

  ContextAsserts(cx).check(obj, id, val);

Similarly, check(script) could default to checkRealm(script).

You could still override this default for objects/scripts etc by calling checkRealm/checkCompartment/checkZone explicitly.

Comment 2

[Jan de Mooij [:jandem]]

Any thoughts on the design in comment 1 and the options mentioned there? Just to avoid writing a patch for this and then rewriting it after review :)

Comment 3

[Luke Wagner [:luke]]

Sorry for the delayed feedback!  IIUC, the only case where there is any choice in whether to check same-realm vs. same-compartment is object, with same-compartment being the default/normal case.  Given that, what about:
  cx->check(anything1, anything2, ...)
and using variadics and overloading to do the right thing.  In the rare case of asserting same-object-realm, perhaps just MOZ_ASSERT(cx->realm() == obj->realm()) is better since it calls out the special case where we have, or rely on, a tighter invariant than the default one.

Comment 4

[Jan de Mooij [:jandem]]

That's nice and simple and close to what we already have, so WFM. Thanks!

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Use variadic templates for assertSameCompartment functions

This also removes the START_ASSERT_SAME_COMPARTMENT macro :)

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Replace releaseAssertSameCompartment with JSContext::releaseCheck

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Replace assertSameCompartmentDebugOnly with JSContext::debugOnlyCheck

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Replace assertSameCompartment with JSContext::check

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Replace assertSameCompartmentImpl with JSContext::checkImpl

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Rename CompartmentChecker to ContextChecks and support realm checks

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Avoid a TLS lookup for each compartment check

Just a small optimization.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Change compartment check to realm check for JSScript and AbstractFramePtr

Comment 13

[Jan de Mooij [:jandem]]

I verified compartment checks still work correctly with these patches (by adding cx->check(obj) and similar to Compartment::wrap).

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 9002715 [details] [diff] [review]
Part 1 - Use variadic templates for assertSameCompartment functions

Review of attachment 9002715 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/JSContext-inl.h
@@ +180,5 @@
> +    // depends on other objects not having been swept yet.
> +    if (JS::RuntimeHeapIsCollecting())
> +        return;
> +    CompartmentChecker c(cx);
> +    c.check(t1, argIndex);

I don't know how widely the release compartment asserts are used, but I wonder if there's any perf benefit to having a single RuntimeHeapIsCollecting() check and CompartmentChecker object created at the root of the recursive call and having the CompartmentChecker passed downward via reference.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 9002717 [details] [diff] [review]
Part 3 - Replace assertSameCompartmentDebugOnly with JSContext::debugOnlyCheck

Review of attachment 9002717 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/JSContext-inl.h
@@ +210,2 @@
>  template <class... Args> inline void
> +JSContext::debugOnlyCheck(const Args&... args)

Maybe worth a MOZ_ALWAYS_INLINE (for the benefit of Interpret() which, I expect due to crossing some limit, is pretty aggressive about not inlining things unless they are marked always-inline).

::: js/src/vm/JSContext.h
@@ +954,5 @@
>                             js::HandleObject incumbentGlobal);
>      void addUnhandledRejectedPromise(JSContext* cx, js::HandleObject promise);
>      void removeUnhandledRejectedPromise(JSContext* cx, js::HandleObject promise);
>  
> +    template <class... Args> inline void debugOnlyCheck(const Args&... args);

nit: IMO 'debugCheck()' would be basically as clear, shorter and symmetric with 'releaseCheck()', but up to you.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 9002717 [details] [diff] [review]
Part 3 - Replace assertSameCompartmentDebugOnly with JSContext::debugOnlyCheck

Review of attachment 9002717 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/JSContext.h
@@ +954,5 @@
>                             js::HandleObject incumbentGlobal);
>      void addUnhandledRejectedPromise(JSContext* cx, js::HandleObject promise);
>      void removeUnhandledRejectedPromise(JSContext* cx, js::HandleObject promise);
>  
> +    template <class... Args> inline void debugOnlyCheck(const Args&... args);

Actually, n/m, I see how this contrasts with 'check()' in the next patch.

Comment 17

[Luke Wagner [:luke]]

Comment on attachment 9002718 [details] [diff] [review]
Part 4 - Replace assertSameCompartment with JSContext::check

Review of attachment 9002718 [details] [diff] [review]:
-----------------------------------------------------------------

Much prettier.

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Part 9 - Some more cleanup

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bf5eb6fe16d
part 1 - Use variadic templates for assertSameCompartment functions. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb4cd7c449e
part 2 - Replace releaseAssertSameCompartment with JSContext::releaseCheck. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/224b09c2e661
part 3 - Replace assertSameCompartmentDebugOnly with JSContext::debugOnlyCheck. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/239b363ac50d
part 4 - Replace assertSameCompartment with JSContext::check. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/af49f7a464d5
part 5 - Replace assertSameCompartmentImpl with JSContext::checkImpl. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5cb8442b5d
part 6 - Rename CompartmentChecker to ContextChecks and support realm checks. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a263d3e088e
part 7 - Avoid a TLS lookup for each compartment check. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a85b3753ac
part 8 - Change compartment check to realm check for JSScript and AbstractFramePtr. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a88fb8b1a6
part 9 - Some more cleanup. r=luke

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/3bf5eb6fe16d
https://hg.mozilla.org/mozilla-central/rev/5cb4cd7c449e
https://hg.mozilla.org/mozilla-central/rev/224b09c2e661
https://hg.mozilla.org/mozilla-central/rev/239b363ac50d
https://hg.mozilla.org/mozilla-central/rev/af49f7a464d5
https://hg.mozilla.org/mozilla-central/rev/ff5cb8442b5d
https://hg.mozilla.org/mozilla-central/rev/1a263d3e088e
https://hg.mozilla.org/mozilla-central/rev/64a85b3753ac
https://hg.mozilla.org/mozilla-central/rev/87a88fb8b1a6