TI: Crash [@ JSString::isFlat]

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: gkw, Assigned: bhackett)

Tracking

(Blocks: 2 bugs, {crash, testcase})

Trunk
x86
Mac OS X
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5- wontfix, firefox6+ fixed, firefox7+ fixed, firefox8+ fixed, status2.0 wanted, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical?] [jst likes for 6] [landed m-c 6/27][fixed-in-tracemonkey][qa-], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
gczeal()
let n = {}
let o = {}
for (let i = 0; i < 5; i++) {
    Array();
    for (let j = 0; j < 7; j++) {
        o.__defineSetter__("", function() {})
        o.p3 = 1
        delete o.p3
    }
}

crashes js debug shell on JM changeset 6d423e5f2e48 with -m, -j and -n at JSString::isFlat

This was found using a combination of jsfunfuzz and jandem's method fuzzer.

(gdb) bt
#0  0x000000010003d134 in JSString::isFlat (this=0x13e760) at jsstr.h:313
#1  0x00000001000477b5 in JSString::ensureFlat (this=0x13e760, cx=0x100925920) at jsstr.h:723
#2  0x0000000100047835 in JSString::getCharsZ (this=0x13e760, cx=0x100925920) at jsstr.h:703
#3  0x00000001000478e0 in js::types::MakeTypeId (cx=0x100925920, id={asBits = 1304416}) at jsinferinlines.h:118
#4  0x00000001000e99f2 in InlineAddTypeProperty (cx=0x100925920, obj=0x1009301c0, id={asBits = 1304416}, type=1) at /Users/fuzz3/Desktop/jsfunfuzz-dbg-64-jm-70388-6d423e5f2e48/compilePath/js/src/jsinfer.cpp:2723
#5  0x00000001000d671d in js::types::TypeObject::addPropertyType (this=0x1009301c0, cx=0x100925920, id={asBits = 1304416}, type=1) at /Users/fuzz3/Desktop/jsfunfuzz-dbg-64-jm-70388-6d423e5f2e48/compilePath/js/src/jsinfer.cpp:2739
#6  0x00000001001f5554 in js::types::AddTypePropertyId (cx=0x100925920, obj=0x1009301c0, id={asBits = 1304416}, type=1) at jsinferinlines.h:325
#7  0x000000010005a046 in JSObject::deleteProperty (this=0x100f16160, cx=0x100925920, id={asBits = 1304416}, rval=0x7fff5fbfdb10, strict=0) at jsobjinlines.h:161
#8  0x00000001002b093f in js::mjit::stubs::DelProp<0> (f=@0x7fff5fbfdb40, atom=0x13e760) at /Users/fuzz3/Desktop/jsfunfuzz-dbg-64-jm-70388-6d423e5f2e48/compilePath/js/src/methodjit/StubCalls.cpp:2649
#9  0x0000000100e67583 in ?? ()
#10 0x00000001002acb4d in js::mjit::EnterMethodJIT (cx=0x100925920, fp=0x100a67048, code=0x100e67fc0, stackLimit=0x100b1cb00) at /Users/fuzz3/Desktop/jsfunfuzz-dbg-64-jm-70388-6d423e5f2e48/compilePath/js/src/methodjit/MethodJIT.cpp:886
#11 0x00000001002acc82 in CheckStackAndEnterMethodJIT (cx=0x100925920, fp=0x100a67048, code=0x100e67fc0) at /Users/fuzz3/Desktop/jsfunfuzz-dbg-64-jm-70388-6d423e5f2e48/compilePath/js/src/methodjit/MethodJIT.cpp:918

/snip
Group: core-security
(Assignee)

Comment 1

7 years ago
TM bug triggered by a GC in the middle of a SetProp PIC update.  The PIC caches the IC's secondShapeGuard field (I think because it might modify it), and can also trigger a GC which resets the PIC (including the secondShapeGuard).  After the GC it uses the old value of secondShapeGuard and patches the inline path wrt that offset, which ends up trashing code memory after the SETPROP (in this case, the write of the atom passed to the DelProp stub).

It looks like other ICs may have similar problems --- which ICs can trigger a GC on paths where they end up patching the IC afterwards?
Brian, can you suggest a sg: severity rating here per https://wiki.mozilla.org/Security_Severity_Ratings?
Assignee: general → bhackett1024
(Assignee)

Comment 3

