Closed
Bug 1024756
Opened 10 years ago
Closed 10 years ago
Crash [@ js::jit::JitFrameIterator::script] or Assertion failure: [crash diagnostics] Marking invalid pointer 600 @ 7fff5fbfbb60 of type JSTRACE_OBJECT, named "ion-callee", at gc/Marking.cpp
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
VERIFIED
FIXED
mozilla33
Tracking | Status | |
---|---|---|
firefox31 | --- | unaffected |
firefox32 | --- | unaffected |
firefox33 | --- | verified |
firefox-esr24 | --- | unaffected |
b2g-v1.3 | --- | unaffected |
b2g-v1.3T | --- | unaffected |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.1 | --- | fixed |
People
(Reporter: gkw, Assigned: lth)
References
Details
(5 keywords, Whiteboard: [jsbugmon:])
Crash Data
Attachments
(4 files, 2 obsolete files)
3.94 KB,
text/plain
|
Details | |
4.57 KB,
text/plain
|
Details | |
3.42 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
1.95 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(function() { Array.buildPar(15891, function() { return [].map(function() {}) }) })() asserts js debug shell on m-c changeset aab3362f97e9 with --ion-eager --ion-parallel-compile=off at Assertion failure: [crash diagnostics] Marking invalid pointer 600 @ 7fff5fbfbb60 of type JSTRACE_OBJECT, named "ion-callee", at gc/Marking.cpp My configure flags are: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options> autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/573458d10426 user: Lars T Hansen date: Mon Jun 09 22:04:14 2014 -0700 summary: Bug 933313: Per-worker generational GC for PJS. r=jandem r=terrence r=shu r=jonco Lars, is bug 933313 a likely regressor? Shu-yu mentions this is PJS-specific, so nightly-only. Also, the usual drill about GC stuff, sec-high and s-s by default.
Flags: needinfo?(lhansen)
Reporter | ||
Comment 1•10 years ago
|
||
Configuration parameters: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR parameters>
Reporter | ||
Updated•10 years ago
|
Crash Signature: [@ js::jit::JitFrameIterator::script]
status-firefox32:
--- → unaffected
status-firefox33:
--- → affected
Keywords: crash
Summary: Assertion failure: [crash diagnostics] Marking invalid pointer 600 @ 7fff5fbfbb60 of type JSTRACE_OBJECT, named "ion-callee", at gc/Marking.cpp → Crash [@ js::jit::JitFrameIterator::script] or Assertion failure: [crash diagnostics] Marking invalid pointer 600 @ 7fff5fbfbb60 of type JSTRACE_OBJECT, named "ion-callee", at gc/Marking.cpp
Assignee | ||
Comment 2•10 years ago
|
||
Reproducible in a straight --enable-debug --disable-optimize build on Linux64 with --thread-count=1.
Assignee | ||
Comment 3•10 years ago
|
||
Happens on the first PJS gc, processing the first IonJSFrame. The layout looks like it is off by one word: (gdb) print layout $12 = (js::jit::IonJSFrameLayout *) 0x7fffffffa420 (gdb) print *layout $13 = { <js::jit::IonCommonFrameLayout> = { returnAddress_ = 0x0, descriptor_ = 140737352838206, static FrameTypeMask = 15 }, members of js::jit::IonJSFrameLayout: calleeToken_ = 0x600, numActualArgs_ = 140737305208096 } If this structure started one word further into memory it would look right. Consider code that would then be at the return address: (gdb) x/10i layout->descriptor_ 0x7ffff7ec2c3e: add $0x18,%rsp 0x7ffff7ec2c42: jmpq 0x7ffff7ec2c75 0x7ffff7ec2c47: mov %rax,%rax 0x7ffff7ec2c4a: mov %rsp,%rdi 0x7ffff7ec2c4d: and $0xfffffffffffffff0,%rsp 0x7ffff7ec2c51: push %rdi 0x7ffff7ec2c52: sub $0x8,%rsp 0x7ffff7ec2c56: mov %rax,%rdi 0x7ffff7ec2c59: test $0xf,%spl 0x7ffff7ec2c5d: je 0x7ffff7ec2c64 and the fact that the descriptor and callee token would all look right. Ergo some sort of frame/fp alignment issue.
Assignee | ||
Comment 4•10 years ago
|
||
Almost certainly the use of raw masm.push() / masm.pop() in NewDenseArrayPar -- RestPar has the same problem but is not used here -- because adding more of those pairs around the allocation changes the error in a reasonably predictable way.
Assignee | ||
Comment 5•10 years ago
|
||
Using masm.Push() and masm.Pop(), so as to properly track the frame size, seems to deal with the problem. There are the two known instances of that but I'll look to see if I introduced others before posting a patch. Gary, this bug is a regression but regressed only the PJS code paths, no other code paths should have been impacted. It's not clear to me that this is exploitable in any reasonable way. One would have to somehow write code that forces Ion to create a frame descriptor that can masquerade as a function pointer, and in that case the stack walk will likely crash on the next iteration with a much-too-large frame size.
Assignee: nobody → lhansen
Flags: needinfo?(lhansen)
Assignee | ||
Comment 6•10 years ago
|
||
[Security approval request comment] How easily could an exploit be constructed based on the patch? Probably difficult, see remarks in previous comment. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The check-in comment probably hints at that, yes. Which older supported branches are affected by this flaw? None, the patch that introduced the problem is recent and is in m-c only. If not all supported branches, which bug introduced the flaw? Bug 933313. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? N/A. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions. Needs only normal testing. Has been tested locally. Input from JIT module owner (jandem) has been sought.
Attachment #8439819 -
Flags: sec-approval?
Updated•10 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Comment 7•10 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 48eee276b1ee).
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
Updated•10 years ago
|
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
Comment 8•10 years ago
|
||
JSBugMon: Fix Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first good revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/48eee276b1ee tag: tip parent: 188516:adcf3f05f813 parent: 188531:7a69694fa94e user: Wes Kocher date: Thu Jun 12 17:27:02 2014 -0700 summary: Merge b2g-inbound to m-c a=merge This iteration took 216.606 seconds to run. The bug was introduced by a merge (it was not present on either parent). I don't know which patches from each side of the merge contributed to the bug. Sorry.
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #8) > JSBugMon: Fix Bisection requested, result: > autoBisect shows this is probably related to the following changeset: > > The first good revision is: I don't think this is accurate, so we should probably ignore it and get the patch in.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #9) > (In reply to Christian Holler (:decoder) from comment #8) > > JSBugMon: Fix Bisection requested, result: > > autoBisect shows this is probably related to the following changeset: > > > > The first good revision is: > > I don't think this is accurate, so we should probably ignore it and get the > patch in. OK. I don't have commit privileges yet, so anybody who wants to should feel free to push it. Note also that bug 1024567 may be caused by the same problem.
Comment 11•10 years ago
|
||
Lars, you don't need sec approval for PJS sec patches, since we are only enabled on Nightly.
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8439819 [details] [diff] [review] Patch sec-approval unrequested.
Attachment #8439819 -
Flags: sec-approval?
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 13•10 years ago
|
||
Lars, I'd suggest that you first get an r+ from someone, then add the testcase since this is nightly-only.
Flags: needinfo?(lhansen)
Keywords: checkin-needed
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #13) > Lars, I'd suggest that you first get an r+ from someone, then add the > testcase since this is nightly-only. OK, thanks. Most of that'll have to wait until tomorrow (or Monday in the worst case).
Flags: needinfo?(lhansen)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8439819 [details] [diff] [review] Patch (Pinging shu for feedback since he originally suggested going with "push" rather than "Push".)
Attachment #8439819 -
Flags: review?(jdemooij)
Attachment #8439819 -
Flags: feedback?(shu)
Comment 16•10 years ago
|
||
Comment on attachment 8439819 [details] [diff] [review] Patch Review of attachment 8439819 [details] [diff] [review]: ----------------------------------------------------------------- My bad, I should've caught this in review. Forgot about framePushed tracking for VM calls.
Attachment #8439819 -
Flags: feedback?(shu) → feedback+
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8439819 [details] [diff] [review] Patch Need to test something else and incorporate test cases first, let's wait on the review.
Attachment #8439819 -
Flags: review?(jdemooij)
Assignee | ||
Comment 18•10 years ago
|
||
Shu, the code is the same as what you saw earlier but the test case is new. Running try build, based on mozilla-inbound: https://tbpl.mozilla.org/?tree=Try&rev=4ff694cec56f
Attachment #8439819 -
Attachment is obsolete: true
Attachment #8440572 -
Flags: review?(shu)
Comment 19•10 years ago
|
||
Comment on attachment 8440572 [details] [diff] [review] Patch, with test case Review of attachment 8440572 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for suggesting push over Push. :(
Attachment #8440572 -
Flags: review?(shu) → review+
Reporter | ||
Comment 20•10 years ago
|
||
(In reply to Lars T Hansen [:lth] from comment #18) > Running try build, based on mozilla-inbound: > https://tbpl.mozilla.org/?tree=Try&rev=4ff694cec56f Try is green, r+, nightly-only since it's PJS stuff, helping to push to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce3b998dbbc
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/2ce3b998dbbc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Comment 22•10 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Comment 23•10 years ago
|
||
This issue is more subtle still. Pushing anything, with a tracked frame size or not, messes up IonJS frame marking. The marking code considers |fp() - ionScript->frameSize()| the spill base pointer -- the start of the stack where all the live regs were pushed on the stack. Pushing something invalidates this assumption and may crash during marking.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 24•10 years ago
|
||
I'm not sure why we were pushing lengthReg anyways, so just remove it. See explanation in c23.
Attachment #8442586 -
Flags: review?(jdemooij)
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #24) > Created attachment 8442586 [details] [diff] [review] > Remove unnecessary saving of lengthReg in NewDenseArrayPar codegen. > > I'm not sure why we were pushing lengthReg anyways, so just remove it. See > explanation in c23. I doubt that patch is right. For one thing, there's another instance of this pattern for RestPar (see my original patch). For the other, the reason lengthReg is saved is that the alloc call may call out to the VM and I had the distinct experience previously that that would clobber the length register. That could have been a consequence of other bugs present at the time, but it needs to be sorted out.
Comment 26•10 years ago
|
||
Lars pointed out that RestPar is the same. Jan, am I correct in assuming that all of a LIR's registers will be considered live by the regalloc at least until the next LIR? That is, lengthReg and numActuals will definitely be saved by saveLive(lir) for MNewDenseArrayPar and MRestPar, respectively?
Attachment #8442586 -
Attachment is obsolete: true
Attachment #8442586 -
Flags: review?(jdemooij)
Attachment #8442599 -
Flags: review?(jdemooij)
Comment 27•10 years ago
|
||
Comment on attachment 8442599 [details] [diff] [review] Remove unnecessary saving of registers across PJS OOL VM calls. Review of attachment 8442599 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Shu-yu Guo [:shu] from comment #26) > Jan, am I correct in assuming that all of a LIR's registers will be > considered > live by the regalloc at least until the next LIR? Yes that's correct AFAIK.
Attachment #8442599 -
Flags: review?(jdemooij) → review+
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6464ae5b5020
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Updated•10 years ago
|
status-b2g-v1.3:
--- → unaffected
status-b2g-v1.3T:
--- → unaffected
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.1:
--- → fixed
status-firefox31:
--- → unaffected
Updated•10 years ago
|
status-firefox-esr24:
--- → unaffected
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Comment 30•10 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•10 years ago
|
Group: javascript-core-security
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•