Closed Bug 1115776 Opened 6 years ago Closed 5 years ago

Crashes in EnterIon on Pinterest

Categories

(Core :: JavaScript Engine, defect)

34 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed
firefox-esr31 36+ 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

People

(Reporter: lizzard, Assigned: jandem)

Details

(Keywords: crash, sec-critical, topcrash, Whiteboard: [adv-main36+][adv-esr31.5+])

Crash Data

Attachments

(2 files)

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)
(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)
I do hit the crash on pinterest sometimes... Not reliably but hopefully I can add some instrumentation.
Flags: needinfo?(jdemooij)
Group: core-security
Attached 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)
[Tracking Requested - why for this release]:
Topcrash and safe fix, we should backport this.
Comment on attachment 8542774 [details] [diff] [review]
Patch

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

LGTM.
Attachment #8542774 - Flags: review?(shu) → review+
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?
Keywords: sec-critical
OS: Windows NT → All
Hardware: x86 → All
Summary: crash in EnterIon → Crashes in EnterIon on Pinterest
Attached file Shell testcase
Crashes JS shell without the patch.
(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.
(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?
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]
(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+
https://hg.mozilla.org/mozilla-central/rev/68e641adcfb6
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/936d24a4d78a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Whiteboard: [adv-main36+][adv-esr31.5+]
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.