7 years ago
I think this would be hard to exploit (need to trigger a GC at exactly the right point), but would be nasty if it there is a consistent way to trigger the failure --- we are patching code memory with a value (shape number) that an attacker has a small amount of control over.
Whiteboard: [sg:critical?]
I don't know what the landing schedule is for the TI branch, but we need to make sure this gets fixed before we ship this new code.
status-firefox5: --- → unaffected
status-firefox6: --- → unaffected
tracking-firefox5: --- → -
tracking-firefox6: --- → -
tracking-firefox7: --- → +

Updated

7 years ago
tracking-firefox5: - → ---

Updated

7 years ago
tracking-firefox5: --- → -
(Assignee)

Comment 5

7 years ago
Should have been clearer in comment 3, the behavior described is possible on m-c and (I think) all versions of Firefox since 4.0.
tracking-firefox5: - → ---
(Assignee)

Updated

7 years ago
Duplicate of this bug: 663628
(Assignee)

Comment 7

7 years ago
Crashing testcase for TM tip (rev 49cfb12c2225, OS X x86, debug build, run with -m -a).  It's hard to trigger this with a plain gczeal(), as the IC needs to warm up to trip this but is reset by garbage collections.

gczeal(2, 7);
function foo(x) {
  x.f = 0;
  return [1,2,3,4,5];
}
foo({});
foo({a:0});
foo({b:0});
foo({c:0});
For future reference, we also have schedulegc(n) now, which schedules a GC after n allocations.
(Assignee)

Comment 9

7 years ago
Created attachment 538776 [details] [diff] [review]
patch

Patch, watches for GC on both SetProp and GetProp compilers.  It didn't look like other PIC stub compilers carry the secondShapeGuard info across VM calls.  I'm still not sure if the GetProp compiler can trigger GC on paths where it eventually tries to add a stub, but figure it's best to check regardless.
Attachment #538776 - Flags: review?(dvander)
Crash Signature: [@ JSString::isFlat]
Attachment #538776 - Flags: review?(dvander) → review+
(Assignee)

Comment 10

7 years ago
How should this land?
(In reply to comment #10)
> How should this land?

Both TM and TI branches, I think.
(In reply to comment #5)
> Should have been clearer in comment 3, the behavior described is possible on
> m-c and (I think) all versions of Firefox since 4.0.

Feel free to adjust the status-<branch> fields to reflect reality the same way you would the overall bug status. Also if you think we're wrong not to track something it would be better to re-nominate it ('?') rather than set '-' back to the default.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
status2.0: --- → wanted
status-firefox5: unaffected → wontfix
status-firefox6: unaffected → affected
status-firefox7: --- → affected
tracking-firefox5: --- → -
tracking-firefox6: - → +
(Assignee)

Comment 13

7 years ago
Hmm, never intended to modify the status flags, not sure what happened there.
Luke, or anyone else, can we get this landed...?
http://hg.mozilla.org/tracemonkey/rev/a3fc40d483e5
Whiteboard: [sg:critical?] → [sg:critical?][fixed-in-tracemonkey]
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
This patch is good for 6, right? If so, please request beta approval for the patch!
status-firefox7: affected → fixed
status-firefox8: --- → fixed
tracking-firefox8: --- → +
Whiteboard: [sg:critical?][fixed-in-tracemonkey] → [sg:critical?][needs Fx6 approval request/landing][fixed-in-tracemonkey]

Updated

7 years ago
Whiteboard: [sg:critical?][needs Fx6 approval request/landing][fixed-in-tracemonkey] → [sg:critical?] [jst likes for 6] [landed m-c 6/27][fixed-in-tracemonkey]
Comment on attachment 538776 [details] [diff] [review]
patch

dvander says this is safe for branches and could fix random crashes too. Requesting approvals.
Attachment #538776 - Flags: approval-mozilla-beta?
Attachment #538776 - Flags: approval-mozilla-aurora?

Comment 19

7 years ago
Comment on attachment 538776 [details] [diff] [review]
patch

approve for releases/mozilla-beta. Please land asap. We do not need to approve for aurora as it is already there
Attachment #538776 - Flags: approval-mozilla-beta?
Attachment #538776 - Flags: approval-mozilla-beta+
Attachment #538776 - Flags: approval-mozilla-aurora?
qa-: no QA fix verification needed
Whiteboard: [sg:critical?] [jst likes for 6] [landed m-c 6/27][fixed-in-tracemonkey] → [sg:critical?] [jst likes for 6] [landed m-c 6/27][fixed-in-tracemonkey][qa-]
Group: core-security
(Reporter)

Comment 22

6 years ago
Testcase needs tracer (-j) to trigger, tracer is long gone.

-> VERIFIED.
Status: RESOLVED → VERIFIED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.