Closed Bug 1252707 Opened 4 years ago Closed 4 years ago

Crash [@ IsInsideNursery] with OOM and use-after-free

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + verified
firefox-esr38 46+ fixed
firefox-esr45 46+ fixed

People

(Reporter: decoder, Assigned: bhackett1024)

References

(Blocks 2 open bugs)

Details

(7 keywords, Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.1+][adv-esr38.8+])

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision e15383656900 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2 --ion-extra-checks --ion-offthread-compile=off):

STATUS = "STATUS "
function printStatus(msg) {
    lines = msg.split();
    print(STATUS + lines[0]);
}
Object.defineProperty(Object.prototype, 'e', {});
Object.prototype.apply = summary = 'GC without recursion';
Object.prototype[0] = /a/
printStatus(summary);
oomAfterAllocations(10);
printStatus(summary)({}.__proto__.watch('x', print));



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
IsInsideNursery (cell=0xfffc4d4d4d4d4d4d) at js/src/debug64/dist/include/js/HeapAPI.h:333
#0  IsInsideNursery (cell=0xfffc4d4d4d4d4d4d) at js/src/debug64/dist/include/js/HeapAPI.h:333
#1  isTenured (this=0xfffc4d4d4d4d4d4d) at js/src/gc/Heap.h:222
#2  js::gc::TenuredCell::arenaHeader (this=0xfffc4d4d4d4d4d4d) at js/src/gc/Heap.h:1411
#3  0x0000000000979f3f in isMarked (color=0, this=<optimized out>) at js/src/gc/Heap.h:1380
#4  js::Shape::sweep (this=this@entry=0x7ffff7e8ce70) at js/src/jspropertytree.cpp:206
#5  0x00000000008ff9a4 in SweepThing (shape=0x7ffff7e8ce70) at js/src/jsgc.cpp:5463
#6  SweepArenaList<js::Shape> (sliceBudget=..., arenasToSweep=<optimized out>) at js/src/jsgc.cpp:5484
#7  js::gc::GCRuntime::sweepPhase (this=this@entry=0x7ffff695d430, sliceBudget=...) at js/src/jsgc.cpp:5585
#8  0x0000000000908113 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff695d430, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6192
#9  0x00000000009092e0 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff695d430, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6415
#10 0x0000000000909821 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff695d430, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6521
#11 0x0000000000909a53 in js::gc::GCRuntime::gc (this=this@entry=0x7ffff695d430, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6579
#12 0x00000000008bb40c in js::DestroyContext (cx=0x7ffff6907800, mode=js::DCM_FORCE_GC) at js/src/jscntxt.cpp:181
#13 0x00000000008bb65e in JS_DestroyContext (cx=<optimized out>) at js/src/jsapi.cpp:581
#14 0x000000000048076f in DestroyContext (withGC=true, cx=0x7ffff6907800) at js/src/shell/js.cpp:6351
#15 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7253
rax	0xfffc4d4d4d4d4d4d	-1040905502110387
rbx	0xfffc4d4d4d4d4d4d	-1040905502110387
rcx	0xfffc4d4d4d400000	-1040905502982144
rdx	0xfffc4d4d4d4fffe8	-1040905501933592
rsi	0x0	0
rdi	0xfffc4d4d4d4d4d4d	-1040905502110387
rbp	0x7fffffffd030	140737488343088
rsp	0x7fffffffd030	140737488343088
r8	0x0	0
r9	0x7ffff6a00368	140737331069800
r10	0x7ffff6930420	140737330218016
r11	0x7ffff6a00379	140737331069817
r12	0x7ffff7e8ce70	140737352617584
r13	0x7ffff46ac000	140737294024704
r14	0x7fffffffd550	140737488344400
r15	0x7ffff7e8ce70	140737352617584
rip	0x4dc950 <js::gc::TenuredCell::arenaHeader() const+32>
=> 0x4dc950 <js::gc::TenuredCell::arenaHeader() const+32>:	mov    (%rdx),%edx
   0x4dc952 <js::gc::TenuredCell::arenaHeader() const+34>:	test   %edx,%edx


This looks like a use-after-free crash or some other memory corruption given the memory patterns in the crashing register. Marking s-s.
Yeah this doesn't look too healthy. NI? terrence but feel free to forward if it's somewhere else.
Flags: needinfo?(terrence)
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:   https://hg.mozilla.org/mozilla-central/rev/f82a7f0db599
user:        Tooru Fujisawa
date:        Wed Mar 18 18:22:05 2015 +0900
summary:     Bug 1079919 - Part 0: Add RegExp ClassSpec. r=bholley

