877287 - Assertion failure: !cx->compartment->activeAnalysis, at jsinterp.cpp:365 or Crash [@ js::ion::CodeGenerator::link] with Proxy

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision e58336e81395 (run with --ion-eager):


this.__proto__ = null;
var p = Proxy.create({
    has : function(id) {}
});
Object.prototype.__proto__ = p;
evaluate("var Uint16Array = 0;");

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Crash is a null-deref, but marking s-s because other bugs with this assertion have been s-s.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   132895:e1bca8b56470
user:        Jan de Mooij
date:        Fri May 24 14:03:31 2013 +0200
summary:     Bug 868431 - Disable Ion when Baseline is disabled, remove bailout-to-interpreter code. r=djvj

This iteration took 13.506 seconds to run.

Comment 4

[Andrew McCreight [:mccr8]]

Can you look into this, djvj, as Jandem is on PTO this week?  Thanks!

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: Fix CanEffectlesslyCallLookupGenericOnObject

CanEffectlesslyCallLookupGenericOnObject doesn't check for resolve hooks on object.

Comment 6

[Andrew McCreight [:mccr8]]

I'm going to assume the regressing bug is correct.

Comment 7

[Kannan Vijayan [:djvj]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> I'm going to assume the regressing bug is correct.

I'm trying to find the specific patch/bug that brought this in.

Comment 8

[Andrew McCreight [:mccr8]]

Thanks!

Comment 9

[Kannan Vijayan [:djvj]]

Best candidate for offending bug/commit:

changeset:   106691:d142f2087917
user:        Kannan Vijayan <[e-mail]>
date:        Wed Aug 08 18:51:24 2012 -0400
summary:     Bug 781214 - Ensure plain-ness of template object before looking up properties on it in jsop_initprop. (r=mjrosenb)

Comment 10

[Kannan Vijayan [:djvj]]

Try run looks good:
https://tbpl.mozilla.org/?tree=Try&rev=1b2f89d29acd

Comment 11

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7df36088f645

Comment 12

[Kannan Vijayan [:djvj]]

Setting in-testsuite? flag to remember to add this to the test suite after uplift to affected versions and suitable time has passed.

Comment 13

[Kannan Vijayan [:djvj]]

Can someone confirm that this resolves the issue in the affected releases?

Comment 14

[Joe Drew (not getting mail)]

Sadface:

Regression: Mozilla-Inbound-Non-PGO - V8 version 7 - Ubuntu HW 12.04 - 7.4% decrease
Regression: Mozilla-Inbound-Non-PGO - Kraken Benchmark - Ubuntu HW 12.04 - 5.01% increase
Regression: Mozilla-Inbound-Non-PGO - Dromaeo (CSS) - Ubuntu HW 12.04 - 5.2% decrease

Across the board, it seems. :(

Comment 15

[Boris Zbarsky [:bzbarsky]]

Kannan, is the performance regression expected?

Comment 16

[Ed Morley [:emorley]]

(In reply to Kannan Vijayan [:djvj] from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7df36088f645

Merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/7df36088f645

Leaving open for the perf regression.

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #14)
> Sadface:
> 
> Regression: Mozilla-Inbound-Non-PGO - V8 version 7 - Ubuntu HW 12.04 - 7.4%
> decrease
> Regression: Mozilla-Inbound-Non-PGO - Kraken Benchmark - Ubuntu HW 12.04 -
> 5.01% increase
> Regression: Mozilla-Inbound-Non-PGO - Dromaeo (CSS) - Ubuntu HW 12.04 - 5.2%
> decrease
> 
> Across the board, it seems. :(

The same is visible on all architectures including FFOS on awfy[1].

[1] http://www.arewefastyet.com/#machine=14

Comment 18

[Hannes Verschore [:h4writer]]

Just looked to JM, and JM didn't need this check. So I assume our check is wrong here. What is JM checking and IM isn't?

Comment 19

[Kannan Vijayan [:djvj]]

Check was too strict.  Updated patch coming now.

Comment 20

[Kannan Vijayan [:djvj]]

Attachment: Fix massive regression.

Fixes massive regression by passing the name being looked up to the checker function.  If the property is defined on a current object as a native property, checks for resolve hook on that object and its prototypes can be skipped.

Comment 21

[Hannes Verschore [:h4writer]]

Comment on attachment 759163 [details] [diff] [review]
Fix massive regression.

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

Nice, let's remove that regression ;)

Comment 22

[Tom S]

Comment on attachment 759163 [details] [diff] [review]
Fix massive regression.

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

::: js/src/ion/IonBuilder.cpp
@@ +5258,5 @@
>  {
>      while (obj) {
>          if (!obj->isNative())
>              return false;
> +        if (obj->nativeLookup(cx, id))

I think this check makes more sense after the next if.

Comment 23

[Kannan Vijayan [:djvj]]

Attachment: Fix regression, v2.

I looked through the call path of JSObject::lookupGeneric, and I can't find a single call to the lookupProperty hook.  There are calls to the lookupGeneric hook in the path, but not lookupProperty.

I'm changing the patch to reflect review comment as well as the above observation.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 759248 [details] [diff] [review]
Fix regression, v2.

You may want to add comments in nativeLookup and its callees to adjust this code if their code ever changes...

Comment 25

[Kannan Vijayan [:djvj]]

Boris: Added comments in relevant places.  Thanks for the suggestion.

Committed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f98048e391b7

Comment 26

[Tom S]

Probably doesn't happen, but something could have some of the other lookup stubs and not this one.

Comment 27

[Nicolas B. Pierron [:nbp]]

(In reply to Kannan Vijayan [:djvj] from comment #25)
> Boris: Added comments in relevant places.  Thanks for the suggestion.
> 
> Committed:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f98048e391b7

Fix acknowledged by AWFY on FFOS [1], other architectures are not yet updated.

[1] http://www.arewefastyet.com/#machine=14

Comment 28

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9ca690835a5e).

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f98048e391b7

Comment 31

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 33

[Kannan Vijayan [:djvj]]

Comment on attachment 758691 [details] [diff] [review]
Fix CanEffectlesslyCallLookupGenericOnObject

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 781214

User impact if declined:
Javascript correctness error - Property hooks may be called unnecessarily.

Testing completed (on m-c, etc.): 
Three days clean on m-c.

Risk to taking this patch (and alternatives if risky):
Low.

String or IDL/UUID changes made by this patch:
None.

Comment 34

[Kannan Vijayan [:djvj]]

> User impact if declined:
> Javascript correctness error - Property hooks may be called unnecessarily.

Also: security issue.  We should never execute JS code while Ion-compiling, but this bug allows for that to happen.

Comment 35

[Kannan Vijayan [:djvj]]

Comment on attachment 759248 [details] [diff] [review]
Fix regression, v2.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 781214

User impact if declined:
Regression introduced by above patch.  Either both of these patches should be approved, or neither should be approved.

Testing completed (on m-c, etc.): 
Three days clean on m-c.

Risk to taking this patch (and alternatives if risky):
Low.

String or IDL/UUID changes made by this patch:
None.

Comment 36

[Alex Keybl [:akeybl]]

What is the security rating of this bug? That's a prerequisite for tracking/landing.

Comment 37

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Alex Keybl [:akeybl] (out till 6/10) from comment #36)
> What is the security rating of this bug? That's a prerequisite for
> tracking/landing.

Setting needinfo.

Comment 38

[Kannan Vijayan [:djvj]]

I have to punt this question to bhackett.  Brian, what are the security implications of running JS code inadvertantly when doing Ion compilation?  Is it serious?

Comment 39

[Brian Hackett [Laid off!]]

It's pretty bad if code runs during compilation --- mainly, no invalidation will occur until the original compilation finishes, so if the JS code executes any jitcode it could induce a crash or exploit by changing the jitcode's type assumptions before calling it.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d3302ae3555
https://hg.mozilla.org/releases/mozilla-aurora/rev/8fb828aae198

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Backed out for jit-test failures.
https://hg.mozilla.org/releases/mozilla-aurora/rev/40c9d5acd1b7

https://tbpl.mozilla.org/php/getParsedLog.php?id=24080191&tree=Mozilla-Aurora

Comment 42

[Alex Keybl [:akeybl]]

Our final beta build is on Monday. Is this safe to land given the recent failures? Makes me a bit more concerned.

Comment 43

[Kannan Vijayan [:djvj]]

(In reply to Alex Keybl [:akeybl] from comment #42)
> Our final beta build is on Monday. Is this safe to land given the recent
> failures? Makes me a bit more concerned.

No, it's not.  Bug 882834 needs to land first.  But I can't land that yet because it causes mochitest and browser chrome test failures.

I thought the beta build got frozen next week (June 24)?  I was hoping to get these in this week, before that window.

Comment 44

[Boris Zbarsky [:bzbarsky]]

June 24 is when the release happens.

Comment 45

[Alex Keybl [:akeybl]]

Comment on attachment 758691 [details] [diff] [review]
Fix CanEffectlesslyCallLookupGenericOnObject

Too late for our final beta of FF22.

Comment 46

[Alex Keybl [:akeybl]]

djvj is going to make the case on the security list to take this in FF22 once he's completed the regression fixes.

Comment 47

[Kannan Vijayan [:djvj]]

Talked with bhackett with this for a while and he suggested a much more minor, safer, simpler fix that would address the sec-crit issues for release.

The main issue here is running JS code while ion-compiling JS code.  The solution he proposed was just to take the debug-only ASSERTs that check for this, and turn them into runtime soft checks that throw exceptions.

I'm jit-testing a patch for this right now, but attaching and requesting beta approval for it in the meantime.

Comment 48

[Kannan Vijayan [:djvj]]

Attachment: Make the activeAnalysis check into a soft check.

This throws "undefined" as an exception if we encounter the activeAnalysis flag on the compartment when trying to interpret.

Comment 49

[Brian Hackett [Laid off!]]

Comment on attachment 763673 [details] [diff] [review]
Make the activeAnalysis check into a soft check.

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

::: js/src/jsinterp.cpp
@@ +394,5 @@
>  {
>      JS_ASSERT(args.length() <= StackSpace::ARGS_LENGTH_MAX);
>      JS_ASSERT(!cx->compartment->activeAnalysis);
>  
> +    // Even on optimized builds, never allow execution of JS code when compiling.

rm the 'even on optimized builds' bit.

@@ +396,5 @@
>      JS_ASSERT(!cx->compartment->activeAnalysis);
>  
> +    // Even on optimized builds, never allow execution of JS code when compiling.
> +    if (cx->compartment->activeAnalysis) {
> +        cx->setPendingException(UndefinedValue());

I think it would be better to throw a real error here.  Something like this should be fine:

JS_ReportError(cx, "Can't run scripts during analysis");

@@ +1050,5 @@
> +    // Even on optimized builds, never allow execution of JS code when compiling.
> +    if (cx->compartment->activeAnalysis) {
> +        cx->setPendingException(UndefinedValue());
> +        return Interpret_Error;
> +    }

Ditto the above (or consolidate these into a helper method).

Comment 50

[Kannan Vijayan [:djvj]]

Attachment: UPDATED: Make the activeAnalysis check into a soft check.

With comments addressed above.

Comment 51

[Kannan Vijayan [:djvj]]

Comment on attachment 763682 [details] [diff] [review]
UPDATED: Make the activeAnalysis check into a soft check.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 781214

User impact if declined: 
Exploitable security issue, controlled native code execution.
This patch disables all code execution while compilation is occurring.  This not only prevents the bug mentioned above, but also prevents this entire class of bugs.  If some other exploitable path is found which allows an attacker to run JS code from within the browser, this patch will ensure that it throws an exception instead of running the exploit.

Testing completed (on m-c, etc.): 
jit-tests on x86-32 and x86-64.

Risk to taking this patch (and alternatives if risky):
Very low.
 
String or IDL/UUID changes made by this patch:
None.

Comment 52

[Alex Keybl [:akeybl]]

Comment on attachment 763682 [details] [diff] [review]
UPDATED: Make the activeAnalysis check into a soft check.

Thanks for coming up with a very low risk fix on such a short timeline!

Comment 53

[Kannan Vijayan [:djvj]]

Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ee9cc4f09fb6

Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/9f0d4fe5db55