[webkit-reviews] review denied: [Bug 90005] [Qt][Linux][WK2] Putting QtWebProcess into a chrooted sandbox : [Attachment 177000] Proposed patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Renata Hodovan
<reni at webkit.org>'s request for review:
Bug 90005: [Qt][Linux][WK2] Putting QtWebProcess into a chrooted sandbox
https://bugs.webkit.org/show_bug.cgi?id=90005

Attachment 177000: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=177000&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=177000&action=review


Looks much better, only a couple of minor nits left to fix! ;)

> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:52
> +enum { PathSize = 512 };

It's a little weird to use an enum here, can't you just use a const unsigned?
Also, globals should still start with a lowercase letter. I think a better name
would be maximumPathLength.

> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:114
> +    if (lstat(sandboxDirectory, &sandboxDirectoryInfo) != -1) {
> +	   if (!S_ISDIR(sandboxDirectoryInfo.st_mode)) {
> +	       fprintf(stderr, "%s is not a directory!\n", sandboxDirectory);
> +	       return;
> +	   }
> +    } else {
> +	   fprintf(stderr, "Sandbox directory (%s) is not available: %s.\n",
sandboxDirectory, strerror(errno));
> +	   return;
> +    }

I think this would look better as checking if lstat returns -1 and returning
early, and then checking the ISDIR flag.

> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:150
> +    if (setenv(SANDBOX_HELPER_PID, sandboxHelperPID , 1) == -1) {

Extra space before the comma.

> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:170
> +    return true;

I think there should be an extra newline before this return statement.

> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:221
> +	   if (cap_set_flag(capabilities, CAP_EFFECTIVE, length,
capabilityList, CAP_SET)
> +	       || cap_set_flag(capabilities, CAP_INHERITABLE, length,
capabilityList, CAP_SET)
> +	       || cap_set_flag(capabilities, CAP_PERMITTED, length,
capabilityList, CAP_SET) == -1) {

I think you need to check each cap_set_flag value for -1 here.

> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:313
> +static bool createDirectoryChain(const char* path)

I think this could use a comment detailing what it does.

> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:655
> +    appendDirectoryComponent(localDirectory, home, "/.local/share/Nokia/");
> +    appendDirectoryComponent(cacheDirectory, home, "/.cache/Nokia/");

Nokia?