1255949 - Crash [@ ??] with weird memory address

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 3a11a57b43aa (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

a = [1, 2, 3, 4, 5];
function foo4(x, m, n) {
    v = 0;
    for (var i = m; i < n; i++)
      v += x[i] + x[i - 1] + x[i - 2];
    return v
}
for (i = 0; i < 5; i++) 
  foo4(a, 2, 5);
foo4('xxxxxxxxxxxxx', 0, 5);



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7fc12ae in ?? ()
#0  0x00007ffff7fc12ae in ?? ()
#1  0x00007ffff7e00b68 in ?? ()
#2  0x0000000900000000 in ?? ()
#3  0x00007ffff7fe8bcd in ?? ()
#4  0x0000000000001044 in ?? ()
#5  0x00007ffff7e6d162 in ?? ()
#6  0x0000000000000000 in ?? ()
rax	0x7ffff7e00b68	140737352043368
rbx	0x5	5
rcx	0x7ffff7e8a070	140737352605808
rdx	0x7ffff7e82c90	140737352576144
rsi	0x7ffff7e82c88	140737352576136
rdi	0xc	12
rbp	0x0	0
rsp	0x7fffffffcca0	140737488342176
r8	0x0	0
r9	0xffffffff	4294967295
r10	0x7ffff3300050	140737273397328
r11	0x7ffff695d1e8	140737330401768
r12	0x0	0
r13	0x0	0
r14	0x1044	4164
r15	0x7fffffffcd50	140737488342352
rip	0x7ffff7fc12ae	140737353880238
=> 0x7ffff7fc12ae:	movzbl (%rdx,%r9,1),%edx
   0x7ffff7fc12b3:	cmp    $0x100,%edx


Marking s-s because this crashes with a weird memory address. Also marking as fuzzblocker because there is practically nothing a signature could reliably match on.

Comment 1

[Andrew McCreight [:mccr8]]

I'm going to assume this is crashing inside JIT code based on the lack of symbols. Jan, can you look at this or find somebody to look at it? Thanks.

Comment 2

[Jan de Mooij [:jandem]]

Bug 1131955 added MBoundsCheck::fallible_, if that's false we don't emit any LIR or code for it.

Here TryEliminateBoundsCheck is eliminating a fallible bounds check by updating an infallible one before it, but since it's infallible, we don't emit any code for it...

This is serious.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 4

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking since this is a sec-critical crash.

Comment 5

[Hannes Verschore [:h4writer]]

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

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

Good find!

Comment 6

[Jan de Mooij [:jandem]]

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

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Writing a test for this is not completely obvious or trivial, but it's also not extremely difficult...

> 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?
39+.

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

> 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?
Unlikely; I don't expect any perf/correctness regressions.

Comment 7

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

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

sec-approval+ for trunk. We'll want this on branches too.

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a57a5f81339

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a57a5f81339

Comment 10

[Jan de Mooij [:jandem]]

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

Approval Request Comment
[Feature/regressing bug #]: Bug 1131955.
[User impact if declined]: Security bugs, crashes.
[Describe test coverage new/current, TreeHerder]: Fixes the reported test.
[Risks and why]: Low risk. The patch just adds some checks to disable an optimization in certain cases.
[String/UUID change made/needed]: None.

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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

Crash fix, sec-critical, taking this for aurora and for 46 beta 6.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1cb807f4882c

Comment 14

[Sylvestre Ledru [:Sylvestre]]

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

Sec-critical, taking it.
Should be in 45.1.0

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/476ab557fc68