Closed Bug 877287 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- wontfix
firefox22 + fixed
firefox23 + fixed
firefox24 + verified
firefox-esr17 --- unaffected
b2g18 --- wontfix

People

(Reporter: decoder, Assigned: djvj)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main22+])

Crash Data

Attachments

(5 files, 1 obsolete file)

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;");
Crash is a null-deref, but marking s-s because other bugs with this assertion have been s-s.
Crash Signature: [@ js::ion::CodeGenerator::link]
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
Can you look into this, djvj, as Jandem is on PTO this week?  Thanks!
Flags: needinfo?(kvijayan)
CanEffectlesslyCallLookupGenericOnObject doesn't check for resolve hooks on object.
Attachment #758691 - Flags: review?
Flags: needinfo?(kvijayan)
Assignee: general → kvijayan
I'm going to assume the regressing bug is correct.
(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.
Thanks!
Best candidate for offending bug/commit:

changeset:   106691:d142f2087917
user:        Kannan Vijayan <kvijayan@mozilla.com>
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)
Attachment #758691 - Flags: review? → review?(hv1989)
Attachment #758691 - Flags: review?(hv1989) → review+
Setting in-testsuite? flag to remember to add this to the test suite after uplift to affected versions and suitable time has passed.
Flags: in-testsuite?
Can someone confirm that this resolves the issue in the affected releases?
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. :(
Kannan, is the performance regression expected?
Flags: needinfo?(kvijayan)
(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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla24
(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
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?
Check was too strict.  Updated patch coming now.
Flags: needinfo?(kvijayan)
Attached patch Fix massive regression. (obsolete) — Splinter Review
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.
Attachment #759163 - Flags: review?(hv1989)
Comment on attachment 759163 [details] [diff] [review]
Fix massive regression.

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

Nice, let's remove that regression ;)
Attachment #759163 - Flags: review?(hv1989) → review+
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.
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.
Attachment #759163 - Attachment is obsolete: true
Attachment #759248 - Flags: review?(hv1989)
Depends on: 880241
Attachment #759248 - Flags: review?(hv1989) → review?(bhackett1024)
Attachment #759248 - Flags: review?(bhackett1024) → review+
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...
Boris: Added comments in relevant places.  Thanks for the suggestion.

Committed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f98048e391b7
Probably doesn't happen, but something could have some of the other lookup stubs and not this one.
(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
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9ca690835a5e).
Duplicate of this bug: 880241
https://hg.mozilla.org/mozilla-central/rev/f98048e391b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Duplicate of this bug: 867756
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.
Attachment #758691 - Flags: approval-mozilla-beta?
Attachment #758691 - Flags: approval-mozilla-aurora?
> 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 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.
Attachment #759248 - Flags: approval-mozilla-beta?
Attachment #759248 - Flags: approval-mozilla-aurora?
What is the security rating of this bug? That's a prerequisite for tracking/landing.
(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.
Flags: needinfo?(kvijayan)
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?
Flags: needinfo?(bhackett1024)
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.
Flags: needinfo?(bhackett1024)
Attachment #758691 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #759248 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 882834
Our final beta build is on Monday. Is this safe to land given the recent failures? Makes me a bit more concerned.
(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.
Flags: needinfo?(kvijayan)
June 24 is when the release happens.
Comment on attachment 758691 [details] [diff] [review]
Fix CanEffectlesslyCallLookupGenericOnObject

Too late for our final beta of FF22.
Attachment #758691 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #759248 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
djvj is going to make the case on the security list to take this in FF22 once he's completed the regression fixes.
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.
This throws "undefined" as an exception if we encounter the activeAnalysis flag on the compartment when trying to interpret.
Attachment #763673 - Flags: review?(bhackett1024)
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).
Attachment #763673 - Flags: review?(bhackett1024) → review+
With comments addressed above.
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.
Attachment #763682 - Flags: approval-mozilla-beta?
Attachment #763682 - Flags: approval-mozilla-aurora?
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!
Attachment #763682 - Flags: approval-mozilla-beta?
Attachment #763682 - Flags: approval-mozilla-beta+
Attachment #763682 - Flags: approval-mozilla-aurora?
Attachment #763682 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main22+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.