Closed Bug 466206 Opened 11 years ago Closed 11 years ago

Crash [@ js_Interpret] : function variables may be unrooted

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jorendorff, Assigned: igor)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(2 files, 1 obsolete file)

/* This crashes trying to access x.y. If I understand correctly this is because the Call object's slot for x isn't populated. Note that if the generator yields, js_PutCallObject is called and then there is no crash. */

var f;

function g() {
  var x = {};
  f = function () { x.y; };
  if (0) yield;
}

try { g().next(); } catch (e) {}
gc();
f();
Credit: nth16sd found this one using jsfunfuzz.
Blocks: 349611
Assignee: general → igor
Correction.  js_PutCallObject is not called if you change the "if (0)" to "if
(1)".  I think it's still at least potentially wrong in that case, and I think
the generator frame should go away at gc().  I don't know why it doesn't crash.

igor: brendan mentioned SendToGenerator is probably the right place for the call.  Because yield previously couldn't fail, this might be nontrivial.
(In reply to comment #1)
> Credit: nth16sd found this one using jsfunfuzz.

" Testcase from Gary " would sound better. :) Thanks Jason for helping to reduce!
Severity: normal → critical
Summary: js_PutCallObject needed when walking off end of generator → Crash [@ js_Interpret] : js_PutCallObject needed when walking off end of generator
The testcase in comment #0 crashes near null at js_Interpret in trunk with or without "-j", and this bug also seems capable of morphing to crash near potentially exploitable areas.
Flags: blocking1.9.1?
Keywords: crash, testcase
(In reply to comment #2)
> I think the generator frame should go away at gc().

For the generator the frame is a part of the generator object and fp->slots points into the memory allocated together with the generator. Thus without js_PutCallObject any live reference to a call object will root the corresponding generator and so the frame. See args_or_call_trace, http://hg.mozilla.org/mozilla-central/file/cf10a03eefe5/js/src/jsfun.cpp#l544 .

This is why yielding generator is not problematic in the example - the call object will root its frame. The problem is that js_Interpret sets fp->regs to null on exit and js_TraceFrame, http://hg.mozilla.org/mozilla-central/file/cf10a03eefe5/js/src/jsgc.cpp#l2781 , does not trace the argument and var slots when !fp->regs. This is bad even without the generator since it means that during js_PutArgsObject the arguments and variables are unrooted. Since the later can trigger the GC when adding properties during shape generation, we have a hazard for normal JS code.
Attached patch v1 (obsolete) — Splinter Review
The fix always roots the fixed portion of fp->slots.
Attachment #349966 - Flags: review?(brendan)
Attachment #349966 - Flags: review?(brendan)
Attachment #349966 - Flags: review+
Attachment #349966 - Flags: approval1.9.1?
Attachment #349966 - Flags: approval1.8.1.next?
The bug is a regression due to bug 441686 and exists only starting from 1.9.1.
Blocks: 441686
Attachment #349966 - Flags: approval1.8.1.next?
I am changing the title to better reflect the reality.
Summary: Crash [@ js_Interpret] : js_PutCallObject needed when walking off end of generator → Crash [@ js_Interpret] : function variables may be unrooted
Blocks: 466808
Comment on attachment 349966 [details] [diff] [review]
v1

a191=beltzner
Attachment #349966 - Flags: approval1.9.1? → approval1.9.1+
Attached patch v1 (mq patch)Splinter Review
Attachment #349966 - Attachment is obsolete: true
Attachment #350299 - Flags: review+
Pushed changeset ab646b8253b0 to mozilla-central.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
No longer blocks: 466808
Duplicate of this bug: 466808
Flags: in-testsuite+
Flags: in-litmus-
v fixed mozilla-central, but not tracemonkey.
Status: RESOLVED → VERIFIED
Flags: blocking1.9.1?
Whiteboard: [sg:critical?]
The patch is in 1.9.1 repository as it was pushed to mozilla-central before forking mozilla-1.9.1 repo:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ab646b8253b0
Keywords: fixed1.9.1
v 1.9.1
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.13?
Flags: wanted1.9.1.x+
Flags: blocking1.9.0.13?
Flags: blocking1.9.0.13+
I'm not sure why I marked this blocking given comment 7. That still holds true, doesn't it? We're not likely to take bug 441686 on the 1.9.0 branch. Maybe I got confused by the dupe which was filed using FF3.0.4, but if I would have read more closely I'd have seen that it affected "3.1" but not 3.

Please confirm that this bug does not affect 1.9.0 and that it's OK to unhide the bug.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.13?
Flags: blocking1.9.0.13+
Keywords: regression
Group: core-security
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x-
Flags: blocking1.9.0.14?
http://hg.mozilla.org/tracemonkey/rev/c064d7cd3ab3
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-466206.js,v  <--  regress-466206.js
initial revision: 1.1
Crash Signature: [@ js_Interpret]
You need to log in before you can comment on or make changes to this bug.