Closed
Bug 328457
Opened 19 years ago
Closed 19 years ago
scriptable plugins can cause crash and heap corruption [with ambigous members]
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jblitz, Assigned: jst)
Details
(Keywords: crash, fixed1.8.0.4, fixed1.8.1, Whiteboard: [need testcase])
Attachments
(2 files)
6.58 KB,
patch
|
brendan
:
superreview+
jst
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
6.64 KB,
patch
|
Details | Diff | Splinter Review |
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•19 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•19 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•19 years ago
|
||
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 3•19 years ago
|
||
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 4•19 years ago
|
||
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•19 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•19 years ago
|
||
Assignee | ||
Comment 7•19 years ago
|
||
Fix landed on the trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Flags: blocking1.8.0.3?
Flags: blocking-firefox2?
Flags: blocking-aviary1.0.9?
Resolution: --- → FIXED
Assignee | ||
Updated•19 years ago
|
Attachment #213052 -
Flags: approval1.8.0.3?
Attachment #213052 -
Flags: approval-branch-1.8.1?
Attachment #213052 -
Flags: approval-aviary1.0.9?
Comment 8•19 years ago
|
||
(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•19 years ago
|
Component: General → Plug-ins
Flags: review+
Flags: blocking-firefox2?
Product: Firefox → Core
Version: unspecified → Trunk
Updated•19 years ago
|
Flags: blocking1.8.1+
Updated•19 years ago
|
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•19 years ago
|
Attachment #213052 -
Flags: approval-branch-1.8.1? → approval-branch-1.8.1+
Comment 10•19 years ago
|
||
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•19 years ago
|
Keywords: fixed1.8.0.3
Comment 11•18 years ago
|
||
Does anyone have a testcase for this bug or an easy way to verify the fix?
Whiteboard: [need testcase]
Assignee | ||
Updated•16 years ago
|
Attachment #213052 -
Flags: approval-aviary1.0.9?
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•