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

RESOLVED FIXED

Status

()

Core
Plug-ins
--
critical
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Jeff Blitz, Assigned: jst)

Tracking

({crash, fixed1.8.0.4, fixed1.8.1})

Trunk
x86
Windows XP
crash, fixed1.8.0.4, fixed1.8.1
Points:
---
Bug Flags:
blocking1.7.14 +
blocking-aviary1.0.9 +
blocking1.8.1 +
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [need testcase])

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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.
Assignee: nobody → jst
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

11 years ago
Summary: scriptable plugin can cause crash and heap corruption → scriptable plugins can cause crash and heap corruption [with ambigous members]
(Assignee)

Comment 2

11 years ago
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.
Attachment #213052 - Flags: superreview?(brendan)
Attachment #213052 - Flags: review?(mrbkap)
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
Attachment #213052 - Flags: superreview?(brendan) → superreview+
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.
Attachment #213052 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 5

11 years ago
(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.
(Assignee)

Comment 6

11 years ago
Created attachment 213132 [details] [diff] [review]
Updated fix.
(Assignee)

Comment 7

11 years ago
Fix landed on the trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: blocking1.8.0.3?
Flags: blocking-firefox2?
Flags: blocking-aviary1.0.9?
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #213052 - Flags: approval1.8.0.3?
Attachment #213052 - Flags: approval-branch-1.8.1?
Attachment #213052 - Flags: approval-aviary1.0.9?
(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

Updated

11 years ago
Component: General → Plug-ins
Flags: review+
Flags: blocking-firefox2?
Product: Firefox → Core
Version: unspecified → Trunk

Updated

11 years ago
Flags: blocking1.8.1+
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3+
Flags: blocking1.7.14+
Flags: blocking-aviary1.0.9?
Flags: blocking-aviary1.0.9+
(Assignee)

Updated

11 years ago
Attachment #213052 - Flags: approval-branch-1.8.1? → approval-branch-1.8.1+
(Assignee)

Comment 9

11 years ago
Fix landed on the 1.8.1 branch.
Keywords: fixed1.8.1
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
Attachment #213052 - Flags: approval1.8.0.3? → approval1.8.0.3+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.3

Comment 11

11 years ago
Does anyone have a testcase for this bug or an easy way to verify the fix?  
Whiteboard: [need testcase]
Keywords: crash
(Assignee)

Updated

9 years ago
Attachment #213052 - Flags: approval-aviary1.0.9?
You need to log in before you can comment on or make changes to this bug.