1064346 - Crash [@ IsInsideNursery] with use-after-free (likely OOM)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Testcase for shell

The attached testcase crashes on mozilla-central revision 2255d7d187b2 (run with --no-threads --fuzzing-safe).

Comment 1

[Christian Holler (:decoder)]

This looks like a memory-sensitive issue (likely OOM) and it only reproduced for me in an optimized build (--disable-debug --enable-optimize --enable-valgrind --enable-gczeal).

Backtrace:


Program received signal SIGSEGV, Segmentation fault.
IsInsideNursery (cell=0x4b4b4b4b4b4b4b4b) at ../../dist/include/js/HeapAPI.h:264
264         return location & ChunkLocationAnyNursery;
#0  IsInsideNursery (cell=0x4b4b4b4b4b4b4b4b) at ../../dist/include/js/HeapAPI.h:264
#1  MarkInternal<JSAtom> (trc=0x1650518, thingp=0x7ffff535f0b8) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:285
#2  0x000000000078016d in trace (trc=0x1650518, this=0x7ffff535f080) at /srv/repos/mozilla-central/js/src/jsfun.cpp:720
#3  fun_trace (trc=0x1650518, obj=0x7ffff535f080) at /srv/repos/mozilla-central/js/src/jsfun.cpp:757
#4  0x00000000004fe8e5 in js::GCMarker::processMarkStackTop (this=0x1650518, budget=...) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:1748
#5  0x00000000004d6cad in js::GCMarker::drainMarkStack (this=0x1650518, budget=...) at /srv/repos/mozilla-central/js/src/gc/Marking.cpp:1810
#6  0x000000000079c272 in drainMarkStack (phase=js::gcstats::PHASE_MARK, sliceBudget=..., this=0x164fc18) at /srv/repos/mozilla-central/js/src/jsgc.cpp:4817
#7  js::gc::GCRuntime::incrementalCollectSlice (this=0x164fc18, budget=<optimized out>, reason=<optimized out>) at /srv/repos/mozilla-central/js/src/jsgc.cpp:5351
rax     0x4b4fffe8      5425512962856058856
rbx     0x1650518       23397656
rcx     0x20000 131072
rdx     0xa95db6        11099574
rsi     0xf535f0b8      140737307340984
rdi     0x4b4b4b4b      5425512962855750475
rbp     0x1650518       23397656
rsp     0xffffca00      140737488341504
r8      0xc011  49169
r9      0x21080 135296
r10     0x100   256
r11     0x0     8796093022208
r12     0xf5342158      140737307222360
r13     0xf5360088      140737307345032
r14     0x2     2
r15     0xffffcb40      140737488341824
rip     0x4c1d3c <MarkInternal<JSAtom>(JSTracer*, JSAtom**)+76>
=> 0x4c1d3c <MarkInternal<JSAtom>(JSTracer*, JSAtom**)+76>:     mov    (%rax),%eax


Looks dangerous to me, marking s-s. Ccing Jonco and sfink because it looks related to GGC.

Comment 2

[Andrew McCreight [:mccr8]]

Can you look at this, Terrence?  It looks like Jon is on PTO.

Comment 3

[Terrence Cole [:terrence]]

Taking.

Comment 4

[Terrence Cole [:terrence]]

Attachment: bug_1064346-v0.diff

This is yet another path where JSFunction's quirky C-struct overlay on object slots is biting us.

In this case, we're failing a JSFunction allocation at setSingletonType after Object::create and before NewFunction. This leaves a JSObject with JSFunction::class's trace hook in the heap without having initialized it's memory in a way that the trace hook can understand it. In this case, shape->slotSpan() is hiding the slots and leaving them uninitialized; it really doesn't matter though. Whether the values we leave are uninitialized memory, poisoning, or even correct-for-an-object UndefinedValues, when JSFunction::trace looks at atom_, it's going to try to mark gibberish and crash.

We've already hacked up a bunch of paths at various places in the object and gc guts to manually check for JSFunction, so I think the right solution here is just to extend that and make JSObject::create aware that it needs to give JSFunctions C-style POD memory initialization.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8487552 [details] [diff] [review]
bug_1064346-v0.diff

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

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

Which older supported branches are affected by this flaw?
All of them.

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

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

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

Comment 6

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

How likely is this to cause regressions?

Comment 7

[Terrence Cole [:terrence]]

(In reply to Al Billings [:abillings] from comment #6)
> How likely is this to cause regressions?

Very, very unlikely.

Comment 8

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

Comment on attachment 8487552 [details] [diff] [review]
bug_1064346-v0.diff

sec-approval+ for checkin on 9/23 or later. We should have Aurora, Beta, and ESR31 patches for then too.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Terrence, can we have the uplift request? Thanks

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88d55c033782

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8487552 [details] [diff] [review]
bug_1064346-v0.diff

Approval Request Comment
[Feature/regressing bug #]: None - it's an ancient problem.
[User impact if declined]: A potentially exploitable crash.
[Describe test coverage new/current, TBPL]: The fuzzer testcase is too large (and potentially sensitive?) to be useful as a test.
[Risks and why]: Low. This simply adds initialization for JSFunction.
[String/UUID change made/needed]: None.

Comment 12

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

Comment on attachment 8487552 [details] [diff] [review]
bug_1064346-v0.diff

Giving approvals (except for release branch, which I cleared).

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88d55c033782

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6232dcb73aa5
https://hg.mozilla.org/releases/mozilla-beta/rev/fd4720dd6a46
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/41046048a8f7
https://hg.mozilla.org/releases/mozilla-esr31/rev/611e355a3543
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ae553c9bedd1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/41046048a8f7

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the information from comment #0 and comment #1 using revision 2255d7d187b2

fx 35 [revision: b85c260821ab] - Passed
fx 34 [revision: ee3cd82e3acb] - Passed
fx 33 [revision: d79568d581e6] - Passed
esr31 [revision: 8c431dcec0ff] - Passed