Closed Bug 474157 Opened 11 years ago Closed 11 years ago

Not always checking for plugin exception set with NPN_SetException after invoking an NPClass callback on an NPObject

Categories

(Core :: Plug-ins, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: apatrick, Assigned: jaas)

References

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 GTB5 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5 GTB5 (.NET CLR 3.5.30729)

I am filing this bug as requested in https://bugzilla.mozilla.org/show_bug.cgi?id=470291

In addition to the build identified above, I have checked the latest code in the repository and this bug still exists at the time of writing.

In most cases, any code that invokes an NPClass callback (e.g. hasProperty) on an NPObject should check to see whether the plugin set an exception using NPN_SetException. If this is the case, the exception should immediately be thrown into JavaScript rather than taking any further action as if the exception had not been set.

Here are some examples all from http://hg.mozilla.org/mozilla-central/file/6e59106a308e/modules/plugin/base/src/nsJSNPRuntime.cpp

NPObjWrapper_AddProperty calls hasProperty and does not check for an exception before calling hasMethod. Before calling hasMethod, it should check for an exception and throw if one was set. Similarly, it does not check whether hasMethod throws an exception. Although it will throw an exception if both hasProperty or hasMethod return false, this is not the right thing to do. An exception set by the plugin using NPN_SetException should contain a more specific error than the generic error thrown by NPObjWrapper_AddProperty. Furthermore, even if the plugin returns true from these callbacks, it may still have set an exception. This would be unusual and possibly not compliant with npruntime but if a plugin sets an exception, the right thing to do seems to be to throw it into JavaScript, regardless of its returned result.

NPObjWrapper_DelProperty does not check for a set exception after calling hasProperty or removeProperty.

NPObjWrapper_SetProperty does not check for a set exception after calling hasProperty. Also if setProperty returns false, it does not check for a more specific exception that may have been set by the plugin before throwing its generic exception.

NPObjWrapper_GetProperty does not perform sufficient checks after calling hasProperty, hasMethod or getProperty.

CallNPMethodInternal is complicated but it appears in some cases it will throw a less specific exception when a plugin sets an exception in response to construct, invoke or invokeDefault being called.

NPObjWrapper_newEnumerate will sometimes fail to throw an exception set by the plugin by its enumerate callback and will sometimes throw a less specific exception.

NPObjWrapper_NewResolve does not check for an exception set by the hasProperty and hasMethod callbacks.

CreateNPObjectMember does not check for an exception set by getProperty.

NPObjectMember_Call does not check for an exception set by the invoke callback and will sometimes report a less specific exception.

These are all the examples I could find. There may be others. There are sure to be cases where it doesn't make sense to throw an exception if the plugin sets one. For example, if the plugin sets an exception in a deallocate callback, throwing an exception into JavaScript seems wrong (if it's even possible from where it gets called).

Reproducible: Always
Status: UNCONFIRMED → NEW
Component: General → Plug-ins
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → plugins
Assignee: nobody → joshmoz
Attached patch part 1, v1.0 (obsolete) — Splinter Review
Part 1 fixes NPObjWrapper_AddProperty, NPObjWrapper_DelProperty, NPObjWrapper_SetProperty, and NPObjWrapper_GetProperty.

This includes a fix for an incorrect exception string (set vs. get):

-    if (!npobj->_class->getProperty(npobj, (NPIdentifier)id, &npv)) {
-      ThrowJSException(cx, "Error setting property on scriptable plugin "

Also includes a fix for a case in which NPObjWrapper_GetProperty is called and hasproperty fails but we return true and don't set *vp.
Attachment #375108 - Flags: superreview?(jst)
Attachment #375108 - Flags: superreview?(jst)
Attached patch part 1, v1.1 (obsolete) — Splinter Review
Attachment #375108 - Attachment is obsolete: true
Attachment #375109 - Flags: superreview?(jst)
Attachment #375109 - Attachment description: fix v1.1 → part 1, v1.1
Comment on attachment 375109 [details] [diff] [review]
part 1, v1.1

Canceling review, this has a bug.
Attachment #375109 - Flags: superreview?(jst)
Attached patch part 1, v1.2Splinter Review
Fix getproperty and thus unit tests.
Attachment #375109 - Attachment is obsolete: true
Attachment #375251 - Flags: superreview?(jst)
Attachment #375251 - Attachment description: fix v1.2 → part 1, v1.2
Comment on attachment 375251 [details] [diff] [review]
part 1, v1.2

Looks good.
Attachment #375251 - Flags: superreview?(jst)
Attachment #375251 - Flags: superreview+
Attachment #375251 - Flags: review+
part 1 pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/c7b717f0a404
Attached patch part 2, v1.0 (obsolete) — Splinter Review
Attachment #376785 - Flags: superreview?(jst)
Silverlight is not working; see Bug 492924.
Attachment #376785 - Flags: superreview?(jst) → review?(mrbkap)
Attachment #376785 - Flags: review?(mrbkap) → superreview?(jst)
Comment on attachment 376785 [details] [diff] [review]
part 2, v1.0

- In CreateNPObjectMember():

   *vp = OBJECT_TO_JSVAL(memobj);
   ::JS_AddRoot(cx, vp);
 
   ::JS_SetPrivate(cx, memobj, (void *)memberPrivate);
 
   jsval fieldValue;
   NPVariant npv;
   VOID_TO_NPVARIANT(npv);
-  if (!npobj->_class->getProperty(npobj, (NPIdentifier)id, &npv)) {
+
+  NPBool hasProperty = npobj->_class->getProperty(npobj, (NPIdentifier)id,
+                                                  &npv);
+  if (ReportExceptionIfPending(cx))
+    return JS_FALSE;

You must call ::JS_RemoveRoot(cx, vp) here before returning or you'll leak (and likely crash later on).

+
+  if (!hasProperty) {
     ::JS_RemoveRoot(cx, vp);
-    return false;
+    return JS_FALSE;

r+sr=jst with that fixed.
Attachment #376785 - Flags: superreview?(jst)
Attachment #376785 - Flags: superreview+
Attachment #376785 - Flags: review+
Attached patch part 2, v1.1Splinter Review
Attachment #376785 - Attachment is obsolete: true
part 2 pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/dc227440ba77
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.