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

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied  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 Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234053&action=review


Whoops, sorry i should have removed the r? as i was still making changes :)

Thanks for the feedback though :D

>> Source/WebKit2/NetworkProcess/cocoa/NetworkProcessCocoa.mm:67
>> +	    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.

Hmmm, i'm looking at the diff and i think what happened is that i introduce the
crash while debugging.

>> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:74
>> +	    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.

Yeah, i didn't want to do the extra alloc, but guess this should be hot and i'm
doing an unnecessary premature optimisation.

>> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:77
>> +	    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.

Ah, ok

>> 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.

Ah, ok, i'm just super paranoid about such things

>> 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.

I'll do CFString.... to be consistent with webcore

>> 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.

Even for value returns? a forward decl would not allow the surely? What am i
missing?

>> 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.

oh right we have empty vs null. Sigh.

Also re periods: i can't grammar

>> Source/WebKit2/UIProcess/WebContext.cpp:414
>> +	parameters.cookieStorageDirectory = (cookieStorageDirectory());
> 
> The extra parentheses here are a bit non-idiomatic. I’d leave them out.

Yeah, whoops left over cruft

>> Source/WebKit2/UIProcess/mac/WebContextMac.mm:276
>> +	    path = [@"~/" stringByStandardizingPath];
> 
> Maybe use NSHomeDirectory() instead?

Hmmmm, i knew there was an api like that (couldn't remember the name), but
everywhere else was the awful ~ thing. Will switch.