Closed Bug 1222917 Opened 4 years ago Closed 4 years ago

Crash [@ array_length_setter]

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox42 --- unaffected
firefox43 --- unaffected
firefox44 + verified
firefox45 + verified
firefox46 --- verified
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- fixed
b2g-v2.2r --- unaffected
b2g-master --- affected

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(4 files)

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.
Flags: needinfo?(jdemooij)
Attached file 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)
Attached file 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)
Keywords: sec-high
Attached patch Part 1 - FixSplinter Review
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8686637 - Flags: review?(bhackett1024)
Attached patch Part 2 - TestSplinter Review
Attachment #8686637 - Flags: review?(bhackett1024) → review+
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.
Attachment #8686637 - Flags: sec-approval?
sec-approval+. Please nominate it for Aurora as well.
Attachment #8686637 - Flags: sec-approval? → sec-approval+
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.
Attachment #8686637 - Flags: approval-mozilla-aurora?
Attachment #8686637 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/2786c62ef4c9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Group: javascript-core-security → core-security-release
Gary, can you verify fixed? Thanks.
Flags: needinfo?(gary)
Group: core-security-release
Status: RESOLVED → VERIFIED
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
JSBugMon has verified fixed.
Flags: needinfo?(gary)
You need to log in before you can comment on or make changes to this bug.