Assertion failure: allocated(), at ../../gc/Heap.h:472 or Opt-Crash [@ js::GCMarker::drainMarkStack]

VERIFIED FIXED in Firefox 20

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: dvander)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla22
x86
Linux
assertion, crash, csectype-uaf, sec-critical, testcase
Points:
---

Firefox Tracking Flags

(firefox19- wontfix, firefox20+ verified, firefox21+ fixed, firefox22+ fixed, firefox-esr17 unaffected, b2g18 disabled, b2g18-v1.0.0 disabled, b2g18-v1.0.1 disabled)

Details

(Whiteboard: [jsbugmon:update,ignore][adv-main20+], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 709731 [details]
Testcase for shell

The attached testcase asserts on mozilla-central revision 847e28c7ba67 (run with --ion-eager).
(Reporter)

Comment 1

5 years ago
Debug backtrace of assertion:


Program received signal SIGSEGV, Segmentation fault.
0x0804d5df in js::gc::ArenaHeader::getAllocKind (this=0xf742d000) at ../../gc/Heap.h:472
472             JS_ASSERT(allocated());
(gdb) bt
#0  0x0804d5df in js::gc::ArenaHeader::getAllocKind (this=0xf742d000) at ../../gc/Heap.h:472
#1  0x08071615 in js::gc::Cell::getAllocKind (this=0xf742d010) at ../gc/Heap.h:960
#2  0x08116d6f in js::gc::GetGCThingTraceKind (thing=0xf742d010) at ../jsgcinlines.h:59
#3  0x083ab6e2 in js::gc::MarkKind (trc=0xffffb328, thingp=0xffffb21c, kind=JSTRACE_OBJECT) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:353
#4  0x083abd38 in MarkValueInternal (trc=0xffffb328, v=0xf7434028) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:473
#5  0x083ac525 in js::gc::MarkObjectSlots (trc=0xffffb328, obj=(JSObject *) 0xf7434010 [object Object], start=0, nslots=2) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:620
#6  0x082e24ae in js::ObjectImpl::markChildren (this=0xf7434010, trc=0xffffb328) at /srv/repos/mozilla-central/js/src/vm/ObjectImpl.cpp:308
#7  0x083ad679 in js::gc::MarkChildren (trc=0xffffb328, obj=(JSObject *) 0xf7434010 [object Object]) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:943
#8  0x083aefe6 in js::TraceChildren (trc=0xffffb328, thing=0xf7434010, kind=JSTRACE_OBJECT) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:1454
#9  0x08090364 in JS_TraceChildren (trc=0xffffb328, thing=0xf7434010, kind=JSTRACE_OBJECT) at /srv/repos/mozilla-central/js/src/jsapi.cpp:2442
#10 0x0812e4c7 in CheckForCompartmentMismatches (rt=0x891fd30) at /srv/repos/mozilla-central/js/src/jsgc.cpp:2653
#11 0x0812e54a in BeginMarkPhase (rt=0x891fd30) at /srv/repos/mozilla-central/js/src/jsgc.cpp:2666
#12 0x08132d01 in IncrementalCollectSlice (rt=0x891fd30, budget=-2, reason=JS::gcreason::DEBUG_GC, gckind=js::GC_NORMAL) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4152
#13 0x08133455 in GCCycle (rt=0x891fd30, incremental=true, budget=-2, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4339
#14 0x08133906 in Collect (rt=0x891fd30, incremental=true, budget=-2, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4464
#15 0x08134026 in js::gc::RunDebugGC (cx=0x8946e18) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4638
#16 0x080c430f in js::gc::NewGCThing<JSObject, (js::AllowGC)1> (cx=0x8946e18, kind=js::gc::FINALIZE_OBJECT4_BACKGROUND, thingSize=48) at ../jsgcinlines.h:501
#17 0x0858cdb4 in js::ion::NewGCThing (cx=0x8946e18, allocKind=js::gc::FINALIZE_OBJECT4_BACKGROUND, thingSize=48) at /srv/repos/mozilla-central/js/src/ion/VMFunctions.cpp:119
#18 0xf7fcad37 in ?? ()


Opt-crash trace:

==29044== Invalid read of size 4
==29044==    at 0x82673C0: js::GCMarker::drainMarkStack(js::SliceBudget&) (Heap.h:671)
==29044==    by 0x80CECF4: IncrementalCollectSlice(JSRuntime*, long long, JS::gcreason::Reason, js::JSGCInvocationKind) (jsgc.cpp:3717)
==29044==    by 0x80D0BB4: GCCycle(JSRuntime*, bool, long long, js::JSGCInvocationKind, JS::gcreason::Reason) (jsgc.cpp:4339)
==29044==    by 0x80D0FB9: _ZL7CollectP9JSRuntimebxN2js18JSGCInvocationKindEN2JS8gcreason6ReasonE.part.284 (jsgc.cpp:4464)
==29044==    by 0x80D1C31: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:4383)
==29044==    by 0x8327E49: js::ion::IonCode::New(JSContext*, unsigned char*, unsigned int, JSC::ExecutablePool*) (jsgcinlines.h:501)
==29044==    by 0x835C8CB: _ZN2js3ion6Linker7newCodeEP9JSContextPNS0_14IonCompartmentE.constprop.408 (IonLinker.h:52)
==29044==    by 0x835D03D: js::ion::IonCacheGetProperty::attachReadSlot(JSContext*, js::ion::IonScript*, JSObject*, JSObject*, JS::Handle<js::Shape*>) (IonLinker.h:70)
==29044==    by 0x8363DAE: js::ion::GetPropertyCache(JSContext*, unsigned int, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) (IonCaches.cpp:925)
==29044==    by 0x5549E1F: ???
==29044==  Address 0xfc0b0 is not stack'd, malloc'd or (recently) free'd


