Bug 46279 – Random plug-in cleanups

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Anders Carlsson]

Random plug-in cleanups

[Anders Carlsson]

Created attachment 68395 [details]
Patch

[WebKit Review Bot]

Attachment 68395 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/WebProcess/Plugins/Plugin.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 68395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68395&action=review

> WebKit2/WebProcess/Plugins/Plugin.cpp:48
> +    parameters.url = KURL(ParsedURLString, urlString);

Are you sure this is the constructor we want to use here? This is normally only suitable if we know that the URL is already parsed by KURL and the string came out of KURL. Is that going to be true. Further, is this consistent with the “don’t trust the other process” philosophy?

> WebKit2/WebProcess/Plugins/Plugin.cpp:57
> +    if (!decoder->decode(parameters.names))
> +        return false;
> +    if (!decoder->decode(parameters.values))
> +        return false;
> +    if (!decoder->decode(parameters.mimeType))
> +        return false;
> +    if (!decoder->decode(parameters.loadManually))
> +        return false;

Do these functions normally check for validity? For example, are the names and values arrays the same length? Are they all strings in those arrays? Are there any invalid values for anything?

[Anders Carlsson]

(In reply to comment #3)
> (From update of attachment 68395 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68395&action=review
> 
> > WebKit2/WebProcess/Plugins/Plugin.cpp:48
> > +    parameters.url = KURL(ParsedURLString, urlString);
> 
> Are you sure this is the constructor we want to use here? This is normally only suitable if we know that the URL is already parsed by KURL and the string came out of KURL. Is that going to be true. Further, is this consistent with the “don’t trust the other process” philosophy?
> 

I hadn't thought about the possibility that the URLs might be invalid because they came from a compromised web process. 

I think the best way to solve this is to add an argument coder for KURL which checks that the URLs are valid, and I'll do this in a followup patch.

> > WebKit2/WebProcess/Plugins/Plugin.cpp:57
> > +    if (!decoder->decode(parameters.names))
> > +        return false;
> > +    if (!decoder->decode(parameters.values))
> > +        return false;
> > +    if (!decoder->decode(parameters.mimeType))
> > +        return false;
> > +    if (!decoder->decode(parameters.loadManually))
> > +        return false;
> 
> Do these functions normally check for validity? For example, are the names and values arrays the same length? Are they all strings in those arrays? Are there any invalid values for anything?

They check that the strings are valid, but not that the number of names matches the number of values. I'll fix that.

[Anders Carlsson]

Committed r68058: <http://trac.webkit.org/changeset/68058>