Crash [@ js_watch_set]

VERIFIED FIXED in Firefox 5

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: gkw, Assigned: jorendorff)

Tracking

({crash, regression, testcase})

Trunk
mozilla5
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5 fixed, blocking2.0 .x+, status2.0 wontfix, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [ccbr][sg:dos][fixed-in-tracemonkey], crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
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
(Reporter)

Comment 2

7 years ago
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)

Updated

7 years ago
Assignee: general → jorendorff
It is an NPE.
Group: core-security
blocking2.0: ? → .x+
Whiteboard: [ccbr][sg:critical?] → [ccbr][sg:dos]
(Assignee)

Comment 4

7 years ago
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.
(Assignee)

Comment 5

7 years ago
Created attachment 515756 [details] [diff] [review]
v1

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)
(Assignee)

Comment 6

7 years ago
Created attachment 515758 [details] [diff] [review]
v2 - Same as v1 but with the test attached, d'oh
Attachment #515756 - Attachment is obsolete: true
Attachment #515756 - Flags: review?(brendan)
Attachment #515758 - Flags: review?(brendan)
(Reporter)

Comment 7

7 years ago
(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);
}
(Assignee)

Comment 8

7 years ago
Not intended. Looking.
(Assignee)

Comment 9

7 years ago
Created attachment 515949 [details] [diff] [review]
v3 - same as v2, but drop the watchpoint properly
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+
(Reporter)

Updated

7 years ago
Attachment #515949 - Flags: approval2.0?
Attachment #515949 - Flags: approval2.0? → approval2.0+

Comment 11

7 years ago
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).

Updated

7 years ago
Attachment #515949 - Flags: approval2.0+

Comment 12

7 years ago
(its already .x, so we should be good without renom I think)
(Assignee)

Comment 13

7 years ago
Yes.
(Assignee)

Comment 15

7 years ago
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]
(Reporter)

Comment 16

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
status-firefox5: --- → fixed
Target Milestone: --- → mozilla5
status2.0: --- → wanted
Assuming this doesn't affect 1.9.2.x since it's a recent regression.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
status2.0: wanted → wontfix
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+
(Reporter)

Comment 19

5 years ago
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.