382631 - Creating proxy of "this" in JavaScript using nsProxyObjectManager::GetProxyForObject causes random memory rewrite

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Michal Novotny [:michal]]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.5; Linux) KHTML/3.5.6 (like Gecko)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1) Gecko/20070205 BonEcho/2.0

In short:
When getProxyForObject (for "this") is called from JS, then on line
http://lxr.mozilla.org/mozilla1.8/source/js/src/xpconnect/src/xpcwrappednative.cpp#1482
is not method GetJSObject(&jso) called directly but through proxy (PROXY_ASYNC | PROXY_ALWAYS). When posted event is served variable jso that was on stack is no longer valid and unknown data are rewritten. This can cause crash or other unexpected behavior. More details are in Steps to reproduce...

Reproducible: Always

Steps to Reproduce:
Download attached test.zip, unzip and load test.html in browser.
Attach gdb to firefox-bin.
Set breakpoint where proxyevent is created

(gdb) b PL_InitEvent if (handler == &EventHandler)
Breakpoint 1 at 0x2aaaab126185: file /opt/src/cvs/mozilla/xpcom/threads/plevent.c, line 659.
(gdb) c
Continuing.

Then click on "Register observer" button. And gdb should stop.

Breakpoint 1, PL_InitEvent (self=0x1441170, owner=0x14411d0, handler=0x2aaaab1303f2 <EventHandler>, 
    destructor=0x2aaaab13136a <DestroyHandler>) at /opt/src/cvs/mozilla/xpcom/threads/plevent.c:659
659         PR_INIT_CLIST(&self->link);

Make sure that this is the right event (note that following expression works for linux x86_64):

(gdb) x/wa *(PRUint64 *)((*(PRUint64 *)((nsProxyObject *)((nsProxyObjectCallInfo *)owner).mOwner).mRealObject) + (((nsProxyObjectCallInfo *)owner).mMethodIndex)*8)
0x2aaaafa69b9a <_ZThn8_N14nsXPCWrappedJS11GetJSObjectEPP8JSObject>:     0xfffffffff8c78348

There is 1 parameter:

(gdb) p ((nsProxyObjectCallInfo *)owner).mParameterCount
$1 = 1

And it is void* (type 0x80 | 0xd) and points to 0x7fffe1e3cc08 :

(gdb) p/x (((nsProxyObjectCallInfo *)owner).mParameterList)[0]
$1 = {<nsXPTCMiniVariant> = {val = {i8 = 0x0, i16 = 0x0, i32 = 0x0, i64 = 0x0, u8 = 0x0, u16 = 0x0, u32 = 0x0, 
      u64 = 0x0, f = 0x0, d = 0x0, b = 0x0, c = 0x0, wc = 0x0, p = 0x0}}, ptr = 0x7fffe1e3cc08, 
  type = {<XPTTypeDescriptorPrefix> = {flags = 0x8d}, <No data fields>}, flags = 0x1}


On frame 5 is call to GetJSObject(&jso) :

(gdb) bt 8
#0  PL_InitEvent (self=0x1441170, owner=0x14411d0, handler=0x2aaaab1303f2 <EventHandler>, 
    destructor=0x2aaaab13136a <DestroyHandler>) at /opt/src/cvs/mozilla/xpcom/threads/plevent.c:659
#1  0x00002aaaab13180f in nsProxyObject::Post (this=0x1228990, methodIndex=3, methodInfo=0x7aef98, 
    params=0x7fffe1e3ca40, interfaceInfo=0x115ae50) at /opt/src/cvs/mozilla/xpcom/proxy/src/nsProxyEvent.cpp:507
#2  0x00002aaaab133a1e in nsProxyEventObject::CallMethod (this=0x13dfef0, methodIndex=3, info=0x7aef98, 
    params=0x7fffe1e3ca40) at /opt/src/cvs/mozilla/xpcom/proxy/src/nsProxyEventObject.cpp:559
#3  0x00002aaaab153ed2 in PrepareAndDispatch (self=0x13dfef0, methodIndex=3, args=0x7fffe1e3cbc0, 
    gpregs=0x7fffe1e3cb40, fpregs=0x7fffe1e3cb70)
    at /opt/src/cvs/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:156
#4  0x00002aaaab152f23 in SharedStub () at ../../../../../../dist/include/xpcom/xptcstubsdef.inc:255
#5  0x00002aaaafa77100 in XPCWrappedNative::InitTearOff (this=0x143fd80, ccx=@0x7fffe1e3d6d0, aTearOff=0x143fdc0, 
    aInterface=0x142f1b0, needJSObject=0) at /opt/src/cvs/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:1482
