1234425 - Crash [@ js::jit::BaselineFrame::script]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 388bdc46ba51 (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --target=i686-pc-linux-gnu --without-intl-api --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2 --baseline-eager --ion-extra-checks --ion-eager):

__defineGetter__("x", function() isFinite);
evaluate(`for (i=0;i<1000000;++i) x`);



Backtrace:

==16531==ERROR: AddressSanitizer: SEGV on unknown address 0x000002a6 (pc 0x088768e8 sp 0xffce2710 bp 0xffce2738 T0)
    #0 0x88768e7 in js::jit::BaselineFrame::script() const js/src/jsscript.h:1235
    #1 0x88768e7 in js::jit::BaselineFrame::overridePc() const js/src/jit/BaselineFrame.h:377
    #2 0x88768e7 in js::jit::JitFrameIterator::baselineFrame() const js/src/jit/BaselineFrame.h:382
    #3 0x88768e7 in js::jit::JitFrameIterator::baselineScriptAndPc(JSScript**, unsigned char**) const js/src/jit/JitFrames.cpp:237
    #4 0x8885c3f in js::jit::GetPcScript(JSContext*, JSScript**, unsigned char**) js/src/jit/JitFrames.cpp:1683
    #5 0x8eebf39 in JSContext::currentScript(unsigned char**, JSContext::MaybeAllowCrossCompartment) const js/src/jscntxtinlines.h:462
    #6 0x8eebf39 in JSContext::findVersion() const js/src/jscntxt.cpp:1160
    #7 0x8f2440a in JS::CompileOptions::CompileOptions(JSContext*, JSVersion) js/src/jsapi.cpp:3885
    #8 0x918c6ad in CreateEmptyScriptForClone(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>) js/src/jsscript.cpp:3642
    #9 0x918d47b in js::CloneScriptIntoFunction(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSFunction*>, JS::Handle<JSScript*>) js/src/jsscript.cpp:3680
    #10 0x9666974 in JSRuntime::cloneSelfHostedFunctionScript(JSContext*, JS::Handle<js::PropertyName*>, JS::Handle<JSFunction*>) js/src/vm/SelfHosting.cpp:2069
    #11 0x8fe927c in JSFunction::createScriptForLazilyInterpretedFunction(JSContext*, JS::Handle<JSFunction*>) js/src/jsfun.cpp:1468
    #12 0x955829b in JSFunction::getOrCreateScript(JSContext*) js/src/jsfun.h:398
    #13 0x955829b in JSObject::makeLazyGroup(JSContext*, JS::Handle<JSObject*>) js/src/vm/ObjectGroup.cpp:310
    #14 0x9722dc9 in js::TypeSet::ObjectKey::singleton() js/src/jsobjinlines.h:136
    #15 0x9722dc9 in js::HeapTypeSetKey::instantiate(JSContext*) js/src/vm/TypeInference.cpp:1301
    #16 0x9751a3f in (anonymous namespace)::CompilerConstraintInstance<(anonymous namespace)::ConstraintDataFreezeObjectFlags>::generateTypeConstraint(JSContext*, js::RecompileInfo) js/src/vm/TypeInference.cpp:1193
    #17 0x97239fa in js::FinishCompilation(JSContext*, JS::Handle<JSScript*>, js::CompilerConstraintList*, js::RecompileInfo*, bool*) js/src/vm/TypeInference.cpp:1404
    #18 0x8654f37 in js::jit::CodeGenerator::link(JSContext*, js::CompilerConstraintList*) js/src/jit/CodeGenerator.cpp:8133
    #19 0x87f7ed5 in LinkCodeGen(JSContext*, js::jit::IonBuilder*, js::jit::CodeGenerator*, JS::MutableHandle<js::TraceableVector<JSScript*, 0u, js::TempAllocPolicy, js::DefaultGCPolicy<JSScript*> > >, OnIonCompilationInfo*) js/src/jit/Ion.cpp:566
    #20 0x86ca224 in LinkBackgroundCodeGen(JSContext*, js::jit::IonBuilder*, JS::MutableHandle<js::TraceableVector<JSScript*, 0u, js::TempAllocPolicy, js::DefaultGCPolicy<JSScript*> > >, OnIonCompilationInfo*) js/src/jit/Ion.cpp:588
    #21 0x86ca224 in js::jit::LazyLink(JSContext*, JS::Handle<JSScript*>) js/src/jit/Ion.cpp:614
    #22 0x86cafff in js::jit::LazyLinkTopActivation(JSContext*) js/src/jit/Ion.cpp:645

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV js/src/jsscript.h:1235 js::jit::BaselineFrame::script() const
==16531==ABORTING


