Closed Bug 1362590 Opened 3 years ago Closed 3 years ago

Crash at weird memory address or Assertion failure: index < length_, at js/src/jit/FixedList.h:83

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr52 54+ fixed
firefox53 --- wontfix
firefox54 + fixed
firefox55 + verified

People

(Reporter: gkw, Assigned: shu)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [fuzzblocker][jsbugmon:ignore][adv-main54+][adv-esr52.2+])

Attachments

(3 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision c273884ffe6b (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

let { x } = '';

Backtrace:

#0  0x0000000000e894d4 in js::jit::FixedList<js::jit::StackValue>::operator[] (index=<optimized out>, this=0x7ffef35a09b0) at js/src/jit/FixedList.h:83
#1  js::jit::FrameInfo::rawPush (this=this@entry=0x7ffef35a09a0) at js/src/jit/BaselineFrameInfo.h:193
#2  js::jit::FrameInfo::push (this=this@entry=0x7ffef35a09a0, knownType=knownType@entry=JSVAL_TYPE_UNKNOWN, val=...) at js/src/jit/BaselineFrameInfo.h:227
#3  0x0000000000e914af in js::jit::BaselineCompiler::emit_JSOP_INITGLEXICAL (this=this@entry=0x7ffef359ff50) at js/src/jit/BaselineCompiler.cpp:3243
#4  0x0000000000eb3d38 in js::jit::BaselineCompiler::emitBody (this=this@entry=0x7ffef359ff50) at js/src/jit/BaselineCompiler.cpp:1030
#5  0x0000000000eb5511 in js::jit::BaselineCompiler::compile (this=this@entry=0x7ffef359ff50) at js/src/jit/BaselineCompiler.cpp:121
/snip

For detailed crash information, see attachment.

This crashes opt shells at a weird memory address, opt stack coming up.
Attached file Opt stack
Actually this is happening really often, and also other testcases crash with weird stacks like:

#0  0xf64ad290 in ?? ()
#1  0xf64ac470 in ?? ()
#2  0xf64ac470 in ?? ()

Thus, setting [fuzzblocker], and I'm inclined to back out the regressor.
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ed8623eefe6c
user:        André Bargull
date:        Thu Apr 27 09:24:08 2017 -0700
summary:     Bug 1360220 - Replace emitRequireObjectCoercible with JSOP_CHECKOBJCOERCIBLE. r=shu

Andre, is bug 1360220 a likely regressor?
Flags: needinfo?(andrebargull)
Blocks: 1360220
FWIW, this bug is sort-of FIXED due to the backout of bug 1360220, but leaving this open for now till that bug is fixed.
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker][jsbugmon:ignore]
I was able to resolve this issue by adding a |frame.syncStack(0)| after |frame.push(R0)| in  BaselineCompiler::emit_JSOP_INITGLEXICAL() [1]. But I have no idea why this fixes the crash and if that's the correct approach to fix the crash. FWIW the crash can also be reproduced by removing the RequireObjectCoercible() call in BytecodeEmitter.cpp [2], so I assume this isn't a bug in BaselineCompiler::emit_JSOP_CHECKOBJCOERCIBLE(), but instead a problem in  BaselineCompiler::emit_JSOP_INITGLEXICAL().

shu, can you take a look at this issue?

[1] http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/js/src/jit/BaselineCompiler.cpp#3239
[2] http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/js/src/frontend/BytecodeEmitter.cpp#5891-5892
Flags: needinfo?(andrebargull) → needinfo?(shu)
From reading comment 6, I think the problem is that BaselineCompiler::emit_JSOP_INITGLEXICAL pushes multiple stack values (for JSOP_SETPROP) but in vm/Opcodes.h we have nuses and ndefs 1. So we're using more frame slots than the compiler expects.
I don't know how this worked before. Shouldn't have been hardcoding the extra
slot into MinJITStackSize, but should've always been conditionally giving
global scripts an extra slot.
Attachment #8865716 - Flags: review?(jdemooij)
anba, does this patch fix the crashes from your patch?
Flags: needinfo?(shu) → needinfo?(andrebargull)
Attachment #8865716 - Flags: review?(jdemooij) → review+
(In reply to Shu-yu Guo [:shu] from comment #9)
> anba, does this patch fix the crashes from your patch?

Yes, the test case from comment #0 no longer crashes with the patch applied.
Flags: needinfo?(andrebargull)
Hm, I wonder if we need sec-approval for this. The bug is technically older, but I guess it was impossible to trigger?
Attachment #8865716 - Attachment is obsolete: true
Comment on attachment 8866041 [details] [diff] [review]
Always give global scripts an extra frame slot in JIT code for compiling INITGLEXICAL.

Not sure I need sec-approval, asking just in case. The bug is older but doesn't seem trigger currently.

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

I'm not sure how to exploit this.

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

Hard to say -- would give people ideas on where to start poking around to try to exploit, but certainly not a bulls-eye.

> Which older supported branches are affected by this flaw?

All

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

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

Easy to create, not risky to uplift since bugfix.

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

Unlikely.
Attachment #8866041 - Flags: sec-approval?
sec-approval+ for trunk.
We'll want this on Beta and ESR-52 as well, please nominate patches there.
Attachment #8866041 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/358b6ad12d18
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Assignee: nobody → shu
Please request Beta/ESR52 approval on this when you get a chance.
Flags: needinfo?(shu)
Target Milestone: --- → mozilla55
Comment on attachment 8866041 [details] [diff] [review]
Always give global scripts an extra frame slot in JIT code for compiling INITGLEXICAL.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Low, bug fix only
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 589199
[User impact if declined]: As far as I can tell, none, because the bug doesn't trigger without anba's new patch. Of course, won't rule out possible exploits 100%.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Bug fix
[String changes made/needed]: None
Flags: needinfo?(shu)
Attachment #8866041 - Flags: approval-mozilla-esr52?
Attachment #8866041 - Flags: approval-mozilla-beta?
Comment on attachment 8866041 [details] [diff] [review]
Always give global scripts an extra frame slot in JIT code for compiling INITGLEXICAL.

Fix a sec-high. Beta54+ & ESR52+. Should be in 54 beta 9.
Attachment #8866041 - Flags: approval-mozilla-esr52?
Attachment #8866041 - Flags: approval-mozilla-esr52+
Attachment #8866041 - Flags: approval-mozilla-beta?
Attachment #8866041 - Flags: approval-mozilla-beta+
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [fuzzblocker][jsbugmon:ignore] → [fuzzblocker][jsbugmon:ignore][adv-main54+][adv-esr52.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.