[webkit-reviews] review granted: [Bug 187310] Add the possibility to run unsandboxed plug-ins : [Attachment 344257] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 187310: Add the possibility to run unsandboxed plug-ins
https://bugs.webkit.org/show_bug.cgi?id=187310

Attachment 344257: Patch

https://bugs.webkit.org/attachment.cgi?id=344257&action=review




--- Comment #6 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 344257
  --> https://bugs.webkit.org/attachment.cgi?id=344257
Patch

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

> Source/WebCore/page/RuntimeEnabledFeatures.h:269
> +    void setSandboxPlugInEnabled(bool isEnabled) { m_isSandboxPlugInEnabled
= isEnabled; }
> +    bool sandboxPlugInEnabled() const { return m_isSandboxPlugInEnabled; }

This name seems confusing. Maybe something like
"setExperimentalPlugInSandboxProfilesEnabled"?

> Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:638
> +    if (!confstr(_CS_DARWIN_USER_CACHE_DIR, cacheDirectory,
sizeof(cacheDirectory))) {

This look suspicious to me (but no more suspicious than existing code). The
result of confstr depends on DIRHELPER_USER_DIR_SUFFIX, which is only set in
ChildProcess::initializeSandbox that's called later. How is that OK? Which
directory paths are we getting here?

Perhaps it's OK because we only use this with randomized paths like
"WebKitPlugin-aQVpAV". But I don't see where that's being enforced.

> Source/WebKit/Shared/WebPreferences.yaml:1264
> +  humanReadableName: "Sandbox Plug-Ins"
> +  humanReadableDescription: "Enable Plug-In sandboxing"

These also need better names.

> Source/WebKit/UIProcess/Plugins/mac/PluginInfoStoreMac.mm:87
> +bool PluginInfoStore::shouldRunPluginUnsandboxed(const String&
pluginBundleIdentifier)

Maybe allowPluginToRunUnsandboxed?

> Source/WebKit/UIProcess/Plugins/mac/PluginInfoStoreMac.mm:92
> +    return pluginBundleIdentifier == "com.cisco.webex.plugin.gpc64"

Let's make it _s at least, as this doesn't seem very performance sensitive to
warrant a precomputed table of any kind.