Crashes in EnterIon on Pinterest

RESOLVED FIXED in Firefox 36

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: lizzard, Assigned: jandem)

Tracking

({crash, sec-critical, topcrash})

34 Branch
mozilla38
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox34 wontfix, firefox35 wontfix, firefox36+ fixed, firefox37+ fixed, firefox38+ fixed, firefox-esr3136+ fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main36+][adv-esr31.5+], crash signature)

Attachments

(2 attachments)

This bug was filed from the Socorro interface and is 
report bp-7084ac9f-b873-4608-adeb-159db2141219.
=============================================================
This is currently the #6 topcrash for Firefox 34.0.5 with 4467/162224 crashes in the last 7 days. It's in the top 10 crash signatures for 34.0 as well. 

Many comments mention Pinterest. 



crashing thread:

0 		@0xabe23b7 	
1 	mozjs.dll 	EnterIon 	js/src/jit/Ion.cpp
2 	mozjs.dll 	js::jit::IonCannon(JSContext*, js::RunState&) 	js/src/jit/Ion.cpp
3 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
4 	mozjs.dll 	CanEnterBaselineJIT 	js/src/jit/BaselineJIT.cpp
5 	mozjs.dll 	js::InterpreterFrame::epilogue(JSContext*) 	js/src/vm/Stack.cpp
6 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
7 	mozjs.dll 	JSObject::putProperty<0>(js::ExclusiveContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>), unsigned int, unsigned int, unsigned int) 	js/src/vm/Shape.cpp
8 	mozjs.dll 	DefinePropertyOrElement<0> 	js/src/jsobj.cpp
9 	mozjs.dll 	js::GetElements(JSContext*, JS::Handle<JSObject*>, unsigned int, JS::Value*) 	js/src/jsarray.cpp
10 	mozjs.dll 	mozjs.dll@0x2033ff 	
11 		@0x17d78f3f 	
12 	mozjs.dll 	js::types::TypeObject::markPropertyNonData(js::ExclusiveContext*, jsid) 	js/src/jsinfer.cpp
13 	mozjs.dll 	js::types::MarkTypePropertyNonData(js::ExclusiveContext*, JSObject*, jsid) 	js/src/jsinferinlines.h
14 	mozjs.dll 	JSObject::changeProperty<0>(js::ExclusiveContext*, JS::Handle<JSObject*>, JS::Handle<js::Shape*>, unsigned int, unsigned int, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>), bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, bool, JS::MutableHandle<JS::Value>)) 	js/src/vm/Shape.cpp
15 	mozjs.dll 	js::jit::GetPcScript(JSContext*, JSScript**, unsigned char**) 	js/src/jit/IonFrames.cpp
16 	mozjs.dll 	js::types::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&)
I suspect EnterIon is an aggregate of many different crashes...

Jan, what information could we dig up to make this bug more actionable?
Flags: needinfo?(jdemooij)
Assignee

Comment 2

