1222917 - Crash [@ array_length_setter]

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

x = [];
print(Object.defineProperty(x, 0, {
    get: function() {
        this["length"] = this;
    }
}));
print(x);
print(x);

crashes js debug and opt shell on m-c changeset 42627d5369b3 with --fuzzing-safe --no-threads --ion-eager at array_length_setter

Configure options: (debug)

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --disable-threadsafe --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 42627d5369b3

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/73e1760e3c4d
user:        Jan de Mooij
date:        Thu Oct 15 16:20:29 2015 +0200
summary:     Bug 1214562 part 2 - Refactor SetPropertyCache regalloc. r=bhackett

Jan, is bug 1214562 a likely regressor? Setting s-s because crashes with arrays on the stack make me uneasy.

Comment 1

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

Attachment: debug stack

(lldb) bt 5
* thread #1: tid = 0x1a787, 0x000000010007e045 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] js::ObjectGroup::clasp(this=0xe4e4e4e400000000) const at ObjectGroup.h:94, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x000000010007e045 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] js::ObjectGroup::clasp(this=0xe4e4e4e400000000) const at ObjectGroup.h:94
    frame #1: 0x000000010007e045 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] JSObject::getClass(this=0x0000000102d78000) const + 3 at jsobj.h:122
    frame #2: 0x000000010007e042 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] bool JSObject::is<js::ArrayObject>(this=0x0000000102d78000) const at jsobj.h:543
    frame #3: 0x000000010007e042 js-dbg-64-dm-darwin-42627d5369b3`array_length_setter(cx=0x0000000102d69400, obj=<unavailable>, id=JS::HandleId @ rdx, vp=JS::MutableHandleValue @ r10, result=0x00007fff5fbfcc48) + 18 at jsarray.cpp:494
    frame #4: 0x0000000102c4bc32
(lldb)

Comment 2

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

Attachment: Opt stack

(lldb) bt 5
* thread #1: tid = 0x1a93f, 0x000000010004e8c0 js-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] js::ObjectGroup::clasp(this=0x0000000000000000) const at ObjectGroup.h:94, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010004e8c0 js-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] js::ObjectGroup::clasp(this=0x0000000000000000) const at ObjectGroup.h:94
    frame #1: 0x000000010004e8c0 js-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] JSObject::getClass(this=0x0000000101e6f000) const + 3 at jsobj.h:122
    frame #2: 0x000000010004e8bd js-64-dm-darwin-42627d5369b3`array_length_setter(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>, JS::ObjectOpResult&) [inlined] bool JSObject::is<js::ArrayObject>(this=0x0000000101e6f000) const at jsobj.h:543
    frame #3: 0x000000010004e8bd js-64-dm-darwin-42627d5369b3`array_length_setter(cx=0x0000000101e79400, obj=<unavailable>, id=JS::HandleId @ rdx, vp=JS::MutableHandleValue @ r10, result=0x00007fff5fbfd578) + 13 at jsarray.cpp:494
    frame #4: 0x00000001017c3ce7
(lldb)

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

The object and value registers for a setprop can now overlap:

  o.prop = o;

This fixes the JSPropertyOp setter code to not clobber the object register in this case.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Test

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8686637 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
It's not trivial or easy. Maybe with some work.

* 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?
44+.

* If not all supported branches, which bug introduced the flaw?
Bug 1214562.

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

* How likely is this patch to cause regressions; how much testing does it need?
Unlikely; it's also a pretty rare case we're fixing.

Comment 6

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

sec-approval+. Please nominate it for Aurora as well.

Comment 7

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2786c62ef4c9

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8686637 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1214562.
[User impact if declined]: Crashes, potential security bugs.
[Describe test coverage new/current, TreeHerder]: Fixes the test.
[Risks and why]: Low risk, patch is pretty straight-forward (and it's a bit of an edge case).
[String/UUID change made/needed]: None.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2786c62ef4c9

Comment 10

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c7fef57944e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3c7fef57944e

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Gary, can you verify fixed? Thanks.

Comment 13

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx44
JSBugMon: This bug has been automatically verified fixed on Fx45

Comment 14

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

JSBugMon has verified fixed.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62b2390a22b2

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62b2390a22b2