1000598 - Use after free JS_ASSERT(arenaHeader()->allocated());

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Gregor Wagner [:gwagner]]

Seen on nexus 4 with debug gecko and current trunk.

STR:
Go to maps.google.com
press GPS icon a few times and keep zooming and moving around

Program received signal SIGSEGV, Segmentation fault.
0xb5e56b56 in js::gc::Cell::isMarked (color=<optimized out>, this=<optimized out>) at ../../../js/src/gc/Heap.h:1018
1018	    JS_ASSERT(arenaHeader()->allocated());
(gdb) bt
#0  0xb5e56b56 in js::gc::Cell::isMarked (color=<optimized out>, this=<optimized out>) at ../../../js/src/gc/Heap.h:1018
#1  0xb5e5722e in js::gc::Cell::isMarked (this=0xb0684440, color=0) at ../../../js/src/gc/Heap.h:1021
#2  0xb5e5746a in js::gc::IsAboutToBeFinalized<js::UnownedBaseShape> (thingp=<optimized out>) at ../../../js/src/gc/Marking.cpp:388
#3  0xb5fe9cc6 in js::types::TypeCompartment::sweep (this=0xb0984834, fop=<optimized out>) at ../../../js/src/jsinfer.cpp:4166
#4  0xb5fea894 in js::types::TypeZone::sweep (this=0xb0983694, fop=0xbeb836b8, releaseTypes=<optimized out>, oom=0xbeb83677) at ../../../js/src/jsinfer.cpp:4425
#5  0xb5e6ccc2 in JS::Zone::sweep (this=0xb0983400, fop=0xbeb836b8, releaseTypes=false, oom=0xbeb83677) at ../../../js/src/gc/Zone.cpp:119
#6  0xb5fd891e in BeginSweepingZoneGroup (rt=0xb31e4000) at ../../../js/src/jsgc.cpp:3718
#7  0xb5fda0d4 in BeginSweepPhase (rt=0xb31e4000, lastGC=<optimized out>) at ../../../js/src/jsgc.cpp:3822
#8  0xb5fdb7ba in IncrementalCollectSlice (rt=0xb31e4000, budget=<optimized out>, reason=JS::gcreason::ALLOC_TRIGGER, gckind=js::GC_NORMAL) at ../../../js/src/jsgc.cpp:4428
#9  0xb5fdbc92 in GCCycle (rt=0xb31e4000, incremental=<optimized out>, budget=0, gckind=js::GC_NORMAL, reason=JS::gcreason::ALLOC_TRIGGER) at ../../../js/src/jsgc.cpp:4564
#10 0xb5fdbff0 in Collect (rt=0xb31e4000, incremental=<optimized out>, budget=30000, gckind=js::GC_NORMAL, reason=JS::gcreason::ALLOC_TRIGGER) at ../../../js/src/jsgc.cpp:4697
#11 0xb5fdc296 in js::gc::GCIfNeeded (cx=<optimized out>) at ../../../js/src/jsgc.cpp:4840
#12 0xb5fa2aca in js::InvokeInterruptCallback (cx=0xb3cbec60) at ../../../js/src/jscntxt.cpp:1020
#13 0xb3d62d2c in ?? ()
#14 0xb3d62d2c in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb) f 3
#3  0xb5fe9cc6 in js::types::TypeCompartment::sweep (this=0xb0984834, fop=<optimized out>) at ../../../js/src/jsinfer.cpp:4166
4166	                if (IsTypeObjectAboutToBeFinalized(&typeObject))
(gdb) p typeObject
$5 = (js::types::TypeObject *) 0xb0684440
(gdb) p *typeObject
$6 = {<js::gc::BarrieredCell<js::types::TypeObject>> = {<js::gc::Cell> = {<No data fields>}, <No data fields>}, clasp_ = 0x4b4b4b4b, 
  proto_ = {<js::BarrieredPtr<JSObject, unsigned int>> = {{value = 0x4b4b4b4b, other = 1263225675}}, <No data fields>}, 
  singleton_ = {<js::BarrieredPtr<JSObject, unsigned int>> = {{value = 0x4b4b4b4b, other = 1263225675}}, <No data fields>}, static LAZY_SINGLETON = 1, flags_ = 1263225675, 
  addendum = {<js::BarrieredPtr<js::types::TypeObjectAddendum, unsigned int>> = {{value = 0x4b4b4b4b, other = 1263225675}}, <No data fields>}, propertySet = 0x4b4b4b4b, 
  interpretedFunction = {<js::BarrieredPtr<JSFunction, unsigned int>> = {{value = 0x4b4b4b4b, other = 1263225675}}, <No data fields>}, padding = 1263225675}

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Clear TypeCompartment tables.

Gregor tested this in person and it seems to work.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Pretty unlikely.

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

Not really.

> Which older supported branches are affected by this flaw?

aurora, beta, release. I'll let Gregor handle the b2g ones.

> If not all supported branches, which bug introduced the flaw?

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

Should apply cleanly.

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

Should be none. Testing on m-c after sec-approval should be enough.

Comment 3

[Shu-yu Guo [:shu]]

I'm not sure what sec rating this should have. Can one of the GC guys weigh in?

Comment 4

[Terrence Cole [:terrence]]

Use-after-free is sec-high or sec-crit, depending on how hard it is to take advantage of. High enough to bother backporting the fix in any case.

Comment 5

[Gregor Wagner [:gwagner]]

Paul, can you help with the b2g versions that need to be fixed?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Just to be clear, I think bug 897655 is where we regressed.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Bill McCloskey (:billm) from comment #6)
> Just to be clear, I think bug 897655 is where we regressed.

That's right: https://hg.mozilla.org/releases/mozilla-release/rev/b5e301863e69#l18.12

