Last Comment Bug 454142 - Crash [@ js_TraceScopeProperty] with gczeal, setter, watch
: Crash [@ js_TraceScopeProperty] with gczeal, setter, watch
Status: VERIFIED FIXED
[sg:nse] null deref
: crash, fixed1.8.1.21, testcase, verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla1.9.1b3
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on:
Blocks: jsfunfuzz
  Show dependency treegraph
 
Reported: 2008-09-07 23:43 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2012-04-18 10:36 PDT (History)
13 users (show)
sayrer: blocking1.9.1+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
asac: wanted1.8.1.x+
asac: blocking1.8.0.next+
asac: wanted1.8.0.x+
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
gdb backtraces, normal and full (13.41 KB, text/plain)
2008-09-07 23:43 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Fix (1.16 KB, patch)
2008-11-13 14:01 PST, Blake Kaplan (:mrbkap)
brendan: review+
igor: review+
mbeltzner: approval1.9.1+
dveditz: approval1.9.0.6+
dveditz: approval1.8.1.next+
asac: approval1.8.0.next?
Details | Diff | Splinter Review
js1_5/extensions/regress-454142.js (2.20 KB, text/plain)
2008-12-04 08:30 PST, Bob Clary [:bc:]
no flags Details

Description Gary Kwong [:gkw] [:nth10sd] 2008-09-07 23:43:02 PDT
Created attachment 337398 [details]
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] 2008-09-07 23:44:39 PDT
s/nominating/filing

Should have been nominating wanted1.9.1... :-/
Comment 2 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-09-08 04:44:24 PDT
(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] 2008-09-08 05:09:57 PDT
(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 -- probably not reading bugmail closely) 2008-09-08 05:14:31 PDT
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] 2008-09-08 06:51:49 PDT
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) 2008-11-13 14:00:05 PST
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) 2008-11-13 14:01:23 PST
Created attachment 348049 [details] [diff] [review]
Fix
Comment 8 Brendan Eich [:brendan] 2008-11-13 16:13:57 PST
Comment on attachment 348049 [details] [diff] [review]
Fix

Get Igor on this case too.

/be
Comment 9 Blake Kaplan (:mrbkap) 2008-11-20 15:45:18 PST
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] 2008-11-26 21:15:40 PST
Comment on attachment 348049 [details] [diff] [review]
Fix

a191=beltzner
Comment 11 Blake Kaplan (:mrbkap) 2008-11-28 18:07:54 PST
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 2008-11-28 18:21:25 PST
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:] 2008-12-04 08:30:41 PST
Created attachment 351392 [details]
js1_5/extensions/regress-454142.js
Comment 15 Bob Clary [:bc:] 2008-12-06 04:24:51 PST
verified fixed mozilla-central, but not tracemonkey.
Comment 16 Blake Kaplan (:mrbkap) 2008-12-08 13:09:55 PST
Dan, this isn't critical -- it's a null pointer deref.
Comment 17 Daniel Veditz [:dveditz] 2008-12-09 22:37:09 PST
so we can unhide the bug, right?
Comment 18 Blake Kaplan (:mrbkap) 2008-12-10 01:20:53 PST
Yep.
Comment 19 Daniel Veditz [:dveditz] 2008-12-10 14:56:04 PST
Comment on attachment 348049 [details] [diff] [review]
Fix

Approved for 1.9.0.6, a=dveditz for release-drivers.
Comment 20 Robert Sayre 2009-01-06 20:43:59 PST
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 2009-01-12 02:45:18 PST
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:] 2009-01-14 08:22:42 PST
tests checked in http://hg.mozilla.org/mozilla-central/rev/37c7e3fe4344 and cvs
Comment 23 Al Billings [:abillings] 2009-01-14 17:42:11 PST
Bob, can you verify this fix for 1.9.0.6?
Comment 24 Bob Clary [:bc:] 2009-01-15 03:27:35 PST
v 1.9.0.6
Comment 25 Daniel Veditz [:dveditz] 2009-01-16 11:46:06 PST
Comment on attachment 348049 [details] [diff] [review]
Fix

Approved for 1.8.1.21, a=dveditz for release-drivers.
Comment 26 Alexander Sack 2009-01-22 05:19:20 PST
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: gavin@gavinsharp.com] 2009-01-22 09:03:17 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a2c5f14c8d33

Note You need to log in before you can comment on or make changes to this bug.