[webkit-reviews] review granted: [Bug 134360] Restrict network process sandbox : [Attachment 234053] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jun 30 08:09:47 PDT 2014
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?
More information about the webkit-reviews
mailing list