[webkit-reviews] review denied: [Bug 184991] We should cache the compiled sandbox profile in a data vault : [Attachment 343655] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Ben Richards
<benton_richards at apple.com>'s request for review:
Bug 184991: We should cache the compiled sandbox profile in a data vault
https://bugs.webkit.org/show_bug.cgi?id=184991

Attachment 343655: Patch

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




--- Comment #79 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 343655
  --> https://bugs.webkit.org/attachment.cgi?id=343655
Patch

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

This looks really close to done, and looks really solid, just a few comments
and maybe a couple of bugs.

> Source/WTF/ChangeLog:3
> +	   Added trace points for sandbox initialization

This line should be the title of your bugzilla and you should put this
description below the bug "Reviewed by ..." line. (See other changelog entries)

> Source/WebKit/ChangeLog:3
> +	   Added sandbox caching so that we don't have to spend 30+ ms
compiling the sandbox on every process launch

ditto

> Source/WebKit/Scripts/process-plugin-entitlements.sh:2
> +#!/bin/sh
> +set -e

What is this file doing? I think part is better reviewed by somebody who
understands our build system.

> Source/WebKit/Scripts/process-plugin-entitlements.sh:11
> +	   if (( ${TARGET_MAC_OS_X_VERSION_MAJOR} >= 101400 )); then

What is this doing? What do we do for older OSs?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:59
> +#define VERBOSE_LOGGING false

why not `static constexpr bool = false`? This can also just be a hardcoded
constant in the `verboseLog` function

> Source/WebKit/Shared/mac/ChildProcessMac.mm:75
> +    char osVersion[10];

os version string is guaranteed to be strlen()<=9?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:84
> +// [SandboxBuiltin] optional. Present if
CachedSandboxHeader::sanboxBuiltinSize is not UINT_MAX. If present,
sandboxBuiltinSize bytes.

Probably worth specifying here if this contains the null terminating byte at
the end or not.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:162
> +template<typename... Types>
> +void verboseLog(const Types&... values)
> +{
> +    dataLogLnIf(VERBOSE_LOGGING, values...);
> +}

Do you think we still need this? Was it helpful in you writing this patch?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:219
> +	   auto& name = headerPieces.last();

This is wrong. You're grabbing a reference to a Cstring that may move in the
line 220 append call. Vector::append may reallocate storage. Maybe just do the
math to headerSize here before appending?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:380
> +	   WTFLogAlways("%s: \"%s\" is using a reserved path. Attempting to
move it.", getprogname(), directoryPath.data(), directoryPath.data());
> +	   String movedDirectory = info.directoryPath + ".Old";
> +	   while (FileSystem::fileIsDirectory(movedDirectory,
FileSystem::ShouldFollowSymbolicLinks::No)) {
> +	       // Keep trying until we create the directory
> +	       movedDirectory = info.directoryPath + ".Old+" + randomString();
> +	   }
> +	   FileSystem::makeAllDirectories(movedDirectory);
> +	   
> +	   // Now let's move everything to the new directory
> +	   Vector<String> directoryContents =
FileSystem::listDirectory(info.directoryPath, "*");
> +	   for (const auto& file : directoryContents) {
> +	       String fileName = FileSystem::pathGetFileName(file);
> +	       String newPath =
FileSystem::pathByAppendingComponent(movedDirectory, fileName);
> +	       if (!FileSystem::moveFile(file, newPath)) {
> +		   WTFLogAlways("%s: \"%s\" could not be moved and must be
deleted", getprogname(), FileSystem::fileSystemRepresentation(file).data());
> +		   FileSystem::deleteFile(file);
> +	       }
> +	   }

Why isn't this just doing rename on the directory? Seems like you have a TOCTOU
bug on line 369.

I feel like this entire function should just be something like this:

while (true) {
    if (directory is already is a data vault is true)
	break;
    if (try to make a data vault directory is true)
	break;
     rename directory with random suffix
}

> Source/WebKit/Shared/mac/ChildProcessMac.mm:409
> +    // Make sure the data vault/directory we want to cache in is good to go

style nit: this comment doesn't add anything that isn't described by your
function name below

> Source/WebKit/Shared/mac/ChildProcessMac.mm:456
> +	   if (i)
> +	       tempFileString.append(String::number(i));

I wonder if we should use your randomString function here instead, so someone
can't make us iterate for a long time.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:475
> +	       // According to POSIX complience requirements, this should not
only rename the temporary

complience => compliance

Also worth linking to where you found this info

> Source/WebKit/Shared/mac/ChildProcessMac.mm:486
> +    // Clean up

style nit: this comment doesn't add anything

> Source/WebKit/Shared/mac/ChildProcessMac.mm:588
> +    const static NSBundle* webKit2Bundle = [NSBundle
bundleForClass:NSClassFromString(@"WKWebView")];

I think our style for ObjC code is to have the pointer be "Type *ident" instead
of our normal style of "Type* ident"

> Source/WebKit/Shared/mac/ChildProcessMac.mm:676
> +    char temporaryDirectory[PATH_MAX + 1];

why +1 here?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:687
> +#if WK_API_ENABLED
> +    NSBundle* webKit2Bundle = [NSBundle
bundleForClass:NSClassFromString(@"WKWebView")];
> +#else
> +    NSBundle* webKit2Bundle = [NSBundle
bundleForClass:NSClassFromString(@"WKView")];
> +#endif

We do this type of thing twice, maybe we should make a function for it?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:738
> +    NSEvent* event = [NSEvent
otherEventWithType:NSEventTypeApplicationDefined location:NSMakePoint(0, 0)
modifierFlags:0 timestamp:0.0 windowNumber:0 context:nil subtype:0 data1:0
data2:0];

I think the previous style is correct here