473282 - TM: Crash [@ JS_CallTracer]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

this.watch("b", "".substring);
try {
  (new Function("this.__defineGetter__(\"a\", gc).__defineGetter__(\" \", function(){})"))();
  }
catch(runError){};
for each (b in [this, null, null])({});
a;

crashes (TM-only) opt js shell at JS_CallTracer at null and asserts debug js at Assertion failure: thing, at ../jsgc.cpp:2619

Security-sensitive because this involves gc.

Comment 1

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

Attachment: crashlog

this.watch("b", "".substring);
try { (new Function("this.__defineGetter__(\"a\", gc)"))(); }
catch(runError){};
for each (b in [this, null, null])({});
a;

Slightly-further reduced testcase, the second defineGetter apparently wasn't needed.

Comment 2

[Jeff Walden [:Waldo]]

Even more minimally:

this.watch("b", "".substring);
__defineGetter__("a", gc);
for each (b in [this, null, null]);
a

Comment 3

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

Adding [sg:investigate] pending [sg:critical?], thanks Jesse for suggesting this.

Comment 4

[Jeff Walden [:Waldo]]

I last worked on debugging this several days to a week ago; there seems to have been a mismatch between what was in the slot for the |b| variable and the type assigned to it -- for whatever reason the value in the slot was (jsval(0)), i.e. JSVAL_NULL, but the type assigned to it was JSVAL_STRING.  It looks like we're not bailing correctly when setting a property that has a watchpoint, but as I'd been debugging in the trace-execution part of this and not the recording part of this, I don't quite know where the problem is yet.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Don't trace through watches

I took a guess at the smattering of checks I needed to add beyond the obvious one; let me know if I have too many or perhaps need more.

As for security severity, not clear to me -- we'll copy over a value of possibly any type if we have a watchpoint like this, but I don't know how you could use a value of a different type this way to get started on any exploits.  Someone else with more experience in how we do our guarding could say more about this, I'm sure.

Comment 6

[Andreas Gal :gal]

Comment on attachment 358319 [details] [diff] [review]
Don't trace through watches

What happens when we add a watchpoint after code was compiled? Does updating the getter/setter change the shape and we trip that guard?

Comment 7

[Brendan Eich [:brendan]]

Attachment: fix

The bug was in the patch for bug 465443 -- my fault for not catching it on review. TraceRecorder::test_property_cache should not duplicate (mostly, with an added js_watch_set check) the code in TraceRecorder::test_property_cache_direct_slot to work around the bug over in js_FillPropertyCache.

Sanity check:

$ grep JOF_INCDEC js{interp,tracer}.cpp | grep JOF_FOR
jsinterp.cpp:        if (!(cs->format & (JOF_SET | JOF_INCDEC | JOF_FOR)) &&
jstracer.cpp:    uint32 setflags = (js_CodeSpec[*cx->fp->regs->pc].format & (JOF_SET | JOF_INCDEC | JOF_FOR));
jstracer.cpp:        uint32 setflags = (js_CodeSpec[*cx->fp->regs->pc].format & (JOF_SET | JOF_INCDEC | JOF_FOR));
jstracer.cpp:    uint32 setflags = (cs.format & (JOF_SET | JOF_INCDEC | JOF_FOR));

/be

Comment 8

[Brendan Eich [:brendan]]

The test in bug 465443 comment 0 passes with the patch.

/be

Comment 9

[Brendan Eich [:brendan]]

Waldo, hope you don't mind if I steal.

/be

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 358337 [details] [diff] [review]
fix

Looks good to me; no worries about stealing, you know this much better than I do at this point.  :-)

I don't know how much C++isms we're going to tolerate in the long term, but I think it could be worth adding some inline non-virtual methods to JSCodeSpec to centralize these bit-checking definitions.  Separate bug, of course, if this sounds like a good idea.

Comment 11

[Brendan Eich [:brendan]]

Fixed in tm:

http://hg.mozilla.org/tracemonkey/rev/45bb6c3a3be1

A cleanup bug on JSCodeSpec is overdue but I'm gonna forget to file it in a minute, and no time before then to do the deed (freeze coming up fast!). Little help?

/be

Comment 12

[Mike Shaver (:shaver emeritus)]

https://bugzilla.mozilla.org/show_bug.cgi?id=475091

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a7452c6bd155

Comment 14

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/447f1bbf9027

Comment 15

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/extensions/regress-473282.js

Comment 16

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 17

[Bob Clary [:bc] (inactive)]

test checked into 1.9.0, 1.9.1, 1.9.2, tracemonkey. 1.9.3 will get picked up in the next merge.