466206 - Crash [@ js_Interpret] : function variables may be unrooted

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

/* 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();

Comment 1

[Jason Orendorff [:jorendorff]]

Credit: nth16sd found this one using jsfunfuzz.

Comment 2

[Jason Orendorff [:jorendorff]]

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.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #1)
> Credit: nth16sd found this one using jsfunfuzz.

" Testcase from Gary " would sound better. :) Thanks Jason for helping to reduce!

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 5

[Igor Bukanov]

(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.

Comment 6

[Igor Bukanov]

Attachment: v1

The fix always roots the fixed portion of fp->slots.

Comment 7

[Igor Bukanov]

The bug is a regression due to bug 441686 and exists only starting from 1.9.1.

Comment 8

[Igor Bukanov]

I am changing the title to better reflect the reality.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 349966 [details] [diff] [review]
v1

a191=beltzner

Comment 10

[Igor Bukanov]

Attachment: v1 (mq patch)

Comment 11

[[e-mail]]

Pushed changeset ab646b8253b0 to mozilla-central.

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/geniter/regress-466206.js

Comment 14

[Bob Clary [:bc] (inactive)]

v fixed mozilla-central, but not tracemonkey.

Comment 15

[Igor Bukanov]

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

Comment 16

[Bob Clary [:bc] (inactive)]

v 1.9.1

Comment 17

[Daniel Veditz [:dveditz]]

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.

Comment 18

[Bob Clary [:bc] (inactive)]

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