[Webkit-unassigned] [Bug 137425] Plugin process crashes in NPN_InvokeDefault

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=137425

--- Comment #29 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #28)
> Comment on attachment 293400 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293400&action=review
> 
> > Source/WebCore/bridge/NP_jsobject.cpp:171
> > +    if (!npp || !o || !o->_class || !o->_class->invokeDefault)
> > +        return false;
> 
> This is a change in behavior. Before we would return true with a void value
> in result when the invokeDefault pointer was null. I don’t see any test
> coverage for this change in behavior.

Yes, this is not covered by tests, I'm afraid, I just copied the early return condition from firefox, but only null npp and null npobject is covered by the tests (I think, I'll check anyway).

> > Source/WebCore/bridge/NP_jsobject.cpp:173
> >      if (o->_class == NPScriptObjectClass) {
> 
> This is broken now, because invokeDefault is null in NPScriptObjectClass, so
> this code will never run. No test coverage?

Ah, I didn't notice that. 

> > Source/WebCore/bridge/NP_jsobject.cpp:213
> > +    if (!npp || !o || !o->_class || !o->_class->invoke)
> > +        return false;
> 
> This is a change in behavior. Before we would return true with a void value
> for result when the invoke pointer was null. I don’t see any test coverage
> for this change in behavior.
> 
> > Source/WebCore/bridge/NP_jsobject.cpp:215
> >      if (o->_class == NPScriptObjectClass) {
> 
> Also broken like the other case above.
> 
> > Source/WebCore/bridge/NP_jsobject.cpp:293
> > +    if (!npp || !o || !o->_class || !o->_class->getProperty)
> > +        return false;
> 
> This is a change in behavior. Before we would set result to a void value
> when the getProperty pointer was null. Maybe we should test this?
> 
> > Source/WebCore/bridge/NP_jsobject.cpp:295
> >      if (o->_class == NPScriptObjectClass) {
> 
> Also broken like the other cases above.
> 
> > Source/WebCore/bridge/NP_jsobject.cpp:322
> > -    if (o->_class->hasProperty && o->_class->getProperty) {
> > -        if (o->_class->hasProperty(o, propertyName))
> > -            return o->_class->getProperty(o, propertyName, variant);
> > -        return false;
> > -    }
> > +    if (o->_class->hasProperty(o, propertyName))
> > +        return o->_class->getProperty(o, propertyName, variant);
> 
> Why is it OK to drop the check of o->_class->hasProperty for null? Do we
> have a test case covering that?

I went too fast and thought this was only checking getProperty and removed the if because getProperty was already null checked before.

> > Source/WebCore/bridge/NP_jsobject.cpp:333
> >      if (o->_class == NPScriptObjectClass) {
> 
> Also broken like the other cases above.
> 
> > Source/WebCore/bridge/NP_jsobject.cpp:408
> >      if (o->_class == NPScriptObjectClass) {
> 
> Also broken like the other cases above.
> 
> > Source/WebCore/bridge/NP_jsobject.cpp:441
> >      if (o->_class == NPScriptObjectClass) {
> 
> Also broken like the other cases above.
> 
> > Source/WebCore/bridge/NP_jsobject.cpp:522
> >      if (o->_class == NPScriptObjectClass) {
> 
> Also broken like the other cases above.

I'll update the patch.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161031/87905ff3/attachment-0001.html>