This test crashes also regular builds, but it reproduces really bad there (<1%). With a 32-bit ASan build, it almost always reproduces for me.

Comment 1

[Christian Holler (:decoder)]

Putting a needinfo on Hannes because this looks like his area if I remember correctly.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

If this almost always reproduces for a 32-bit ASan build, not sure if it's worth higher than a sec-low rating. I'll leave it to Hannes to double-check the severity.

Comment 3

[Tyson Smith [:tsmith]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> If this almost always reproduces for a 32-bit ASan build, not sure if it's
> worth higher than a sec-low rating. I'll leave it to Hannes to double-check
> the severity.

The crash address is 0x000002a6 which appears to be a null deref. Frequency has nothing to do with the rating here. It should be changed if this analysis is in accurate.

Comment 4

[Christian Holler (:decoder)]

This is actually very likely a sec-high. The 0x2a6 would be a very unusual null-deref for the JS engine but this also showed other crash patterns before if I remember correctly. Also, this is in the baseline jit code and similar crashes were sec-high/critical before.

Comment 5

[Christian Holler (:decoder)]

Oh and not to forget, it crashes on ASan builds but only with a very low percentage on non-ASan builds. So this is likely a memory corruption.

Comment 6

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 7

[Hannes Verschore [:h4writer]]

I haven't been able to get an ASAN build: /usr/bin/ld.bfd.real: cannot find -lstdc++
I've been trying for too long currently. Might be that I just need to get a clean system here. The error suggests I don't have the multilib packages installed. But I have them. Really strange.

Jan have you been able to build asan builds? If you can, can you take a look?

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

Yup, this reproduces for me with ASan on OS X.

We're in GetPCScript and the stack looks like this:

- LazyLink frame
- IonAccessorIC frame (scripted getter/setter IC)
- IonJS frame

GetPCScript knows it has to skip BaselineStub and IonStub frames, but it also has to skip IonAccessorIC frames.

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8704166 [details] [diff] [review]
Patch

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

I know that we tried to use GetPcScript as less as possible because of the performance penalty.
But doesn't this also mean it is harder for fuzzers to hit this issue? Should we look into running GetPcScript more often during debug builds or something?

::: js/src/jit/JitFrames.cpp
@@ +1628,5 @@
>              ++it;
>              MOZ_ASSERT(it.isBaselineStub() || it.isBaselineJS() || it.isIonJS());
>          }
>  
>          // Skip Baseline or Ion stub frames.

Can you update the comment?

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8704166 [details] [diff] [review]
Patch

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
It's pretty complicated; we could only reproduce it reliably with an ASan build. furthermore, I don't know if this is actually exploitable, but let's assume it is.

* 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?
43+

* If not all supported branches, which bug introduced the flaw?
Bug 1178834.

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

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

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 8704166 [details] [diff] [review]
Patch

sec-approval+ and a=dveditz for aurora. We probably want this on beta too but running out of time for that. I'll let you do a more formal beta-approval request from release drivers.

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f950d9cca5

(In reply to Hannes Verschore [:h4writer] from comment #9)
> Should we look into running GetPcScript more often during debug builds or
> something?

Yes that might help. It'd be nice to have this behind a flag so we don't run expensive checks in all debug builds...

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dc043de54b77

Comment 15

[Jan de Mooij [:jandem]]

Was merged to m-c last week:

https://hg.mozilla.org/mozilla-central/rev/f0f950d9cca5

Comment 16

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.