Marking s-s based on opt-crash and GC involved.
Crash Signature: [@ js::GCMarker::drainMarkStack]
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 2

5 years ago
JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   106581:23a84dbb258f
user:        Alex Crichton
date:        Wed Jul 18 23:55:55 2012 -0700
summary:     Bug 775782 - Instrument pro/epilogue of functions for the SPS profiler in ionmonkey. r=pierron,dvander

changeset:   106582:b82fb4d04f60
parent:      106581:23a84dbb258f
parent:      100107:d78729026fb9
user:        David Anderson
date:        Mon Jul 23 12:37:49 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106583:50e28df7ff8f
parent:      106582:b82fb4d04f60
parent:      100276:5598b8c4f271
user:        David Anderson
date:        Tue Jul 24 16:32:08 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106584:7f0f1fdfa5e2
user:        Nicolas B. Pierron
date:        Tue Jul 24 17:24:08 2012 -0700
summary:     Bug 767349 - Simulate hidden instructions when the target is hidden. r=luke

changeset:   106585:2af804d84437
user:        Nicolas Pierron
date:        Tue Jul 24 17:24:07 2012 -0700
summary:     Bug 767349 - Track bad resume points when snapshots are encoded. r=dvander

changeset:   106586:eef915d5a18f
user:        Nicolas B. Pierron
date:        Tue Jul 24 17:48:47 2012 -0700
summary:     Bug 776748 - Do not invalidate ionScript when JM is invalidated. r=dvander

changeset:   106587:41f66d0e46b3
user:        David Anderson
date:        Wed Jul 25 02:08:41 2012 -0700
summary:     Backed out changeset eef915d5a18f

changeset:   106588:d80fbd8493f1
parent:      106587:41f66d0e46b3
parent:      100401:d03aed049b7b
user:        David Anderson
date:        Wed Jul 25 14:30:08 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106589:81146d7c9f51
user:        Sean Stangl
date:        Wed Jul 25 17:10:20 2012 -0700
summary:     Bug 777570 - visitMathFunctionD() should be isCall(). r=dvander

changeset:   106590:02f44534f7f5
user:        Nicolas B. Pierron
date:        Thu Jul 26 11:17:31 2012 -0700
summary:     Bug 776748 - Do not invalidate ionScript when JM is invalidated. r=dvander

changeset:   106591:31f9c38e4cb9
parent:      106590:02f44534f7f5
parent:      100585:f528e021ceb1
user:        David Anderson
date:        Thu Jul 26 18:19:02 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106592:1274d6819bae
user:        Jan de Mooij
date:        Fri Jul 27 13:08:24 2012 -0700
summary:     Fix hasLazyType assertion (bug 777647, r=dvander).

changeset:   106593:ee40f69169e9
user:        Jan de Mooij
date:        Fri Jul 27 13:12:30 2012 -0700
summary:     Don't go through GetPcScript to monitor AddValue edge cases (bug 776022, r=dvander).

changeset:   106594:ba811ef4de1c
user:        David Anderson
date:        Fri Jul 27 14:57:07 2012 -0700
summary:     Fix typo in recent merge.

