[webkit-reviews] review granted: [Bug 134360] Restrict network process sandbox : [Attachment 234053] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 134360: Restrict network process sandbox
https://bugs.webkit.org/show_bug.cgi?id=134360

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234053&action=review


> Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:67
> +   
SandboxExtension::consumePermanently(parameters.cookieStorageDirectoryExtension
Handle);
> +    m_diskCacheDirectory = parameters.diskCacheDirectory;
> +
> +    if (!m_diskCacheDirectory.isNull()) {
> +	   if
(!SandboxExtension::consumePermanently(parameters.diskCacheDirectoryExtensionHa
ndle))

Why are we checking the return value from consuming
diskCacheDirectoryExtensionHandle yet not doing the same for
cookieStorageDirectoryExtensionHandle just above?

Code paragraphing here is peculiar. The code to set m_diskCacheDirectory is
grouped with the code to work with cookieStorageDirectoryExtensionHandle
instead of with the code for diskCacheDirectoryExtensionHandle. I’d move the
blank line or remove it.

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:74
> +    char path[MAXPATHLEN + 1];
> +    if (sandbox_container_path_for_pid(getpid(), path, MAXPATHLEN))
> +	   return String();

This is passing the wrong buffer size. It should not pass the size minus one,
but rather the entire size. I suggest sizeof(path).

It’s a little strange that processHasContainer above is using std::array but we
are opting not to use it here.

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:77
> +    if (!path[0])
> +	   return String();

Is there some reason it’s important to return a null string instead of an empty
string in this case? If it’s not, I suggest just omitting this special case as
the normal case below will work for the empty string.

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:79
> +    path[MAXPATHLEN] = 0;

Not needed. There’s no reason that sandbox_container_path_for_pid would return
a non-terminated string and pretend to succeed.

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:80
> +    return String(path);

Since the path is a UTF-8 string, we should not be using the constructor that
treats characters as Latin-1.

WebCore’s FileSystem.h code even goes so far as to use
CFStringGetFileSystemRepresentation; the corresponding thing to do here would
be to use CFStringCreateWithFileSystemRepresentation.

But it might be OK to call String::fromUTF8.

> Source/WebKit2/Shared/mac/SandboxUtilities.h:30
> +#include <wtf/text/WTFString.h>

No need to include this header just to return an object of type WTF::String.
Instead, we could include wtf/Forward.h.

> Source/WebKit2/Shared/mac/SandboxUtilities.h:37
> +// Returns an empty string if the process is not in a container

The code seems to return a null string in this case, not an empty string.

We normally use a period at the end of these kinds of comments.

> Source/WebKit2/UIProcess/WebContext.cpp:414
> +    parameters.cookieStorageDirectory = (cookieStorageDirectory());

The extra parentheses here are a bit non-idiomatic. I’d leave them out.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:276
> +	   path = [@"~/" stringByStandardizingPath];

Maybe use NSHomeDirectory() instead?