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

VERIFIED FIXED in Firefox 45

Status

()

defect
--
critical
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla46
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(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)

Details

(Whiteboard: [jsbugmon:][adv-main45+], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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.
(Reporter)

Comment 4

3 years ago
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.
(Reporter)

Comment 5

3 years ago
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.
(Reporter)

Updated

3 years ago
Keywords: sec-lowsec-high

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]

Comment 6

3 years ago
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)
(Assignee)

Comment 8

3 years ago
Posted 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
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8704166 - Flags: review?(hv1989)
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?
Attachment #8704166 - Flags: review?(hv1989) → review+
(Assignee)

Comment 10

3 years ago
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.
Attachment #8704166 - Flags: sec-approval?
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.
Attachment #8704166 - Flags: sec-approval?
Attachment #8704166 - Flags: sec-approval+
Attachment #8704166 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 12

3 years ago
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...
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1238475
(Assignee)

Comment 15

3 years ago
Was merged to m-c last week:

https://hg.mozilla.org/mozilla-central/rev/f0f950d9cca5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Group: javascript-core-security → core-security-release

Updated

3 years ago
Status: RESOLVED → VERIFIED

Comment 16

3 years ago
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.