328457 - scriptable plugins can cause crash and heap corruption [with ambigous members]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jeff Blitz]

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)]

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)]

Attachment: 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]]

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) (inactive)]

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)]

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

Attachment: Updated fix.

Comment 7

[Johnny Stenback (:jst)]

Fix landed on the trunk.

Comment 8

[Brendan Eich [:brendan]]

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

Fix landed on the 1.8.1 branch.

Comment 10

[Daniel Veditz [:dveditz]]

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]]

Does anyone have a testcase for this bug or an easy way to verify the fix?