507904 - "Assertion failure: sprop->setter != js_watch_set || pobj != obj, at ../jsdbgapi.cpp"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(function () {
    (function () {
        __proto__.watch("x", function () {})
    })()
})()
while ([].some.watch("x", function () {}))[]
try {
    (function () {
        [].some.unwatch("x")
    })()
} catch(e) {}
(function () {
    [].some.watch("x", function () {})
})()


asserts js debug shell on TM branch without -j at Assertion failure: sprop->setter != js_watch_set || pobj != obj, at ../jsdbgapi.cpp:836

This is most likely a regression between the CVS checkouts of the js shell between 01012003 and 01012006, which means it is likely to also assert on the 1.9.0 and 1.9.1 branches.

In short, another possible super-ancient bug that needs its security impact accessed first.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #0)
> This is most likely a regression between the CVS checkouts of the js shell
> between 01012003 and 01012006, which means it is likely to also assert on the
> 1.9.0 and 1.9.1 branches.

Works with CVS checkout from 2005-04-17 11:30

Assertion fails with:
Assertion failure: sprop->setter != js_watch_set, at jsdbgapi.c:532
with CVS checkout from 2005-04-17 11:32

(I am not sure when the assertion morphed into the one in this bug)

A manual CVS bisect shows bug 290592 may be related:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-04-17+11%3A30%3A00&maxdate=2005-04-17+11%3A32%3A00&cvsroot=%2Fcvsroot

Checkin message:

Bug 290592: Add |forEach|, |indexOf|, |filter|, |map|, |some| and |every| to
Array.prototype, for greater Lispy righteousness. r+a=brendan.

Comment 2

[Jason Orendorff [:jorendorff]]

That's just the patch that introduced [].some. It's probably not relevant. The bug applies to other functions too:

Function.prototype.watch("x", function () {});
var f = Math.sin;  // or `[].some` or `function(){}`
f.watch("x", function () {});
f.unwatch("x");
f.watch("x", function () {});

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #2)
> That's just the patch that introduced [].some. It's probably not relevant. The
> bug applies to other functions too:
> 
> Function.prototype.watch("x", function () {});
> var f = Math.sin;  // or `[].some` or `function(){}`
> f.watch("x", function () {});
> f.unwatch("x");
> f.watch("x", function () {});

In that case, the assertion goes back way earlier.

$ pwd
/Users/skywalker/Desktop/10242002-1802/mozilla/js/src/Darwin_DBG.OBJ
$ ./js
js> Function.prototype.watch("x", function () {});
js> var f = function () {};
js> f.watch("x", function () {});
Assertion failure: sprop->setter != js_watch_set, at jsdbgapi.c:512
Abort trap

With a CVS js executable dating back to 24th October 2002 at 18:02, the minute right after js compilation got enabled on Mac OS X, the testcase still asserts. Let me try on Linux and see if I can get an even earlier window.

This feels like working on the Egyptian pyramids!

Comment 4

[Jason Orendorff [:jorendorff]]

That looks like it might be an older and worse bug. Current TM tip doesn't assert if you paste just those 3 lines in.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Does not assert:

CVS checkout from 02/22/2002 19:48 PST

Asserts:

CVS checkout from 02/22/2002 19:50 PST

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fjs%2Fsrc%2F&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2002-02-22+19%3A48%3A00&maxdate=2002-02-22+19%3A50%3A00&cvsroot=%2Fcvsroot

Bonsai message:

Major footprint/perf win: share property state using a tree whose root represents empty scopes, and whose non-root nodes represent scopes with properties added in order from the root to that node; to use double hashing to map these tree-paths for per-scope property lookup; and avoid locking where possible through immutability and mostly-benign&rare race tolerance (62164, r=shaver, sr=jband).

So a patch for bug 62164 that landed in Mozilla 0.9.9 might have caused this. (or a bug that is discovered some 7 years, 5 months, 10 days, 10 hours, 59 minutes, 23 seconds later [i.e. ~7.5 years], but I digress...)

Nominating for every single branch alive (or available for nomination).

===

~/Desktop/1948-02222002/mozilla/js/src/Linux_All_DBG.OBJ$ ./js
js> Function.prototype.watch("x", function () {});
js> var f = function () {};
js> f.watch("x", function () {});
js> f.unwatch("x");
js> f.watch("x", function () {});
js> 

