Closed
Bug 662132
Opened 13 years ago
Closed 13 years ago
TI: Crash [@ JSString::isFlat]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: gkw, Assigned: bhackett1024)
References
Details
(Keywords: crash, testcase, Whiteboard: [sg:critical?] [jst likes for 6] [landed m-c 6/27][fixed-in-tracemonkey][qa-])
Crash Data
Attachments
(1 file)
3.44 KB,
patch
|
dvander
:
review+
christian
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Updated•13 years ago
|
Group: core-security
Assignee | ||
Comment 1•13 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?
Comment 2•13 years ago
|
||
Brian, can you suggest a sg: severity rating here per https://wiki.mozilla.org/Security_Severity_Ratings?
Assignee: general → bhackett1024
Assignee | ||
Comment 3•13 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?]
Comment 4•13 years ago
|
||
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•13 years ago
|
tracking-firefox5:
- → ---
Updated•13 years ago
|
tracking-firefox5:
--- → -
Assignee | ||
Comment 5•13 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 | ||
Comment 7•13 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•13 years ago
|
||
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)
Updated•13 years ago
|
Crash Signature: [@ JSString::isFlat]
Updated•13 years ago
|
Attachment #538776 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 10•13 years ago
|
||
How should this land?
(In reply to comment #10) > How should this land? Both TM and TI branches, I think.
Comment 12•13 years ago
|
||
(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
status-firefox7:
--- → affected
tracking-firefox5:
--- → -
Assignee | ||
Comment 13•13 years ago
|
||
Hmm, never intended to modify the status flags, not sure what happened there.
Comment 14•13 years ago
|
||
Luke, or anyone else, can we get this landed...?
Comment 15•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/a3fc40d483e5
Whiteboard: [sg:critical?] → [sg:critical?][fixed-in-tracemonkey]
Comment 16•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/a3fc40d483e5
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 17•13 years ago
|
||
This patch is good for 6, right? If so, please request beta approval for the patch!
Updated•13 years ago
|
Whiteboard: [sg:critical?][fixed-in-tracemonkey] → [sg:critical?][needs Fx6 approval request/landing][fixed-in-tracemonkey]
Updated•13 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 18•13 years ago
|
||
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•13 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?
Comment 20•13 years ago
|
||
http://hg.mozilla.org/mozilla-beta/rev/a7367773bb01
Comment 21•13 years ago
|
||
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-]
Updated•13 years ago
|
Group: core-security
Reporter | ||
Comment 22•12 years ago
|
||
Testcase needs tracer (-j) to trigger, tracer is long gone. -> VERIFIED.
Status: RESOLVED → VERIFIED
Comment 23•11 years ago
|
||
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.
Description
•