Last Comment Bug 328457 - scriptable plugins can cause crash and heap corruption [with ambigous members]
: scriptable plugins can cause crash and heap corruption [with ambigous members]
Status: RESOLVED FIXED
[need testcase]
: crash, fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-24 05:48 PST by Jeff Blitz
Modified: 2008-05-05 16:15 PDT (History)
7 users (show)
dveditz: blocking1.7.14+
dveditz: blocking‑aviary1.0.9+
mconnor: blocking1.8.1+
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't hold a strong reference to the NPObject in member objects. (6.58 KB, patch)
2006-02-24 08:39 PST, Johnny Stenback (:jst, jst@mozilla.com)
brendan: superreview+
jst: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Review
Updated fix. (6.64 KB, patch)
2006-02-24 16:30 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review

Description Jeff Blitz 2006-02-24 05:48:35 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

references to scriptable object may not be released before page is unloaded. Heap corruption and crash in the runtime dll.

Reproducible: Always

Steps to Reproduce:
1. read a property from a scriptable plugin
2. close the window
3.

Actual Results:  
crash

Expected Results:  
window closes without crash

This bug is known to the developers. This report is just for tracking.
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2006-02-24 08:21:50 PST
Thanks for filing this. The specific issue here is that when JS accesses a property on a scriptable plugin that happens to be an ambigous member (one for which both a property and a method exists), we create a special member JS object that internally holds a strong reference to the scripted NPObject. But during plugin teardown, we go through and invalidate all of the plugins NPObjects, which will force deletion of them. After this we can still have some of those special member objects still alive in memory, and once the member objects are finalized, they try to release the underlying NPObject which by then has already been freed.

This was introduced by bug 293972. Patch coming up.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2006-02-24 08:39:31 PST
Created attachment 213052 [details] [diff] [review]
Don't hold a strong reference to the NPObject in member objects.

This should fix this crash since with this change we no longer hold a strong reference from the member object data to the NPObject, so there's no need to release it during finalization. Instead, we hold a reference to the NPObject wrapper (the JSObject), and get to the NPObject through it, and to ensure the NPObject wrapper doesn't go away too early we mark the wrapper in the member object's mark hook.
Comment 3 Brendan Eich [:brendan] 2006-02-24 10:01:45 PST
Comment on attachment 213052 [details] [diff] [review]
Don't hold a strong reference to the NPObject in member objects.

>+  // npobjWrapper is the JSObject through which we make sure we don't
>+  // outlive the underlying NPObject, so make sure it points to the
>+  // real JSObject wrapper for the NPObject.
>+  while (JS_GET_CLASS(cx, obj) != &sNPObjectJSWrapperClass) {
>+    obj = ::JS_GetPrototype(cx, obj);
>+  }

Is there any way, at this point, that some evil thing could set __proto__ on some nearer object along the chain, such that this loop runs off the end without matching class and crashes on a null obj?

> NPObjectMember_Call(JSContext *cx, JSObject *obj,
>                     uintN argc, jsval *argv, jsval *rval)
> {
>   JSObject *memobj = JSVAL_TO_OBJECT(argv[-2]);
>   NS_ENSURE_TRUE(memobj, JS_FALSE);
> 
>   NPObjectMemberPrivate *memberPrivate =
>     (NPObjectMemberPrivate *)::JS_GetInstancePrivate(cx, memobj,
>                                                      &sNPObjectMemberClass,
>                                                      nsnull);
>-  if (!memberPrivate || !memberPrivate->npobj)
>+  if (!memberPrivate || !memberPrivate->npobjWrapper)
>     return JS_FALSE;

False means exception, but passing nsnull as fourth arg to JS_GetInstancePrivate makes that API an infallible getter.  You want to pass argv instead.

Looks good otherwise -- GC >> refcounting ;-).

/be
Comment 4 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-02-24 10:28:38 PST
Comment on attachment 213052 [details] [diff] [review]
Don't hold a strong reference to the NPObject in member objects.

r=mrbkap with brendan's comments addressed.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2006-02-24 16:28:44 PST
(In reply to comment #3)
> (From update of attachment 213052 [details] [diff] [review] [edit])
> >+  // npobjWrapper is the JSObject through which we make sure we don't
> >+  // outlive the underlying NPObject, so make sure it points to the
> >+  // real JSObject wrapper for the NPObject.
> >+  while (JS_GET_CLASS(cx, obj) != &sNPObjectJSWrapperClass) {
> >+    obj = ::JS_GetPrototype(cx, obj);
> >+  }
> 
> Is there any way, at this point, that some evil thing could set __proto__ on
> some nearer object along the chain, such that this loop runs off the end
> without matching class and crashes on a null obj?

As coded, there's no way that can happen. If we can't find the right object on the proto chain, we'd never get into this function to begin with (GetNPObject() does the right checks, and if GetNPObject() in the caller of this function (NPObjWrapper_GetProperty) returns null, we throw an error and return early).

> False means exception, but passing nsnull as fourth arg to
> JS_GetInstancePrivate makes that API an infallible getter.  You want to pass
> argv instead.

Fixed.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2006-02-24 16:30:42 PST
Created attachment 213132 [details] [diff] [review]
Updated fix.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2006-02-24 16:33:01 PST
Fix landed on the trunk.
Comment 8 Brendan Eich [:brendan] 2006-02-24 16:51:57 PST
(In reply to comment #5)
> As coded, there's no way that can happen. If we can't find the right object on
> the proto chain, we'd never get into this function to begin with (GetNPObject()
> does the right checks, and if GetNPObject() in the caller of this function
> (NPObjWrapper_GetProperty) returns null, we throw an error and return early).

Great, makes perfect sense -- thanks,

/be
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2006-03-24 17:00:30 PST
Fix landed on the 1.8.1 branch.
Comment 10 Daniel Veditz [:dveditz] 2006-04-03 12:11:02 PDT
Comment on attachment 213052 [details] [diff] [review]
Don't hold a strong reference to the NPObject in member objects.

approved for 1.8.0 branch, a=dveditz for drivers
Comment 11 Jay Patel [:jay] 2006-05-22 10:43:52 PDT
Does anyone have a testcase for this bug or an easy way to verify the fix?  

Note You need to log in before you can comment on or make changes to this bug.