changeset:   106595:a21e8bf3531f
user:        David Anderson
date:        Fri Jul 27 16:13:02 2012 -0700
summary:     Include loop entry types when determining OSR types (bug 774644, r=jandem).

changeset:   106596:a9addbf7e526
user:        Jan de Mooij
date:        Tue Jul 24 16:39:17 2012 +0200
summary:     [mq]: heur

changeset:   106597:db83474903a5
user:        David Anderson
date:        Fri Jul 27 17:16:35 2012 -0700
summary:     Backed out changeset a9addbf7e526

changeset:   106598:ae339e63d268
user:        David Anderson
date:        Fri Jul 27 17:17:26 2012 -0700
summary:     Backout due to orange.

changeset:   106599:83c83b185199
user:        Jan de Mooij
date:        Tue Jul 24 16:39:17 2012 +0200
summary:     Implement JSOP_MOD for doubles (bug 716694, r=dvander).

changeset:   106600:7a13838698ed
user:        Shu-yu Guo
date:        Sun Jul 29 11:52:45 2012 -0700
summary:     Refactor |Compile| to be templated and not use fp (bug 773339, r=dvander).

changeset:   106601:75f02a17f7cd
user:        Jan de Mooij
date:        Mon Jul 30 20:37:14 2012 +0200
summary:     Bug 776880 - Fix dropArguments call in CallConstructor to include |this|. r=dvander

changeset:   106602:54f9ee5403f0
user:        Jan de Mooij
date:        Mon Jul 30 20:43:44 2012 +0200
summary:     No bug - Add Compile to js::ion namespace to fix Clang build. r=dvander

changeset:   106603:08187a7ea897
parent:      106602:54f9ee5403f0
parent:      100846:4ca1d7d1d2da
user:        David Anderson
date:        Mon Jul 30 13:15:39 2012 -0700
summary:     Merge from mozilla-central.

This iteration took 83.998 seconds to run.
(Reporter)

Comment 3

5 years ago
Ccing IonMonkey/GC devs to figure out what's going on here :)
(Assignee)

Comment 4

5 years ago
This was not a failure I would have expected, so CC'ing the baseline compiler folks in case anything similar might affect them.

 (1) Build a SetProp IC, needsBarrier() is false, so no write barriers are generated.
 (2) -> Call Linker::newCode(cx) for the IC. GC turns needsBarrier=true.
 (3)   -> Attached ICs are purged.
 (4) Unattached IC from (1) is then attached, violating GC invariants.

So we have to watch out for cases where allocating a code object changes barrier state. It looks like sstangl had to fix this in CodeGenerator already.

Brian suggests just suppressing GC when allocating the IC code object and that seems reasonable, patch soon.
Assignee: general → dvander
Group: core-security
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Group: core-security
(Assignee)

Comment 5

5 years ago
Created attachment 710471 [details] [diff] [review]
fix
Attachment #710471 - Flags: review?(bhackett1024)
(Assignee)

Comment 6

5 years ago
For anyone debugging horrible GC stuff in the future, this was sort of the workflow:

 (1) Find the address that is faulting. In this case obj1 in |obj2.obj1| was finalized, but still accessible.
 (2) Log when |obj1| gets allocated or finalized in jsgc.
 (3) Log when MarkRuntime() gets called. 
 (*) obj1 was getting finalized after a MarkRuntime() for incremental GC.
     -> guessing missing write barrier
 (4) Turn off GC validator which confused debugging
 (5) Add hw watchpoint for &obj2.obj1
 (*) obj2.obj1 is set in C++, and write barrier is triggered.
     -> likely the missing barrier is elsewhere and kills a whole graph of objects
 (6) Use find/w <heap>, obj and add write barriers all over the place
 (7) Repeat (6) for each containing object until something useful comes up
 (*) land on JIT code that is clearly missing a barrier
     -> hw watchpoint on JIT code to see when it's created
Keywords: csec-uaf, sec-critical
Comment on attachment 710471 [details] [diff] [review]
fix

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

I think it would be simpler if the AutoSuppressGC was in newCode() and newIC() was removed.  When we allocate the IonCode for a finished script compilation we are in an AutoEnterAnalysis, and as of bug 772820 (landed for Fx 21) AutoEnterAnalysis will also suppress GC.  So there is no difference between newIC and newCode, at least on Nightly.
Attachment #710471 - Flags: review?(bhackett1024) → review+
status-b2g18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox19: --- → ?
tracking-firefox20: --- → +
tracking-firefox21: --- → +