Comment 8

[Andrew McCreight [:mccr8]]

Marking flags based on comment 6.

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So double-checked in tree, and yes, affects 1.2 onwards. https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/file/14df3b8bf122/js/src/jscompartment.cpp#l593

I would recommend this as blocking-b2g: 1.3 since it is at least sec-high, maybe sec-critical, with small patch ready to be applied.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

This has sec-approval+ to go in on May 20 or later. We're shipping 29 in a few days and it missed the train for that.

Comment 11

[Shu-yu Guo [:shu]]

Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 897655
User impact if declined: Crashes?
Testing completed: in person
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

I don't see the uplift request for aurora. Is that expected? As Al said in comment #10, it is too late for 29 (and for 28 obviously).

Comment 13

[Shu-yu Guo [:shu]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> Comment on attachment 8411468 [details] [diff] [review]
> Clear TypeCompartment tables.
> 
> I don't see the uplift request for aurora. Is that expected? As Al said in
> comment #10, it is too late for 29 (and for 28 obviously).

That's why I didn't request it -- I thought that implied that I should wait to request approval for aurora after 05/20.

Anyways I don't know what's going on here and I don't really have time to play the tracking flags game. Can someone just make sure it gets checked in and uplifted to all the right branches?

Comment 14

[Al Billings [:abillings - ex-MoCo]]

(In reply to Shu-yu Guo [:shu] from comment #13)

> That's why I didn't request it -- I thought that implied that I should wait
> to request approval for aurora after 05/20.

This is exactly what I figured you would do since you don't have approval to check into trunk until 5/20 and Aurora and Beta then will be different branches than now.

Comment 15

[Andrew McCreight [:mccr8]]

Al or Ryan or somebody will go through and look for these delayed landings in a month, so you shouldn't have to think about it until then.

Comment 16

[Shu-yu Guo [:shu]]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> Al or Ryan or somebody will go through and look for these delayed landings
> in a month, so you shouldn't have to think about it until then.

Cool, thanks!

Comment 17

[Preeti Raghunath(:Preeti)]

Taking it in 1.3 per comment #9.

Comment 18

[Daniel Veditz [:dveditz]]

If this regressed from bug 897655 it should not affect ESR-24. Is the regressing bug wrong or is the esr-24 approval request unnecessary?

Comment 19

[Shu-yu Guo [:shu]]

(In reply to Daniel Veditz [:dveditz] from comment #18)
> If this regressed from bug 897655 it should not affect ESR-24. Is the
> regressing bug wrong or is the esr-24 approval request unnecessary?

The regression bug is right. The flag I set was unnecessary.

Comment 20

[Preeti Raghunath(:Preeti)]

Shu,

We need risk analysis of the patch in question here.

What are the areas of code it touches?

Comment 21

[Paul Theriault [:pauljt] (no longer reading bugmail)]

    To have any that this might make it onto a 1.3 device, we need to have this landed asap. There are two things in the way of this
    - it would mean landing a security fix early in the  desktop FF cycle, which we try to avoid
    - the risk that this patch causes an issue on the 1.3 branch

    For the first one, I've spoken with Al and Dveditz about the risk of landing this early in the cycle, and I think we decided that this was ok for an exception in this case, given the likely complexity involved with discovery and exploitation.

    I can't speak for the second issue though, as I have no idea how likely it is that this bug will cause instability etc. It seems like a relatively straightforward, selfcontained patch, but could I get a comment from someone who knows this code as to whether there is much stability risk of landing this patch on 1.3?

Comment 22

[Preeti Raghunath(:Preeti)]

Gregor,

Please assess for technical risks here. Any potential obvious regressions?

Reaching out to you per IRC with shu:

shu> hi, how's it going?
3:37 PM <Preeti> would you be able to help with risk analysis of the bug?
3:37 PM <shu> what is risk analysis
3:37 PM <shu> what kind of risk am i assessing?
3:37 PM <Preeti> code
3:37 PM <Preeti> modules touched
3:37 PM <shu> ah
3:37 PM <Preeti> areas of regression'
3:37 PM <shu> only js is touched
3:37 PM <Preeti> potential regression causes
3:38 PM <shu> outside of that i don't know much about it, i would ask gwagner
3:38 PM <Preeti> k thx

Comment 23

[Gregor Wagner [:gwagner]]

This patch has very low risk and it fixes a crash that is very easy to reproduce. We understand the problem and the fix. This is a straight forward bug fix. We should get this in.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

Not accepting the esr uplift request since ESR is unaffected.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I was needinfo'd, but I'll just say what Gregor said. This is very low risk and we should take it.

Comment 26

[Jan de Mooij [:jandem]]

Al, you cleared the [checkin on 5/20] whiteboard thing. Just to be sure, does this mean we should land this now?

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Yes, Jan. Paul and others said this needs to land ASAP for b2g timing.

Comment 28

[Jan de Mooij [:jandem]]

(In reply to Al Billings [:abillings] from comment #27)
> Yes, Jan. Paul and others said this needs to land ASAP for b2g timing.

OK, thanks, let's get this in.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64356b36da13

Please request Aurora/Beta approval on this ASAP to expedite it being uplifted everywhere else it needs to go.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8411468 [details] [diff] [review]
Clear TypeCompartment tables.

[Triage Comment]
As the tracking flags say (and RyanVM), we need it for both 30 & 31 (Beta & Aurora).

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64356b36da13

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cff3a438c3e1
https://hg.mozilla.org/releases/mozilla-beta/rev/56d4c53323c0

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/56d4c53323c0
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/55d2782a13c6
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/d5b1e2f40b05

Comment 34

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-release/rev/bd802fef45ae