~/Desktop/1950-02222002/mozilla/js/src/Linux_All_DBG.OBJ$ ./js
js> Function.prototype.watch("x", function () {});
js> var f = function () {};
js> f.watch("x", function () {});
Assertion failure: sprop->setter != js_watch_set, at jsdbgapi.c:451
Aborted

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #4)
> That looks like it might be an older and worse bug. Current TM tip doesn't
> assert if you paste just those 3 lines in.

Bug 361558 merely changed that assertion name, a one-liner fix. See:

https://bug361558.bugzilla.mozilla.org/attachment.cgi?id=246343

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #6)
> (In reply to comment #4)
> > That looks like it might be an older and worse bug. Current TM tip doesn't
> > assert if you paste just those 3 lines in.
> 
> Bug 361558 merely changed that assertion name, a one-liner fix. See:
> 
> https://bug361558.bugzilla.mozilla.org/attachment.cgi?id=246343

That said, I don't know what caused the testcase to require 5 lines as in comment #2, instead of just those 3 lines.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to comment #5)
> (or a bug that is discovered some 7 years, 5 months, 10 days, 10 hours, 59
> minutes, 23 seconds later [i.e. ~7.5 years], but I digress...)
> 
> Nominating for every single branch alive (or available for nomination).

A 7.5 year old bug has got to be the perfect embodiment of a non-blocker. If it's a security bug we should prioritize higher, but still not a blocker. Will approve when patched.

Comment 9

[Jesse Ruderman]

Is this likely to be a security hole?

Comment 10

[Brandon Sterne (:bsterne)]

Again, why is this marked security sensitive?

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #10)
> Again, why is this marked security sensitive?

Some very old asserts have been previously shown (by Igor) to be security issues, so this one was similarly marked just-in-case. See bug 427196 as an example.

Comment 12

[Brandon Sterne (:bsterne)]

We're trying to assign a severity rating to this bug.  I appreciate that other asserts have proven to be security holes in the past.  That doesn't mean that _all_ asserts are security bugs.  This is a different assertion than the one in bug 427196.  We're trying to evaluate the risk of _this_ bug, and Igor is probably the best person to provide that evaluation.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #12)
> We're trying to evaluate the risk of _this_ bug, and Igor is
> probably the best person to provide that evaluation.

Agreed. Which is why Igor is assigned pending the evaluation. I don't know enough to be able to judge this old assert - so it's security-sensitive as a precautionary measure. Please feel free to open up if it isn't. :)

Comment 14

[Brandon Sterne (:bsterne)]

I'll give Igor another couple of days to provide a reason why this needs to stay hidden.  Otherwise, we plan to open it up.

Comment 15

[Igor Bukanov]

I will check this bug during this week for sure, so please do not open it yet.

Comment 16

[Daniel Veditz [:dveditz]]

Assuming worst-case for now.

Comment 17

[Igor Bukanov]

Here is smaller test case that demonstrates the issue:

var o = {};
Object.prototype.watch("x", function () {});
o.watch("x", function () {});
o.unwatch("x");
o.watch("x", function () {});

The reason for the bug is the property cloning code in JS_SetWatchPoint, http://hg.mozilla.org/tracemonkey/file/61bce35370b7/js/src/jsdbgapi.cpp#l877 .

That code does not check that property coming from the prototype is been watched. Thus the first o.watch call above wraps js_watch_set comming from the prototype by js_watch_set again. Then o.unwatch, when restoring the setter, would write js_watch_set to the setter to js_watch_set while removing the watchpoint itself. Then the second o.watch asserts.

Now, as far as I can see the assert is harmless. js_watch_set checks for a watchpoint structure. If it cannot find one, it would do nothing. So we are talking at worst about broken semantics in code that uses the watch points.

Comment 18

[Igor Bukanov]

Attachment: assert removal

Comment 19

[Damon Sicore (:damons)]

Since we're just removing the assert, this isn't an sg:crit, right?  Removing from the whiteboard.  Please correct me if I'm wrong.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 440193 [details] [diff] [review]
assert removal

Should we have a followup to avoid double-wrapping?

/be

Comment 21

[Igor Bukanov]

(In reply to comment #20)
> (From update of attachment 440193 [details] [diff] [review])
> Should we have a followup to avoid double-wrapping?

I am not sure that it is worth the efforts. We do not nest object and the debugger locks. So when we can check that the setter that is been wrapped is js_watch_set another thread could have changed that. So a fix would be rather involved. Plus many of users of watch points could have been replaced with proxies I suggest do nothing here.

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Igor checks in the patch to TM, thanks!

http://hg.mozilla.org/tracemonkey/rev/7facc3422577

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7facc3422577