454142 - Crash [@ js_TraceScopeProperty] with gczeal, setter, watch

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: gdb backtraces, normal and full

this.watch("x", function(){})
delete x
gczeal(2)
this.__defineSetter__("x", function(){})

blows debug at js_TraceScopeProperty at almost null. Doesn't seem to affect opt.

Occurs with both with and without "-j" option for a js shell compiled from mozilla-central.

Since this concerns gczeal, I'm nominating this as security sensitive.

===

Console output:

$ ./js-dbg-mozTrunk-intelmac 
js> this.watch("x", function(){})
js> delete x
true
js> gczeal(2)
js> this.__defineSetter__("x", function(){})
Bus error

$ ./js-opt-mozTrunk-intelmac 
js> this.watch("x", function(){})
js> delete x
true
js> gczeal(2)
typein:3: ReferenceError: gczeal is not defined
js> this.__defineSetter__("x", function(){})
js>

Comment 1

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

s/nominating/filing

Should have been nominating wanted1.9.1... :-/

Comment 2

[Mike Shaver (:shaver emeritus)]

(In reply to comment #0)
> Doesn't seem to affect opt.

Is that just because gczeal isn't defined?

Comment 3

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

(In reply to comment #2)
> (In reply to comment #0)
> > Doesn't seem to affect opt.
> 
> Is that just because gczeal isn't defined?

How should it be defined? (I compiled both debug and opt builds from the same source, mozilla-central, with the same timestamp and no modifications)

Comment 4

[Mike Shaver (:shaver emeritus)]

It can be defined separately via JS_GC_ZEAL.  I was just asking because of

js> gczeal(2)
typein:3: ReferenceError: gczeal is not defined

in your transcript.

Comment 5

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

After some investigation, I found that opt builds crash too, but only if I remove the DEBUG ifdef lines above and below this line:

http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#2625

so I guess it can be said that opt builds crash as well...


===

$ ./js
js> this.watch("x", function(){})
js> delete x
true
js> gczeal(2)
js> this.__defineSetter__("x", function(){})
Bus error

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

This is a non-exploitable null pointer dereference. The sprop-marking code added in bug 381374 assumed that we'd be overwriting if a watchpoint already existed. This is clearly not the case.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 348049 [details] [diff] [review]
Fix

Get Igor on this case too.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 348049 [details] [diff] [review]
Fix

Not sure if we want this on the 1.9.0 branch, but it's a stability fix that's very small and very safe so it seems worth it.

Comment 10

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

Comment on attachment 348049 [details] [diff] [review]
Fix

a191=beltzner

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Checkin message: Bug 454142 - Don't assume that we're overwriting since watchpoints are not tied to the underlying sprop. r=brendan/igor a=beltzner

Comment 12

[Shawn Wilsher :sdwilsh]

I didn't see comment 11 until now.  Checkin message just has the bug number and reviewers in it.

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

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-454142.js

Comment 15

[Bob Clary [:bc] (inactive)]

verified fixed mozilla-central, but not tracemonkey.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Dan, this isn't critical -- it's a null pointer deref.

Comment 17

[Daniel Veditz [:dveditz]]

so we can unhide the bug, right?

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Yep.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 348049 [details] [diff] [review]
Fix

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 20

[Robert Sayre]

fixed for 1.9.0.6

Checking in js/src/jsscope.c;
/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.89; previous revision: 3.88
done

Comment 21

[Alexander Sack]

couldn't get gczeal enabled in 1.8.1, so couldnt check the testcase. But code patched here exists on 1.8 branches and seems to make sense. setting flags ...

Comment 22

[Bob Clary [:bc] (inactive)]

tests checked in http://hg.mozilla.org/mozilla-central/rev/37c7e3fe4344 and cvs

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Bob, can you verify this fix for 1.9.0.6?

Comment 24

[Bob Clary [:bc] (inactive)]

v 1.9.0.6

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 348049 [details] [diff] [review]
Fix

Approved for 1.8.1.21, a=dveditz for release-drivers.

Comment 26

[Alexander Sack]

fixed for 1.8.1.21

/cvsroot/mozilla/js/src/jsscope.c,v  <--  jsscope.c
new revision: 3.45.20.7; previous revision: 3.45.20.6

Comment 27

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a2c5f14c8d33