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

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted 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 343776: Patch

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




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

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

r=me
This patch looks good to me, but I think it's worth getting someone else's eyes
on it too.

Let's also discuss in person if we want to commit it now or hold off a bit.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:240
> +static inline String sandboxParentDirectory()

Might be worth a comment saying we've already set the DIRHELPER_USER_DIR_SUFFIX
environment variable here. Or you could have a debug assert w/ getenv

> Source/WebKit/Shared/mac/ChildProcessMac.mm:301
> +    constexpr size_t alphaNumRange = sizeof(alphaNum) - 1;

strlen here seems more intuitive.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:302
> +    String string;

This should use StringBuilder and return the builder.toString() at the end.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:305
> +	   string.append(alphaNum[rand() % alphaNumRange]);

We should probably use WTF::randomNumber() here just for consictency.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:335
> +    if (errno == EEXIST) {

will this ever be EAGAIN?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:355
> +	   FileSystem::makeAllDirectories(movedDirectory);

Maybe this should be part of the loop since this can fail?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:364
> +		   FileSystem::deleteFile(file);

Should we verify it got deleted? Seems like a vulnerability if it doesn't