Closed Bug 636697 Opened 9 years ago Closed 9 years ago

Crash [@ js_watch_set]

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla5
Tracking Status
firefox5 --- fixed
blocking2.0 --- .x+
status2.0 --- wontfix
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: gkw, Assigned: jorendorff)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [ccbr][sg:dos][fixed-in-tracemonkey])

Crash Data

Attachments

(2 files, 2 obsolete files)

o0 = Iterator.prototype
o7 = {}
function f0(o) {
    delete o.p0
    try {
        o.__proto__ = o7
    } catch (e) {}
    o.p0 = function () {}
    o.watch('p0', function () {})
}
for (i = 0; i < 9; i++) {
    o7.__defineSetter__('p0', function () {});
    f0(o0);
    f0(o7)
}

crashes js debug and opt shell on TM changeset 9d8a96a61d71 without -m nor -j at js_watch_set, and also crashes Firefox 4 Beta 12 on Windows 7 at bp-d5ac9a6e-dc83-480e-a670-034ed2110225

Nominating and s-s just-in-case, assuming [sg:critical?] until further diagnosed.
Might be null-deref, tested on TM changeset 725b8cce5c72 in Linux 32-bit opt shell:

(gdb) x/i $pc
=> 0x808e605 <_ZL20js_watch_set_wrapperP9JSContextjPN2js5ValueE+341>:	movzbl 0x1d(%edx),%eax
(gdb) x/b $edx
0x0:	Cannot access memory at address 0x0
(gdb) x/b $eax
0x838dee0:	0x00

Valgrind with --smc-check=full from the same shell:

==16207== Invalid read of size 1
==16207==    at 0x808E605: js_watch_set_wrapper(JSContext*, unsigned int, js::Value*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CAA28: js::Invoke(JSContext*, js::CallArgs const&, unsigned int) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CB677: js::ExternalInvoke(JSContext*, js::Value const&, js::Value const&, unsigned int, js::Value*, js::Value*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CB731: js::ExternalGetOrSet(JSContext*, JSObject*, int, js::Value const&, JSAccessMode, unsigned int, js::Value*, js::Value*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80F5DA9: js::Shape::set(JSContext*, JSObject*, bool, js::Value*) const (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80EA0C0: js_SetPropertyHelper(JSContext*, JSObject*, int, unsigned int, js::Value*, int) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x82808DC: js::Interpret(JSContext*, JSStackFrame*, unsigned int, JSInterpMode) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CA80F: js::RunScript(JSContext*, JSScript*, JSStackFrame*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80CBE98: js::Execute(JSContext*, JSObject*, JSScript*, JSStackFrame*, unsigned int, js::Value*) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x805B6B9: JS_ExecuteScript (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x804F957: Process(JSContext*, JSObject*, char*, int) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==    by 0x80508CB: Shell(JSContext*, int, char**, char**) (in /home/fuzz1/Desktop/jsfunfuzz-dbg-32-tm-62755-725b8cce5c72/js-opt-32-tm-linux)
==16207==  Address 0x1d is not stack'd, malloc'd or (recently) free'd
OS: Windows 7 → All
Hardware: x86 → All
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   62375:589bb166be02
user:        Jason Orendorff
date:        Tue Feb 08 16:09:33 2011 -0600
summary:     Bug 627984 - Tighten up assertions in JSObject::methodReadBarrier. r=brendan.
Blocks: 627984
Assignee: general → jorendorff
It is an NPE.
Group: core-security
blocking2.0: ? → .x+
Whiteboard: [ccbr][sg:critical?] → [ccbr][sg:dos]
Reduced:

var a = {set p(x) {}};
a.watch('p', function () {});
var b = Object.create(a);
b.watch('p', function () {});
delete b.p;
b.p = 0;

It seems there's just be a big hole in watchpoints where deleted accessor properties are concerned, because:

  var hit = 0;
  var a = {set p(x) { hit++; }};
  a.watch('p', function (a, b, c) {});
  var b = Object.create(a);
  b.p = 0;                                // <=== setter is skipped!
  assertEq(hit, 1);                       // FAIL

Note that I'm not complaining that a's watch-handler is skipped. That's by design. But the watchpoint on a.p shouldn't cause the *setter* to be skipped!

I'm not going to try to fix that hole. But I will fix the null crash.
Attached patch v1 (obsolete) — Splinter Review
With this patch, we behave the same in the case where b.p is watched and deleted as in the case where there is no b.p.

Easily triggered null crashes are bad, so I'd like to get approval and land this.
Attachment #515756 - Flags: review?(brendan)
Attachment #515756 - Attachment is obsolete: true
Attachment #515756 - Flags: review?(brendan)
Attachment #515758 - Flags: review?(brendan)
(In reply to comment #6)
> Created attachment 515758 [details] [diff] [review]
> v2 - Same as v1 but with the test attached, d'oh

With this patch, the following testcase seems to hang continuously. Intended?

o9 = /a/gi;
o59 = /abc*def/.__proto__;
function f17(o) {
    delete o.toString;
}
function f26(o) {
    var _var_ = o;
    o.toString = function () {};
    try {
        _var_.__defineSetter__('toString', function (v) {});
        o.watch('toString', function () {});
    } catch (e) {}
}
for (var i = 0; i < 15; i++)
{
    f26(o59);
    f26(o9);
    f17(o9);
}
Not intended. Looking.
Attachment #515758 - Attachment is obsolete: true
Attachment #515758 - Flags: review?(brendan)
Attachment #515949 - Flags: review?(brendan)
Comment on attachment 515949 [details] [diff] [review]
v3 - same as v2, but drop the watchpoint properly

>diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp
>--- a/js/src/jsdbgapi.cpp
>+++ b/js/src/jsdbgapi.cpp
>@@ -713,23 +713,32 @@ js_watch_set(JSContext *cx, JSObject *ob
>     for (JSWatchPoint *wp = (JSWatchPoint *)rt->watchPointList.next;
>          &wp->links != &rt->watchPointList;
>          wp = (JSWatchPoint *)wp->links.next) {
>         const Shape *shape = wp->shape;
>         if (wp->object == obj && SHAPE_USERID(shape) == id && !(wp->flags & JSWP_HELD)) {
>             wp->flags |= JSWP_HELD;
>             DBG_UNLOCK(rt);
> 
>+            bool ok;
>             jsid propid = shape->id;
>             shape = obj->nativeLookup(propid);
>+            if (!shape) {
>+                /*
>+                 * This happens if the watched property has been deleted, but a
>+                 * prototype has a watched accessor property with the same
>+                 * name. See bug 636697.
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

D'oh! Suggest "of the same name.\n" to make it fit.

r=me with that, thanks.

/be
Attachment #515949 - Flags: review?(brendan) → review+
Attachment #515949 - Flags: approval2.0?
Attachment #515949 - Flags: approval2.0? → approval2.0+
Backed out. Breaks Linux builds.

http://hg.mozilla.org/mozilla-central/rev/99e112c8616d

Revoking approval as per discussion during the triage meeting (anything that wasn't ready to go in right there got dropped, this clearly wasn't).
Attachment #515949 - Flags: approval2.0+
(its already .x, so we should be good without renom I think)
Yes.
Attached patch v4Splinter Review
Fixed the warnings and relanded in tracemonkey only.

http://hg.mozilla.org/tracemonkey/rev/1d7f280804a1
Whiteboard: [ccbr][sg:dos] → [ccbr][sg:dos][fixed-in-tracemonkey]
Original landing: http://hg.mozilla.org/mozilla-central/rev/8b3a403a6d26
Backout: http://hg.mozilla.org/mozilla-central/rev/99e112c8616d
Relanding: http://hg.mozilla.org/mozilla-central/rev/1d7f280804a1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla5
Assuming this doesn't affect 1.9.2.x since it's a recent regression.
Crash Signature: [@ js_watch_set]
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-636697.js.
Flags: in-testsuite+
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.