5 years ago
(In reply to dmajor (away) from comment #1)
> Jan, what information could we dig up to make this bug more actionable?

I've mentioned this a few times but it'd be really useful to have a disassembly view in crash-stats, probably behind the login. I currently use Ted's tool for this but it takes a lot more time. Once we have that, it should be easier to categorize them.

Other than that I don't know. Many comments/URLs mention pinterest (and the addon), I'd like to look into that again today.
Flags: needinfo?(jdemooij)
Assignee

Comment 3

5 years ago
I do hit the crash on pinterest sometimes... Not reliably but hopefully I can add some instrumentation.
Flags: needinfo?(jdemooij)
Assignee

Updated

5 years ago
Group: core-security
Assignee

Comment 4

5 years ago
Posted patch PatchSplinter Review
I was able to track down the pinterest crash. For LApplyArgsGeneric, we didn't call branchIfFunctionHasNoScript if there's a single callee, but this is wrong because functions can be relazified now.

This patch just always calls branchIfFunctionHasNoScript and should be very safe to backport.

Tomorrow I want to investigate how we can trigger this in the shell; it'd be nice if the fuzzers could find those bugs more easily.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8542774 - Flags: review?(shu)
Assignee

Comment 5

5 years ago
[Tracking Requested - why for this release]:
Topcrash and safe fix, we should backport this.

Comment 6

5 years ago
Comment on attachment 8542774 [details] [diff] [review]
Patch

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

LGTM.
Attachment #8542774 - Flags: review?(shu) → review+
Assignee

Comment 7

5 years ago
Comment on attachment 8542774 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not trivial, maybe with some work.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
Assuming 29+.

> If not all supported branches, which bug introduced the flaw?
Bug 886193, most likely.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Very easy to backport.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.

Approval Request Comment
[Feature/regressing bug #]: Bug 886193 most likely.
[User impact if declined]: Topcrash on Pinterest and likely other websites. Security issues.
[Describe test coverage new/current, TBPL]: Confirmed it fixes the pinterest crash.
[Risks and why]: Low risk. Very small and safe patch.
[String/UUID change made/needed]: None.
Attachment #8542774 - Flags: sec-approval?
Attachment #8542774 - Flags: approval-mozilla-esr31?
Attachment #8542774 - Flags: approval-mozilla-beta?
Attachment #8542774 - Flags: approval-mozilla-aurora?
Assignee

Updated

5 years ago
Keywords: sec-critical
OS: Windows NT → All
Hardware: x86 → All
Assignee

Updated

5 years ago
Summary: crash in EnterIon → Crashes in EnterIon on Pinterest
Assignee

Comment 8

5 years ago
Posted file Shell testcase
Crashes JS shell without the patch.
Assignee

Comment 9

5 years ago
(In reply to Jan de Mooij [:jandem] from comment #8)
> Crashes JS shell without the patch.

May require --ion-offthread-compile=off / --no-threads.
This is too late to make it into Firefox 35. We're out of Betas for that and the next build is the RC.
The question about sec-approval, since we missed the Beta branch, is how hard it is to figure out an exploit from this patch. I'd like to approve this now but I'm afraid of having it everywhere but 35 for six weeks.
Assignee

Comment 12

5 years ago
(In reply to Al Billings [:abillings] from comment #10)
> This is too late to make it into Firefox 35. We're out of Betas for that and
> the next build is the RC.

That's pretty unfortunate but I guess it can wait another cycle..

(In reply to Al Billings [:abillings] from comment #11)
> The question about sec-approval, since we missed the Beta branch, is how
> hard it is to figure out an exploit from this patch. I'd like to approve
> this now but I'm afraid of having it everywhere but 35 for six weeks.

Maybe we should just land this everywhere a few weeks from now?

Comment 14

5 years ago
I don't really agree with Al's comment #10 here. This has been rising somewhat to now #13 with .8% of all crashes on 35.0b8, and it is a sec-crit, happening on very common websites (Pinterest at least). I fear that having it sit around for 8+ weeks until we ship a fix is not a good idea, esp. as we have a patch in hand that is categorized as "low risk, very small and safe". This is the kind of thing I think we can even take on RC, despite me being pretty loud recently (and that will continue) on making fewer uplifts in general, esp. to beta.

BTW, on 34.0.5 release, this is now #5 with 1.3% of all crashes, so it impacts release users about double as much as Beta users.
Flags: needinfo?(release-mgmt)
Robert, we have no betas and are about to build our RC. We aren't taking this for 35, in my opinion. I'll needinfo? Release Management folks but I doubt they are going to approve it.
Flags: needinfo?(lsblakk)
Comment on attachment 8542774 [details] [diff] [review]
Patch

sec-approval+ for checkin on Jan 26.
Attachment #8542774 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin on 1/26]
If waiting is an option, happy to wait.  Thanks for the ping.
Flags: needinfo?(lsblakk)
Flags: needinfo?(release-mgmt)
Looks like this needs rebasing on trunk.
Flags: needinfo?(jdemooij)
Flags: in-testsuite?
Whiteboard: [checkin on 1/26]
Assignee

Comment 19

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18)
> Looks like this needs rebasing on trunk.

https://hg.mozilla.org/integration/mozilla-inbound/rev/68e641adcfb6
Flags: needinfo?(jdemooij)
Attachment #8542774 - Flags: approval-mozilla-esr31?
Attachment #8542774 - Flags: approval-mozilla-esr31+
Attachment #8542774 - Flags: approval-mozilla-beta?
Attachment #8542774 - Flags: approval-mozilla-beta+
Attachment #8542774 - Flags: approval-mozilla-aurora?
Attachment #8542774 - Flags: approval-mozilla-aurora+
Assignee

Updated

5 years ago
https://hg.mozilla.org/mozilla-central/rev/68e641adcfb6
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/936d24a4d78a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Whiteboard: [adv-main36+][adv-esr31.5+]

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
Crash volume for signature 'EnterIon':
 - nightly (version 50): 2 crashes from 2016-06-06.
 - aurora  (version 49): 3 crashes from 2016-06-07.
 - beta    (version 48): 16 crashes from 2016-06-06.
 - release (version 47): 67 crashes from 2016-05-31.
 - esr     (version 45): 12 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          0          0
 - aurora           0          0          1          0          0          0          0
 - beta             2          2          2          2          6          0          0
 - release         12         10          8          9         13          9          5
 - esr              0          1          1          3          1          2          1

Affected platform: Windows
You need to log in before you can comment on or make changes to this bug.