bundle-ids need to be sanitized before using them in filesystem paths

[Simon Cooper]

When choosing / looking for a specialized Sandbox for a Plugin the bundle-id needs to be sanitized before using it to construct a filesystem name to look up.  The “/“ character should not be allowed and transformed into something else - e.g. “:”

[Simon Cooper]

Created attachment 194493 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 194493 [details]
Patch

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

r- for the leak.

I suggest using WTF types more here. Something like:

String bundleIdentifier = CFBundleGetIdentifier(pluginBundle.get());
if (bundleIdentifier.isEmpty())
    return String();

// Fold all / characters to : to prevent the plugin bundle-id from trying to escape the profile directory.
bundleIdentifier.replace('/', ':');

RetainPtr<CFStringRef> sandboxFileName = adoptCF(CFStringCreateWithFormat(0, 0, CFSTR("%@.sb"), bundleIdentifier.createCFString().get()));

Or you could go even further, and use String::format instead of CFStringCreateWithFormat too.

> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:300
> +    CFMutableStringRef bundleIdentifier = CFStringCreateMutableCopy(0, 0, bundleID);

This allocated string needs to be released. We normally use a RetainPtr wrapper to automate it, like this:

RetainPtr<CFMutableStringRef> bundleIdentifier = adoptCF(CFStringCreateMutableCopy(0, 0, bundleID));

I think that the distinction between bundleID and bundleIdentifier names is too subtle.

> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:311
>      RetainPtr<CFStringRef> sandboxFileName = CFStringCreateWithFormat(0, 0, CFSTR("%@.sb"), bundleIdentifier);

This is existing code, but it also leaks. The reason is that this RetainPtr constructor retains the value, so the releases are not balanced.

This line should have used adoptCF, like the one above.

[Alexey Proskuryakov]

<rdar://problem/13300254>

[Alexey Proskuryakov]

Oh, and the patch lacks a ChangeLog for some reason.

[Simon Cooper]

Created attachment 195045 [details]
Patch

[Simon Cooper]

Comment on attachment 195045 [details]
Patch

*sigh* this attachment is not what I intended to upload.

[Simon Cooper]

Created attachment 195047 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 195047 [details]
Patch

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

You didn't formally request commit queue, but since you are not a committer yet as far as I know, I'm assuming that you want the patch to be automatically landed.

> Source/WebKit2/ChangeLog:12
> +        filesystem name to look up. The â/â character should not be allowed
> +        and transformed into something else, in this case a â:â

Not sure if quotation marks actually got garbled, but they certainly look garbled now. Did you use a UTF-8 aware editor?

[WebKit Review Bot]

Comment on attachment 195047 [details]
Patch

Clearing flags on attachment: 195047

Committed r146902: <http://trac.webkit.org/changeset/146902>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.