Closed Bug 662132 Opened 13 years ago Closed 13 years ago

TI: Crash [@ JSString::isFlat]

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox5 - wontfix
firefox6 + fixed
firefox7 + fixed
firefox8 + fixed
status2.0 --- wanted
status1.9.2 --- unaffected
status1.9.1 --- unaffected

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)

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
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
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.
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.
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.
Attached patch patchSplinter Review
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+
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.
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
Closed: 13 years ago
Resolution: --- → FIXED
This patch is good for 6, right? If so, please request beta approval for the patch!
Whiteboard: [sg:critical?][fixed-in-tracemonkey] → [sg:critical?][needs Fx6 approval request/landing][fixed-in-tracemonkey]
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 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
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.