[webkit-reviews] review denied: [Bug 113030] bundle-ids need to be sanitized before using them in filesystem paths : [Attachment 194493] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Simon Cooper
<scooper at apple.com>'s request for review:
Bug 113030: bundle-ids need to be sanitized before using them in filesystem
paths
https://bugs.webkit.org/show_bug.cgi?id=113030

Attachment 194493: Patch
https://bugs.webkit.org/attachment.cgi?id=194493&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.