631488 - Crash [@ xpc::XrayWrapper<JSCrossCompartmentWrapper>::get] [@ js::JSProxyHandler::get] navigating closed window

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase (must be local)

Related to bug 628599?

Comment 1

[Jesse Ruderman]

Attachment: crash report

Comment 2

[Andreas Gal :gal]

Why is this sg:crit?

Comment 3

[Jesse Ruderman]

Because the crash stack trace has a random, invalid address at the top.

Comment 4

[Andreas Gal :gal]

10b looks like a near null crash to me with a largish VMT (proxy handler). I will look into it.

Comment 5

[Johnny Stenback (:jst)]

Blocking.

Comment 6

[Andreas Gal :gal]

This doesn't reproduce for me with b10. Trying in a debug build.

Comment 7

[Andreas Gal :gal]

I tried with TM tip. This is definitely no longer crashing. I think I have an idea what fixed this.

In unrelated news, we leak like a sieve on this test case. With quick clicks I can get up to 1000 DOM windows easily. We GC and CC, but each time we only go down to 300 DOM windows or so. Most doc shells get destroyed, but nowhere near all windows. Weird.

--DOCSHELL 0x7fffca67c000 == 9
--DOCSHELL 0x7fffcc839000 == 8
--DOCSHELL 0x7fffcc414000 == 7
--DOCSHELL 0x7fffcc27c800 == 6
--DOCSHELL 0x7fffcb003800 == 5
--DOMWINDOW == 495 (0x7fffccc6a078) [serial = 1990] [outer = 0x7fffcc414c00] [url = about:blank]
--DOMWINDOW == 494 (0x7fffcc414c78) [serial = 1986] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 493 (0x7fffccc68c78) [serial = 1989] [outer = 0x7fffcc27d400] [url = about:blank]

Comment 8

[Andreas Gal :gal]

Blake says this is related to us not properly filtering/censoring setters that are JSPropertyOps. Though I thought we fixed something there recently, too.

Comment 9

[Andreas Gal :gal]

bug 629331 might be related

Comment 10

[Andreas Gal :gal]

Testing again. mrbkap says this did reproduce for him with that patch applied

Comment 11

[Andreas Gal :gal]

Ok, just crashed.

Comment 12

[Andreas Gal :gal]

Attachment: patch

Comment 13

[Andreas Gal :gal]

I stand corrected. Jesse was right. This is exploitable. An object is used as JSPropertyOp.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 510424 [details] [diff] [review]
patch

>diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp
>--- a/js/src/jsproxy.cpp
>+++ b/js/src/jsproxy.cpp
>@@ -173,26 +173,26 @@ JSProxyHandler::set(JSContext *cx, JSObj
>                 return false;
>             if (desc.attrs & JSPROP_SHARED)
>                 return true;
>         }
>         if (!desc.getter)
>             desc.getter = PropertyStub;
>         if (!desc.setter)
>             desc.setter = PropertyStub;
>-        /* fall through */
>-    } else {
>-        /* Pick up the class getter/setter. */
>-        desc.getter = desc.setter = NULL;
>+        return defineProperty(cx, receiver, id, &desc);

The - lines should if(A){B}else{C;return}D, which is a bad pattern. Any more like it? Please fix if so. Should be if(A){B;D}else{C} or if(!A){C;return}B;D.

>     }
> 
>     desc.obj = receiver;
>     desc.value = *vp;
>     desc.attrs = JSPROP_ENUMERATE;
>     desc.shortid = 0;

Blank line before comment taking one or more lines unless preceded by {.

>+    /* Pick up the class getter/setter. */
>+    desc.getter =
>+    desc.setter = NULL;

All on one line please.

/be

Comment 15

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/985782685741

Comment 17

[Jesse Ruderman]

Andreas, did your patch fix both the leak and the crash? Or do we need a new bug on the leak?

Comment 18

[Andreas Gal :gal]

I didn't fix the leak.

Comment 19

[Jesse Ruderman]

Can you still reproduce the leak?

Comment 20

[Andreas Gal :gal]

Bugs are on file for the leak. We are done here.

Comment 21

[Johnny Stenback (:jst)]

http://hg.mozilla.org/mozilla-central/rev/c76d7a190b12