Comment 8

5 years ago
wontfixing for FF19 given where we are in the cycle, let's make sure to get sec-approval here before landing on m-c.
status-firefox19: affected → wontfix
tracking-firefox19: ? → -
(Reporter)

Updated

5 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
(Reporter)

Comment 9

5 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 84cb7aa96a52).
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
status-b2g18-v1.0.1: --- → affected
(Assignee)

Comment 11

5 years ago
Created attachment 715709 [details] [diff] [review]
fix v2

Patch w/ suggestion
Attachment #710471 - Attachment is obsolete: true
Attachment #715709 - Flags: review?(bhackett1024)
(Assignee)

Comment 12

5 years ago
(In reply to Lukas Blakk [:lsblakk] from comment #10)
> Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of
> v1.0.1 are now also status-b2g18-v1.0.1: affected

If IonMonkey is still disabled on b2g18, it should be unaffected.
Attachment #715709 - Flags: review?(bhackett1024) → review+
status-b2g18: affected → disabled
status-b2g18-v1.0.0: --- → disabled
status-b2g18-v1.0.1: affected → disabled
status-firefox21: --- → affected
status-firefox22: --- → affected
(Assignee)

Comment 13

5 years ago
Comment on attachment 715709 [details] [diff] [review]
fix v2

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

Not easily.

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?

All.

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

The oldest branch might not have AutoSuppressGC, if that's the case we'll need an alternate fix. I haven't given that much thought yet but there are low-risk solutions.

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

Very unlikely, just perf testing from awfy.
Attachment #715709 - Flags: sec-approval?
Attachment #715709 - Flags: sec-approval? → sec-approval+
tracking-firefox22: --- → +
tracking-firefox-esr17: --- → ?
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b831500ca4be
(Assignee)

Comment 15

5 years ago
backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/b831500ca4be
(Assignee)

Comment 16

5 years ago
err, https://hg.mozilla.org/integration/mozilla-inbound/rev/6c126d076b0d

Updated

5 years ago
tracking-firefox-esr17: ? → 20+
David: do you have a new patch to try out here?
tracking-firefox-esr17: 20+ → ?
Flags: needinfo?(dvander)
(Assignee)

Comment 18

5 years ago
Whoops, sorry, forgot about this. It bounced for build bustage, will land a proper patch shortly.
Flags: needinfo?(dvander)
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c86595edba
https://hg.mozilla.org/mozilla-central/rev/e6c86595edba
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox22: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 21

5 years ago
JSBugMon: This bug has been automatically verified fixed.
Please prepare patches for all branches, including the ESR, now that this has stuck on m-c and is verified. Thanks!
tracking-firefox-esr17: ? → 20+
(Assignee)

Comment 23

5 years ago
Comment on attachment 715709 [details] [diff] [review]
fix v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #715709 - Flags: approval-mozilla-esr17?
Attachment #715709 - Flags: approval-mozilla-beta?
Attachment #715709 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 24

5 years ago
Whoops, missed the questionnaire

Bug caused by: IonMonkey
User impact if declined: Possible sg:crit
Testing completed: Yes
Risk to taking this patch: Very little
String or UUID changes made by this patch: None
(Assignee)

Comment 25

5 years ago
Comment on attachment 715709 [details] [diff] [review]
fix v2

Actually IonMonkey is 18 so this doesn't need ESR17 landing.
Attachment #715709 - Flags: approval-mozilla-esr17?
Comment on attachment 715709 [details] [diff] [review]
fix v2

Please make sure to land before tomorrow(3/5) to make this into FX20 beta3 going to build tomorrow.
Attachment #715709 - Flags: approval-mozilla-beta?
Attachment #715709 - Flags: approval-mozilla-beta+
Attachment #715709 - Flags: approval-mozilla-aurora?
Attachment #715709 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 27

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/c0633e54e6ff
(Assignee)

Comment 28

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/f2321dba96fd
status-firefox20: affected → fixed
status-firefox21: affected → fixed

Comment 29

5 years ago
Verified as fixed on Firefox 20 beta (03/06 mozilla-beta repo).
status-firefox20: fixed → verified
(In reply to David Anderson [:dvander] from comment #25)
> Comment on attachment 715709 [details] [diff] [review]
> fix v2
> 
> Actually IonMonkey is 18 so this doesn't need ESR17 landing.

Given that, marking ESR17's status accordingly.
status-firefox-esr17: affected → unaffected
tracking-firefox-esr17: 20+ → ---
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main20+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.