#6  0x00002aaaafa7770a in XPCWrappedNative::FindTearOff (this=0x143fd80, ccx=@0x7fffe1e3d6d0, aInterface=0x142f1b0, 
    needJSObject=0, pError=0x7fffe1e3ce9c) at /opt/src/cvs/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:1396
#7  0x00002aaaafa7a36b in XPCWrappedNative::GetNewOrUsed (ccx=@0x7fffe1e3d6d0, Object=0xd92dd0, Scope=0x109b820, 
    Interface=0x142f1b0, isGlobal=0, resultWrapper=0x7fffe1e3cf98)
    at /opt/src/cvs/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:411
(More stack frames follow...)
(gdb) f 5
#5  0x00002aaaafa77100 in XPCWrappedNative::InitTearOff (this=0x143fd80, ccx=@0x7fffe1e3d6d0, aTearOff=0x143fdc0, 
    aInterface=0x142f1b0, needJSObject=0) at /opt/src/cvs/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:1482
1482                if(NS_SUCCEEDED(wrappedJS->GetJSObject(&jso)) &&
(gdb) l
1477
1478            nsCOMPtr<nsIXPConnectWrappedJS> wrappedJS(do_QueryInterface(obj));
1479            if(wrappedJS)
1480            {
1481                JSObject* jso = nsnull;
1482                if(NS_SUCCEEDED(wrappedJS->GetJSObject(&jso)) &&
1483                   jso == GetFlatJSObject())
1484                {
1485                    // The implementing JSObject is the same as ours! Just say OK
1486                    // without actually extending the set.
(gdb) p &jso
$4 = (JSObject **) 0x7fffe1e3cc08

Now see when the GetJSObject() is really called and who access the memory in the meantime...

(gdb) watch *0x7fffe1e3cc08
Hardware watchpoint 2: *140736983190536
(gdb) b EventHandler
Breakpoint 3 at 0x2aaaab130403: file /opt/src/cvs/mozilla/xpcom/proxy/src/nsProxyEvent.cpp, line 550.
(gdb) f 4
#4  0x00002aaaab152f23 in SharedStub () at ../../../../../../dist/include/xpcom/xptcstubsdef.inc:255
255     SENTINEL_ENTRY(3)
(gdb) fin
Run till exit from #4  0x00002aaaab152f23 in SharedStub () at ../../../../../../dist/include/xpcom/xptcstubsdef.inc:255
[Thread 1105209664 (LWP 31184) exited]
[Thread 1115699520 (LWP 31185) exited]
[Thread 1126189376 (LWP 31186) exited]
[Thread 1136679232 (LWP 31187) exited]
[Thread 1168148800 (LWP 31191) exited]
0x00002aaaafa77100 in XPCWrappedNative::InitTearOff (this=0x143fd80, ccx=@0x7fffe1e3d6d0, aTearOff=0x143fdc0, 
    aInterface=0x142f1b0, needJSObject=0) at /opt/src/cvs/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:1482
1482                if(NS_SUCCEEDED(wrappedJS->GetJSObject(&jso)) &&
(gdb) p jso
$5 = (JSObject *) 0x0

Of course jso is NULL now. I don't know what it can cause in XPCWrappedNative::InitTearOff()

(gdb) c
Continuing.
Hardware watchpoint 2: *140736983190536

Old value = 0
New value = -1419821983
0x00002aaaab5e6b58 in PR_GetCurrentThread@plt () from ./libnspr4.so
Current language:  auto; currently c
(gdb) 
Continuing.
Hardware watchpoint 2: *140736983190536

Old value = -1419821983
New value = 17301248
0x00000032fd6082aa in pthread_mutex_lock () from /lib64/libpthread.so.0
(gdb) 
Continuing.
Hardware watchpoint 2: *140736983190536

Old value = 17301248
New value = 6502944
PR_GetCurrentThread () at /opt/src/cvs/mozilla/nsprpub/pr/src/pthreads/ptthread.c:632
632         if (NULL == thred) thred = pt_AttachThread();
(gdb) 

There were hundreds of access to the memory so I disabled the watch...

Breakpoint 3, EventHandler (self=0x1441170) at /opt/src/cvs/mozilla/xpcom/proxy/src/nsProxyEvent.cpp:550
550         nsProxyObjectCallInfo *info = (nsProxyObjectCallInfo*)PL_GetEventOwner(self);

This is our event because self == 0x1441170.

(gdb) ena 2
(gdb) c
Continuing.
Hardware watchpoint 2: *140736983190536

Old value = 0
New value = 19897088
0x00002aaaafa68329 in nsXPCWrappedJS::GetJSObject (this=0xd085b0, aJSObj=0x7fffe1e3cc08)
    at /opt/src/cvs/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:209
209         if(!(*aJSObj = mJSObj))

Here is finally called the GetJSObject() method. If we were lucky the memory at 0x7fffe1e3cc08 was not in use right now.

Comment 1

[Michal Novotny [:michal]]

Attachment: script that is used in "steps to reproduce"

Comment 2

[Honza Bambas (:mayhemer)]

This seems to be serious problem. There are many bugs about crashes in JS interpreter/XPConnect code that might be related to this bug.

If there is at least some way to add assertion checking that the object is not async proxy would be good.

Comment 3

[timeless]

bug 128560 comes to mind.

Comment 4

[timeless]

Attachment: this is probably right [Checkin: Comment 27]

Comment 5

[Robert Sayre]

blocking to figure this out.

Comment 6

[David Bradley]

I'm not so sure what to believe, the warning or the code without digging into this more. The warning talks about out parameter, but if you read the Doug T's bug comments he's specifically targeting dipper types not out. I'm not sure we want to turn off out parameters for this, or at least I'm not willing to say go ahead without knowing the impact.

Comment 7

[timeless]

the impact is fairly drastic. it'd be much better if we replaced the pointers with our own and did copying later if we can.

Comment 8

[Doug Turner (:dougt)]

does this crash happen if the proxied object is passed into JS instead of being created in JS?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to comment #7)
> the impact is fairly drastic. it'd be much better if we replaced the pointers
> with our own and did copying later if we can.
> 

It is, it could be compared to int* func() {int a; return &a;}. I don't believe this could be permitted any way - even replacing the pointers. Result value could not be stored and is undefined when we return from the proxy transparent method call (which is just an event post).

(In reply to comment #8)
> does this crash happen if the proxied object is passed into JS instead of being
> created in JS?
> 

You mean to create the proxy in C++, pass it via e.g. attribute getter to JS and use it in JS? I tested this. 

My C++ component returns proxy to nsIIOService back to JS. Then in JS script I call its allowPort method and the result is not stored correctly (returns false for "http" and 80). It doesn't crash, but no one knows where the result is actually stored.

Comment 10

[timeless]

ok, for people who want to read stuff, http://developer.mozilla.org/en/docs/nsISupports_proxies

is the thing that clearly says async proxies will crash.

a short summary of discussions with dougt (mostly my opinions, dougt can add his own):

1. xpcom proxies can and should be "fixed" so that they don't crash (merely by moving the out variables to allocated+cleaned up space used by inout and friends)
2. xpconnect and friends need to be able to ask proxy objects what kind they are (this is trivially added by providing a proper interface [nsIProxyObjectInfo] to them, there's a psuedo interface already)
3. i believe the goals of async proxies are useless, they seem to be mostly about avoiding reentrancy for the original proxier. However, if the proxier ever calls a javascript object, then the proxier can easily suffer from reentrancy (this has always been the case, and i've hit these deaths for many years -- about as long as the deaths described here, which dougt remembers from a coworker of mine at Cenzic). as such, I'm fairly tempted to make ASYNC be ignored by the proxy code and converted into a warning that says "If you are trying to avoid reentrancy, please use nsIThreadManager and keep in mind that if you ever touch a javascript object you'll risk being reentered anyway. Otherwise just fix your code to handle reentrancy and remove the async flag."
4. it /might/ be possible for xpconnect to use a [notxpcom]nsISupports nsIProxyObjectInfo.getWrappedObject(), or it /might/ be possible for xpconnect to special case POM and provide its own wrapper objects, because what xpconnect needs to do isn't really the same as what everyone else needs to do.

Basically, xpconnect async objects need to provide async dispatch while providing sync introspection.

Comment 11

[timeless]

for people wondering who's using async and likely to be bitten, i think probably:
allpeers, cenzic, democracyplayer, fireshare, flickr.

Comment 12

[Damon Sicore (:damons)]

Johnny, can you take a look at this?

Comment 13

[timeless]

beltzner: thanks for triaging, but sometimes bugs are appropriately triaged, and even owned.

Comment 14

[timeless]

btw, for consumers looking for a workaround, you should be able to take your JS object ("this" in the example below), and do the following:

SIP=Components.Constructor("@mozilla.org/supports-interface-pointer;1","nsISupportsInterfacePointer");
sip=new SIP;sip.data=this;
var proxy = PM.getProxyForObject(uiQueue, Components.interfaces.nsIObserver, sip.data, 6);

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 304075 [details] [diff] [review]
this is probably right
[Checkin: Comment 27]

this is preventing out parameters being proxied.

this sounds fine, but is a change in API behavior, isnt it? Do we care?

Did you check the tree to see if we have current users?

Comment 16

[timeless]

dougt: any such callers die today. i don't mind this change.

fwiw, the sip thing won't work. it doesn't actually do what i wanted. and i should know better.

Sorry, for reasons which will not be described in this bug, I am not going to list my analysis here.

I will only note random things which aren't relevant and aren't serious enough for me to file a bug at this time.
 nsLDAPChannel::OnLDAPSearchEntry(nsILDAPMessage *aMessage)
  rv = mListener->OnDataAvailable(this, mResponseContext, mReadPipeIn, 
                                  mReadPipeOffset, entryLength);
is scary, I believe the code assumes async proxies fire in sequence. I'm not sure how guaranteed that is.

I think that a number of our classes have messed up the other flags and how to use proxies in general, especially a number of callers aren't using PROXY_ALWAYS when I believe they really want to.

as for changing our API, I claim that the scary block in 
http://developer.mozilla.org/en/docs/nsISupports_proxies

is a sufficient contract. We claimed it would blow up. Now it just blows up w/ an RV instead of a Crash:

Warning about PROXY_ASYNC:

You must take very special care when using this flag. If the calling thread goes away, any function which accesses the calling stack will blow up. For example:

 myFoo->bar(&x)
 
 ... thread goes away ...
 
 bar(PRInt32 *x)
 {
     ...
     *x = 0;   <-----  You will blow up here.
 }

Note: the html version is prettier, and much more colorful. But I'm reproducing it here anyway because I like it.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

The documentation on the wiki page claims that a crash could happen only when the calling thread goes away.  Why is that the case?  I would expect that as soon as the calling function exits, its entire stack frame could be deallocated or reclaimed.

Comment 18

[timeless]

the documentation is sadly not perfect. it really should say stack frame not thread. feel free to fix it if you can find your devmo account (i can't find mine).

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Fixed up the documentation to be more correct.

Comment 20

[Doug Turner (:dougt)]

Comment on attachment 304075 [details] [diff] [review]
this is probably right
[Checkin: Comment 27]

this breaks someone nicely instead of allowing them to crash.

please run a full regression test before landing. if you need help, ask.

Comment 21

[Carsten Book [:Tomcat]]

For people interested in testing the patch from comment #4, i have created some tryserver builds. They can be found here -> https://build.mozilla.org/tryserver-builds/2008-04-11_07:[e-mail]-1207925495/

Comment 22

[Mike Schroepfer]

Doug/Timeless do we need further reviews on this?

Comment 23

[Doug Turner (:dougt)]

It would be good.  I am basically worried that disabling [inout] parameters will be too broad.  Are there some parameter types that are [inout] that we need to support?

http://lxr.mozilla.org/mozilla/source/editor/idl/nsIContentFilter.idl#109

For example, what if you wanted to proxy this interface, but never needed to actually change an out parameter.

Comment 24

[timeless]

Comment on attachment 304075 [details] [diff] [review]
this is probably right
[Checkin: Comment 27]

dougt: so use a sync proxy. this only applies to async proxies. with bug 422231 fixed you could write a throwaway js component that implements nsIRunnable and gets dispatched to the other thread for the purpose of making the call.

schrep: I'm fine w/ landing this now, however if you want to help, please collect a review for bug 422231. I'd much rather land them together (or that one first).

Comment 25

[Doug Turner (:dougt)]

timeless: sure.  it is just functionality we have that will go away after we land this.  I do like your idea of creating of using throwaway js components to dispatch events on other threads.  Please do write an example of that for devmo! :-)

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304075 [details] [diff] [review]
this is probably right
[Checkin: Comment 27]

a=beltzner, that other bug isn't a blocker, so let's just get this landed.

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 304075 [details] [diff] [review]
this is probably right
[Checkin: Comment 27]

{{
2008-04-21 11:57	timeless%mozdev.org 	mozilla/xpcom/proxy/src/nsProxyEventObject.cpp 	1.66
}}