1505181 - Crash [@ js::jit::IonSetPropertyIC::update] or Crash [@ ??] or Crash [@ JSObject::getClass] with invalid address and TypedArray

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision d2963b5a2897 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --disable-oom-functions --ion-eager --baseline-eager):

See attachment.


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555555ec0750 in js::jit::IonSetPropertyIC::update (cx=0x7ffff5f28800, outerScript=..., ic=0x7ffff580c8b0, obj=..., idVal=..., rhs=...) at js/src/jit/IonIC.cpp:265
#0  0x0000555555ec0750 in js::jit::IonSetPropertyIC::update (cx=0x7ffff5f28800, outerScript=..., ic=0x7ffff580c8b0, obj=..., idVal=..., rhs=...) at js/src/jit/IonIC.cpp:265
#1  0x000032ab7bbb658a in ?? ()
#2  0x00007fffffffa5a0 in ?? ()
#3  0x00007fffffffa4c8 in ?? ()
#4  0x00007fffffffa5a8 in ?? ()
#5  0x000055555739b860 in ?? ()
#6  0x000032ab7bcf8363 in ?? ()
#7  0x0000000000008020 in ?? ()
#8  0x00007ffff59d3a60 in ?? ()
#9  0x00007ffff580c8b0 in ?? ()
#10 0x0007800000000000 in ?? ()
#11 0xfffb7ffff599db28 in ?? ()
#12 0xfffe7ffff59d2ac0 in ?? ()
#13 0xe5e5e5e5e5e5e5e5 in ?? ()
#14 0x0007800000000000 in ?? ()
#15 0x00007ffff5abcd40 in ?? ()
#16 0x00007ffff59d2ac0 in ?? ()
#17 0x00007ffff580c8b0 in ?? ()
#18 0x00007ffff59d2b40 in ?? ()
#19 0x0000000000000000 in ?? ()
rax	0x7800000000000	2111062325329920
rbx	0x0	0
rcx	0x0	0
rdx	0x0	0
rsi	0x7fffffffa4e0	140737488332000
rdi	0x7ffff5f28800	140737319700480
rbp	0x7fffffffa4f8	140737488332024
rsp	0x7fffffffa1c0	140737488331200
r8	0x7fffffffa4f8	140737488332024
r9	0x7fffffffa500	140737488332032
r10	0x7fffffffa4e0	140737488332000
r11	0x55555739b860	93825023981664
r12	0x7fffffffa4f0	140737488332016
r13	0x7ffff580c8b0	140737312245936
r14	0x7fffffffa220	140737488331296
r15	0x7ffff5f28800	140737319700480
rip	0x555555ec0750 <js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)+272>
=> 0x555555ec0750 <js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)+272>:	mov    (%rax),%rdx
   0x555555ec0753 <js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)+275>:	mov    (%rdx),%rdx


This looks like a write with a broken address and the address has changed multiple times during reduction, from looking totally random to NULL to the one in this trace now. Marking as sec-critical based on that. The testcase is not fully reduced yet (the crash seems to require some of the logic of the new driver I wrote, just running the finalized code doesn't work), so I'm filing it as it is so we get to work on it as soon as possible.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jan de Mooij [:jandem]]

Attachment: More reduced testcase

This repros with --ion-offthread-compile=off --ion-eager

Comment 3

[Jan de Mooij [:jandem]]

Attachment: A bit nicer

Comment 4

[Jan de Mooij [:jandem]]

I'm looking into this. Seems to be TI related probably.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1505181 - Use canonical function in TypeNewScript::rollbackPartiallyInitializedObjects. r?bhackett

Comment 6

[Jan de Mooij [:jandem]]

Old bug. Took me a while to track down.

Comment 7

[Jan de Mooij [:jandem]]

Just to make sure: the worst that can happen here is that we reinterpret UndefinedValue as something else, right? Can you think of worse problems when we don't rollbackPartiallyInitializedObjects?

Comment 8

[Jan de Mooij [:jandem]]

Or there's also the UnboxedPlainObject::convertToNative call that we shouldn't be skipping probably. Enough complexity to keep this sec-crit I guess.

Comment 9

[Christian Holler (:decoder)]

During reduction, I have 100% seen the crash address jump around, it wasn't just 0x7800000000000 but I also saw addresses starting with 0x33... etc. so I assume this can have multiple side-effects.

Comment 10

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Just to make sure: the worst that can happen here is that we reinterpret
> UndefinedValue as something else, right? Can you think of worse problems
> when we don't rollbackPartiallyInitializedObjects?

This seems like it should be the case, and not calling convertToNative shouldn't cause downstream problems, but I can't be sure and comment 9 suggests otherwise.

Comment 11

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/7820fd141998
user:        Brian Hackett
date:        Tue Jan 27 02:47:25 2015 -0700
summary:     Bug 1116855 - Add default-disabled unboxed objects for use by interpreted constructors, r=jandem.

Would bug 1116855 be a likely regressor?

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 9023414 [details]
Bug 1505181 - Use canonical function in TypeNewScript::rollbackPartiallyInitializedObjects. r?bhackett

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: It's 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?: All

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

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: Should either apply or be easy to backport.

How likely is this patch to cause regressions; how much testing does it need?: Should be unlikely. Fuzzing + Nightly testing will be sufficient I think.

Comment 13

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

sec-approval+ for trunk.
Please nominate beta and ESR60 patches as well.

Comment 14

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2599b449d317ad862d059fdc5fe0d97e9caa15b2

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 9023414 [details]
Bug 1505181 - Use canonical function in TypeNewScript::rollbackPartiallyInitializedObjects. r?bhackett

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Unknown

User impact if declined: Security issues, crashes.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): Patch is relatively small and well understood but it's tricky code.

String changes made/needed: None

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Patch for ESR60

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2599b449d317

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9023414 [details]
Bug 1505181 - Use canonical function in TypeNewScript::rollbackPartiallyInitializedObjects. r?bhackett

[Triage Comment]
Fixes a sec-crit. Approved for 64.0b10 and 60.4.0esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/ee204e26690e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f5ad8dcc3ee8

Comment 21

[Vlad Lucaci, Desktop QA (:vlucaci)]

I have tried to reproduce this issue with the following build : https://tools.taskcluster.net/index/gecko.v2.mozilla-central.pushdate.2018.11.06.20181106095323.firefox but without any success. 

I see that this is covered by automated tests so I will remove the qe-verify+ flag.