Closed Bug 1234425 Opened 5 years ago Closed 5 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox42 --- unaffected
firefox43 --- wontfix
firefox44 + wontfix
firefox45 + fixed
firefox46 + fixed
firefox47 --- verified
firefox-esr38 --- unaffected
firefox-esr45 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- affected
b2g-v2.2r --- unaffected
b2g-master --- affected


(Reporter: decoder, Assigned: jandem)



(4 keywords, Whiteboard: [jsbugmon:][adv-main45+])

Crash Data


(1 file)

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`);


==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

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.
Putting a needinfo on Hannes because this looks like his area if I remember correctly.
Flags: needinfo?(hv1989)
Keywords: sec-low
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.
(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.
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.
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.
Keywords: sec-lowsec-high
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
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?
Flags: needinfo?(hv1989) → needinfo?(jdemooij)
Attached patch PatchSplinter Review
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.
Assignee: nobody → jdemooij
Flags: needinfo?(jdemooij)
Attachment #8704166 - Flags: review?(hv1989)
Comment on attachment 8704166 [details] [diff] [review]

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?
Attachment #8704166 - Flags: review?(hv1989) → review+
Comment on attachment 8704166 [details] [diff] [review]

[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?

* Which older supported branches are affected by this flaw?

* 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?
Attachment #8704166 - Flags: sec-approval?
Comment on attachment 8704166 [details] [diff] [review]

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.
Attachment #8704166 - Flags: sec-approval?
Attachment #8704166 - Flags: sec-approval+
Attachment #8704166 - Flags: approval-mozilla-aurora+

(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...
Duplicate of this bug: 1238475
Was merged to m-c last week:
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Group: javascript-core-security → core-security-release
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main45+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.