This iteration took 192.276 seconds to run.
Pending more analysis by Terrence, not sure if bug 1079919 is the culprit here since OOM is related.
We're going off the rails right here:

https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Shape.cpp#403

We've allocated a fresh Shape, but we return before initializing it because the allocation in growSlots fails. If I move the initialization up above the setSlotSpan, other assertions fail. I think we need some way to initialize the shape's pointers to something safe before exiting.

It looks like this was regressed by db8e6de6f68f in bug 693221. Passing ni? to Brian.
Flags: needinfo?(terrence) → needinfo?(bhackett1024)
Attached patch patchSplinter Review
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8726754 - Flags: review?(terrence)
Comment on attachment 8726754 [details] [diff] [review]
patch

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

Awesome!
Attachment #8726754 - Flags: review?(terrence) → review+
Comment on attachment 8726754 [details] [diff] [review]
patch

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

Probably very hard --- an OOM at a specific point causes the GC to access uninitialized GC thing memory.

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?

Trivial.

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

Not at all.

Approval Request Comment
[Feature/regressing bug #]: bug 693221
[User impact if declined]: potential crash
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none
Attachment #8726754 - Flags: sec-approval?
Attachment #8726754 - Flags: approval-mozilla-esr45?
Attachment #8726754 - Flags: approval-mozilla-beta?
Attachment #8726754 - Flags: approval-mozilla-aurora?
Comment on attachment 8726754 [details] [diff] [review]
patch

This was a bit last minute before the merge and I am about to start the beta 1 build. We can uplift this for beta 2 though. Just letting you know I see it.
Comment on attachment 8726754 [details] [diff] [review]
patch

sec-approval+
Attachment #8726754 - Flags: sec-approval? → sec-approval+
Comment on attachment 8726754 [details] [diff] [review]
patch

Crash fix, sec-high, approved for beta and aurora.
Attachment #8726754 - Flags: approval-mozilla-beta?
Attachment #8726754 - Flags: approval-mozilla-beta+
Attachment #8726754 - Flags: approval-mozilla-aurora?
Attachment #8726754 - Flags: approval-mozilla-aurora+
Attachment #8726754 - Flags: approval-mozilla-esr38?
*sigh* I could have sworn I'd fixed the author information on this before pushing. In the future, attaching patches with proper commit information would have avoided that problem...

https://hg.mozilla.org/mozilla-central/rev/b6daac63d773
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Comment on attachment 8726754 [details] [diff] [review]
patch

Fix a sec-high, taking it.
Should be in 45.1.0 & 38.8.0
Attachment #8726754 - Flags: approval-mozilla-esr45?
Attachment #8726754 - Flags: approval-mozilla-esr45+
Attachment #8726754 - Flags: approval-mozilla-esr38?
Attachment #8726754 - Flags: approval-mozilla-esr38+
This doesn't apply cleanly to esr38.
Flags: needinfo?(bhackett1024)
Attached patch esr38 patchSplinter Review
This patch should apply cleanly to esr38.
Flags: needinfo?(bhackett1024)
This needs ESR38 checkin.
Keywords: checkin-needed
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main46+][adv-esr45.1+]
has problems to uplift to esr38:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 59f372c35b24
grafting 335694:59f372c35b24 "Bug 1252707. r=terrence a=lizzard"
merging js/src/vm/Shape.cpp
warning: conflicts while merging js/src/vm/Shape.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(bhackett1024)
(In reply to Carsten Book [:Tomcat] from comment #20)
> has problems to uplift to esr38:
> 
> Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 59f372c35b24
> grafting 335694:59f372c35b24 "Bug 1252707. r=terrence a=lizzard"
> merging js/src/vm/Shape.cpp
> warning: conflicts while merging js/src/vm/Shape.cpp! (edit, then use 'hg
> resolve --mark')
> abort: unresolved conflicts, can't continue
> (use hg resolve and hg graft --continue)

See comment 17/18
Flags: needinfo?(bhackett1024)
Keywords: checkin-needed
Flags: needinfo?(cbook)
sorry brian, missed this patch

landed in https://hg.mozilla.org/releases/mozilla-esr38/rev/3fdd280fa099
Flags: needinfo?(cbook)
Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.1+] → [jsbugmon:update][adv-main46+][adv-esr45.1+][adv-esr38.8+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.