[webkit-reviews] review denied: [Bug 90005] [WK2] Putting QtWebProcess into a chrooted sandbox : [Attachment 174722] Proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Nov 27 19:42:31 PST 2012
Anders Carlsson <andersca at apple.com> has denied Renata Hodovan
<reni at webkit.org>'s request for review:
Bug 90005: [WK2] Putting QtWebProcess into a chrooted sandbox
https://bugs.webkit.org/show_bug.cgi?id=90005
Attachment 174722: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=174722&action=review
------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=174722&action=review
> ChangeLog:6
> + Reviewed by Zoltan Herczeg.
And me!
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:52
> +static char sandboxDirectory[PathSize];
Doesn't Linux have a MAX_PATH #define?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:54
> +static uid_t sandboxUserUid;
> +static uid_t sandboxUserGid;
Please make UID and GID all caps.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:65
> +#define strlcpy(destination, source, maxLength) \
> + do { \
> + (destination)[0] = '\0'; \
> + strncat((destination), (source), (maxLength) - 1); \
> + } while (0)
> +
> +#define strlcat(destination, source, maxLength) \
> + do { \
> + strncat((destination), (source), (maxLength) - 1 -
strnlen((destination), (maxLength) - 1)); \
> + } while (0)
Why are these macros and not inline functions?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:70
> +static void launchChangeRootHelper(int socketPair[])
I think it'd be more clear for this function to take two parameters instead of
a pointer.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:73
> + // a file by mistake. However, CAP_SYS_RESSOURCE capability should be
dropped
Spelling error: CAP_SYS_RESOURCE.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:76
> + if (setrlimit(RLIMIT_NOFILE, &restrictedResource)) {
Please explicitly check for -1 here instead of just any nonzero value.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:77
> + fprintf(stderr, "Helper couldn't set the resourcelimit: %s.\n",
strerror(errno));
resourcelimit -> resource limit.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:81
> + if (close(socketPair[1])) {
Please explicitly check for -1 here instead of just any nonzero value.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:82
> + fprintf(stderr, "Couldn't close socket %d: %s\n", socketPair[1],
strerror(errno));
Missing period before the newline in the format string. Maybe "Failed to close
socket" is a better message?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:86
> + // We expect a 'C' (ChrootMe) message from the WebProcess.
I think this comment is better suited for the if statement below this one.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:89
> + fprintf(stderr, "Couldn't read the proper chrootme msg: %s\n",
strerror(errno));
I suggest making this error message more generic, something like "Failed to
read message from the web process."
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:94
> + fprintf(stderr, "Wrong message recieved.\n");
Maybe print out the message as a hex character here?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:99
> + if (lstat(sandboxDirectory, &sandboxDirectoryInfo) &&
S_ISDIR(sandboxDirectoryInfo.st_mode)) {
I think you want to check whether lstat returns -1 here. Also, if lstat fails,
then i don't think you can count on sandboxDirectoryInfo.st_mode being
initialized.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:104
> + if (chroot(sandboxDirectory)) {
You should check for a -1 return value here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:112
> + if (chdir("/")) {
Please explicitly check for -1 here instead of just any nonzero value.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:126
> +static bool setEnvironmentVariablesForChangeRootHelper(pid_t pid, int
socketPair[])
Same comment about the socketPair array.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:130
> + char sandboxHelperPid[descriptorSize];
The correct WebKit style guidelines name for this would be sandboxHelperPID.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:134
> + fprintf(stderr, "Converting the pid to string is failed: %s\n",
strerror(errno));
snprintf doesn't sett errno. Maybe just print out "Failed to convert the
sandbox helper PID to a string.\n".
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:138
> + if (setenv(SANDBOX_HELPER_PID, sandboxHelperPid, 1)) {
Please check for -1 here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:139
> + fprintf(stderr, "Couldn't set the SBX_HELPER_PID env variable:
%s\n", strerror(errno));
env -> environment.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:145
> + fprintf(stderr, "Converting the file descriptor to string is failed:
%s.\n", strerror(errno));
Same comment about snprintf not setting errno.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:149
> + if (setenv(SANDBOX_DESCRIPTOR, socketDescriptor, 1)) {
Please check for -1 here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:150
> + fprintf(stderr, "Saving the helpers filedescriptor into an env
variable failed: %s\n", strerror(errno));
"Failed to store the helper's file descriptor into an environment variable:
%s.\n"
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:154
> + if (close(socketPair[0])) {
Please check for -1 here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:155
> + fprintf(stderr, "Closing of %d failed: %s\n", socketPair[0],
strerror(errno));
Missing period.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:164
> + pid_t pid;
You can just declare and initialize pid on the same row.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:166
> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, socketPair)) {
Please check for -1 here. Also, do you really need to create a socket pair?
Wouldn't pipe be OK here?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:171
> + pid = syscall(SYS_clone, CLONE_FS | SIGCHLD, 0, 0, 0);
Why are you calling syscall here instead of clone directly?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:173
> + switch (pid) {
I think just using two if statements instead of this switch statement would be
clearer.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:182
> + // We shouldn't reach this part, because launchChrootHelper() should
exit in every cases.
I don't think this comment is correct. launchChrootHelper only exists in the
success case.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:191
> + // We should never reach here.
Missing newline.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:199
> +static bool setCapabilities(cap_value_t* capabilityList, int length)
I think this should take a Vector<cap_value_t> instead of a pointer + length.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:201
> + cap_t capabilities;
Please put the declaration on the same line as the initialization.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:206
> + fprintf(stderr, "Process capabilities init failed: %s\n",
strerror(errno));
"Failed to initialize process capabilities"
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:210
> + if (cap_clear(capabilities)) {
Please check for -1 here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:218
> + 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)) {
Please check for -1 here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:219
> + fprintf(stderr, "Cannot set process capability flags: %s\n",
strerror(errno));
"Failed to ..."
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:226
> + fprintf(stderr, "Cannot set process capabilities: %s\n",
strerror(errno));
"Failed to ..."
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:231
> + if (cap_free(capabilities) == -1) {
I don't think you need to check if cap_free succeeds here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:232
> + fprintf(stderr, "Liberating capabilities failed: %s\n",
strerror(errno));
"Failed to ..."
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:242
> + if (prctl(PR_SET_DUMPABLE, 0, 0, 0, 0)) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:243
> + fprintf(stderr, "Setting dumplable is failed: %s\n",
strerror(errno));
Spelling error.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:248
> + if (setresgid(sandboxUserGid, sandboxUserGid, sandboxUserGid)
> + || setresuid(sandboxUserUid, sandboxUserUid, sandboxUserUid)) {
These should be two separate calls so you can better pinpoint which call failed
in case of failure.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:252
> + // Drop all capabilities. Again, setuid() normally takes care of this if
we had euid 0.
Please add an extra newline before the comment.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:259
> + struct stat fileStat;
> + return !(lstat(path, &fileStat) && errno == ENOENT);
Please rewrite this as two if statements to make it more clear.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:265
> + if (lstat(directory, &fileStat)) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:278
> + strlcpy(pathToCreateInSandbox, sandboxDirectory, PathSize);
> + strlcat(pathToCreateInSandbox, pathToCreate, PathSize);
It looks like this strlcpy/strlcat pattern is common, so I suggest you factor
it out into a "appendDirectoryComponent" function that takes a path and a name
and returns them concatenated.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:281
> + if (mkdir(pathToCreateInSandbox, mode)) {
Please check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:289
> + if (lstat(pathToCreate, &fileInfo)) {
Please check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:296
> + if (fileInfo.st_uid == getuid()) {
> + if (chown(pathToCreateInSandbox, sandboxUserUid, sandboxUserGid))
> + return false;
> + }
This could use some error reporting/explanation.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:298
> + if (chmod(pathToCreateInSandbox, fileInfo.st_mode))
> + return false;
Ditto.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:302
> +static bool createDirectoryPath(const char* path)
I don't think this name fully clarifies what the function does. Maybe something
to indicate that it creates all intermediate directories as well.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:306
> + char fullPathInSandbox[PathSize];
> + strlcpy(fullPathInSandbox, sandboxDirectory, PathSize);
> + strlcat(fullPathInSandbox, path, PathSize);
This is why I think you should have gone with C++ style strings...
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:318
> + strlcpy(nextDirectoryToCreate, startPos - 1, strnlen(startPos - 1,
endPos - startPos + 1) + 1);
Again, C++ style strings would have made this more clear.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:324
> + // Create the last directory of the directorypath.
missing space between directory and path.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:331
> + const char* dev = "/dev/";
I don't think you need the trailing slash here. I think devDirectory or devPath
would be a better variable name.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:336
> + for (int i = 0; i < 2; ++i) {
Might want to use the num-elements-in-array trick here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:339
> + fprintf(stderr, "Error by obtaining information about device
file (%s): %s\n", devices[i], strerror(errno));
"Failed to stat device file"
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:345
> + // their permissions should be: rw-rw-rw-.
No need for this comment to be on a separate line.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:350
> + if (mknod(device, S_IFCHR | S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP |
S_IROTH | S_IWOTH, makedev(major(dev), minor(dev)))) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:362
> + const char* proc = "/proc/";
Trailing slash. procPath.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:369
> + if (mount(proc, procPathInSandbox, "proc", 0, 0)) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:376
> + const char* sharedMemory = "/run/shm/";
Trailing slash. sharedMemoryPath.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:378
> + if (!createDirectoryPath(sharedMemory))
> + return false;
Don't you want to print out an error message in this case?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:383
> + if (mount(sharedMemory, sharedMemoryPathInSandbox, "tmpfs", 0, 0)) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:385
> + fprintf(stderr, "Error by mounting %s: %s\n", sharedMemory,
strerror(errno));
"Failed to mount '%s': %s.\n"
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:392
> +static bool linkFile(char* sourceFile, char* targetFile)
These should be const char*.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:395
> + bool isSymlink = true;
> + while (isSymlink) {
It looks like you can just do while (true) here since isSymlink is only set
once inside the loop and then you immediately break out of it.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:397
> + if (lstat(sourceFile, &fileInfo)) {
-1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:406
> + char* endOfBaseDirectoryInSource = strrchr(sourceFile, '/');
const char.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:413
> + strlcpy(baseDirectoryOfSource, sourceFile,
endOfBaseDirectoryInSource - sourceFile + 2);
Why +2?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:420
> + if (link(sourceFile, targetFile)) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:431
> + isSymlink = (fileInfo.st_mode & S_IFMT) == S_IFLNK;
> + if (!isSymlink)
> + break;
Again, you can just break directly here if this is false - no need to assign
it.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:438
> + char symlinkTargetInRealWorld[PathSize];
What does real world mean here?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:440
> + // Making difference between relative and absolute paths.
Please add an extra newline before the comment.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:458
> + strlcat(symlinkTargetInRealWorld, symlinkTarget, PathSize);
> +
> + strlcat(symlinkTargetInSandbox, sandboxDirectory, PathSize);
> + strlcat(symlinkTargetInSandbox, symlinkTarget, PathSize);
> + } else {
> + strlcat(symlinkTargetInRealWorld, baseDirectoryOfSource,
PathSize);
> + strlcat(symlinkTargetInRealWorld, "/", PathSize);
> + strlcat(symlinkTargetInRealWorld, symlinkTarget, PathSize);
> +
> + strlcat(symlinkTargetInSandbox, sandboxDirectory, PathSize);
> + strlcat(symlinkTargetInSandbox, "/", PathSize);
> + strlcat(symlinkTargetInSandbox, symlinkTargetInRealWorld,
PathSize);
Again, having a helper function for concatenating paths together would simplify
this a lot.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:465
> + sourceFile[0] = '\0';
> + targetFile[0] = '\0';
> + strlcat(sourceFile, symlinkTargetInRealWorld, PathSize);
> + strlcat(targetFile, symlinkTargetInSandbox, PathSize);
I see now why sourceFile and targetFile are not const pointers. I suggest that
you don't use the parameters like this but instead use your own variables
inside the function.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:470
> +static bool linkDirectory(const char* sourceDirectoryPath, const char*
targetDirectoryPath)
I think this function could use a comment indicating what it does or a better
name.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:481
> + struct dirent *directoryInfo = 0;
> + while ((directoryInfo = readdir(directory))) {
You can put the declaration iside the while statement here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:482
> + char* nextFileInDirectory = directoryInfo->d_name;
I don't think "next" adds anything here. Maybe just fileName?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:485
> + if (!strncmp(nextFileInDirectory, ".", strnlen(nextFileInDirectory,
PathSize)) || !strncmp(nextFileInDirectory, "..", 2))
> + continue;
It'd be more clear to just call strcmp directly here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:489
> + strlcpy(sourceFile, sourceDirectoryPath, PathSize);
> + strlcat(sourceFile, "/", PathSize);
> + strlcat(sourceFile, nextFileInDirectory, PathSize);
Same comment about adding a helper function.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:496
> + bool ok = true;
Give this a more descriptive name, such as "returnValue". No need to initialize
it.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:497
> + if ((directoryInfo->d_type == DT_DIR))
No need for the extra parentheses.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:506
> + // it could have meaning e.g. in the hashgeneration of cache files.
hash generation.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:508
> + if (lstat(sourceDirectoryPath, &fileStat)) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:512
> + struct utimbuf times = { fileStat.st_atime, fileStat.st_mtime };
Please don't use aggregate initialization for this.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:513
> + if (utime(targetDirectoryPath, ×)) {
Check for -1 here.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:514
> + fprintf(stderr, "Couldn't set back the last modification time of %s:
%s\n", targetDirectoryPath, strerror(errno));
It's always good to quote paths, so '%s'.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:532
> + while (*currentRuntimeDependency) {
Instead of null terminating this I think you should use a for-loop and count up
to the number of elements in the array.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:543
> + if (dlinfo(handle, RTLD_DI_LINKMAP, &linkMap)) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:552
> + char pathOfTheLibrary[PathSize];
I don't think this variable is needed, you can just access linkMap->l_name
directly.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:557
> + strlcpy(pathOfTheLibraryInSandbox, sandboxDirectory, PathSize);
> + strlcat(pathOfTheLibraryInSandbox, pathOfTheLibrary, PathSize);
Same comment about a helper function.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:560
> + if (dlclose(handle))
Check for -1 (or just remove the error handling altogether).
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:564
> + if (dlclose(handle)) {
Check for -1 (or just remove the error handling altogether).
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:578
> + char buffer[BUFSIZ];
What's BUFSIZ here?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:589
> + strlcpy(xauthorityOfRealUser, realUser->pw_dir, PathSize);
> + strlcat(xauthorityOfRealUser, "/.Xauthority", PathSize);
Same comment about a helper function.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:592
> + strlcpy(xauthorityInSandbox, sandboxDirectory, PathSize);
> + strlcat(xauthorityInSandbox, xauthorityOfRealUser, PathSize);
Same comment about a helper function.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:606
> + // We copy the .Xauthority file of the real user (instead of linking)
because nobody user
the 'nobody' user.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:609
> + while ((size = fread(buffer, 1, BUFSIZ, source)))
> + fwrite(buffer, 1, size, dest);
Don't you want to check that the write was successful here?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:621
> + if (chown(xauthorityInSandbox, sandboxUserUid, sandboxUserGid)) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:626
> + if (setenv("XAUTHORITY", xauthorityInSandbox, 1)) {
Check for -1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:633
> +static bool initSandbox()
Maybe initializeSandbox?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:636
> + // Create the sandbox directory. We only need to step into it, so
> + // the executable permission is needed only.
"step into it" is oddly worded.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:637
> + if (mkdir(sandboxDirectory, S_IFDIR | S_IXUSR | S_IXOTH)) {
-1.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:656
> + char localDirectory[PathSize];
> + strlcpy(localDirectory, home, PathSize);
> + strlcat(localDirectory, "/.local/share/Nokia/", PathSize);
Helper function!
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:660
> + char cacheDirectory[PathSize];
> + strlcpy(cacheDirectory, home, PathSize);
> + strlcat(cacheDirectory, "/.cache/Nokia/", PathSize);
Helper function!!
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:664
> + char fontDirectory[PathSize];
> + strlcpy(fontDirectory, home, PathSize);
> + strlcat(fontDirectory, "/.fontconfig/", PathSize);
Helper function!!!
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:678
> + const char** currentLinkedDirectory = linkedDirectories;
> + while (*currentLinkedDirectory) {
For loop instead of null terminating.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:681
> + strlcpy(linkedDirectoryInSandbox, sandboxDirectory, PathSize);
> + strlcat(linkedDirectoryInSandbox, *currentLinkedDirectory,
PathSize);
Helper function!!!!
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:697
> + cap_value_t capabiltyList[4];
capabiltyList is misspelled. Please do use aggregate initialization here and
use [] instead of a fixed size.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:714
> +static bool moveToNewPidNamespace()
moveToNewPIDNamespace.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:717
> + // We can't share FS accross namespaces.
FS -> filesystems.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:718
> + pid_t pid, expectedPid;
expectedPid-> expectedPID. (And both variables should be declared where they
are used).
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:720
> + pid = syscall(SYS_clone, CLONE_NEWPID | SIGCHLD, 0, 0, 0);
Again, you can initialize pid directly and why is syscall used instead of just
calling clone directly?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:722
> + switch (pid) {
Again, two if statements would be clearer.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:738
> + fprintf(stderr, "Waitpid is failed with: %s\n",
strerror(errno));
It's not waitpid that fails here, you should indicate that in your error
message.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:749
> +static bool run(int argc, char *const argv[])
Why does this need to be a separate function? Why can't this code just live in
main?
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:758
> + strlcpy(sandboxDirectory, userInfo->pw_dir, PathSize);
> + strlcat(sandboxDirectory, "/.wk2-sandbox", PathSize);
Helper function!
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:760
> + // Currently we use nobody user as the sandbox user and fallback to the
real user
The 'nobody' user. fallback -> fall back.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:761
> + // if it's failed. (We could extend this in the future with a specific
restricted user.)
if it's failed -> if we failed do get it? I think the period should go after
the ).
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:763
> + struct passwd* nobodyUser = getpwnam("nobody");
> + if (nobodyUser) {
Variable declaration can go inside the if statement.
> Source/WebKit2/Shared/linux/SandboxProcess/SandboxEnvironmentLinux.cpp:818
> + if (execl(argv[1], argv[1], argv[2], reinterpret_cast<char*>(0)) == -1)
{
Check for -1. Oh wait! :)
> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:175
> + QProcess* webProcessOrSUIDHelper;
> + webProcessOrSUIDHelper = new QtWebProcess();
Can just initialize the variable on the same line.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:159
> + long int sandboxSocketDescriptor = -1;
> + char* sandboxSocketDescriptorString;
> + char* helperPid;
> + char sandboxMeMessage = MSG_CHROOTME;
> + ssize_t numberOfCharacters;
> + pid_t helper = -1;
Please move these to where they're actually initialized.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:166
> + errno = 0;
No need to set errno here.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:167
> + sandboxSocketDescriptor = strtol(sandboxSocketDescriptorString, (char
**) 0, 10);
Extra space before the ** and the 0.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:168
> + if (errno || (sandboxSocketDescriptor == -1))
Actually, it'd be better to pass a second parameter to strtol and check that
it's \0 instead of checking the return value like this.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:172
> + helperPid = getenv(SANDBOX_HELPER_PID);
This variable should be called helperPID, or maybe sandboxHelperPID.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:178
> + errno = 0;
> + helper = strtol(helperPid, (char **) 0, 10);
> + if (errno || (helper == -1))
Same comment here about passing a parameter to strtol.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:181
> + // Send the chrootMe message to the helper.
Newline before the comment.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:187
> + // Read the acknowledgement message from the helper.
Newline before the comment.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:189
> + if ((numberOfCharacters != 1) || (sandboxMeMessage != MSG_CHROOTED)) {
No need for the extra parentheses.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:194
> + // Wait for the helper process.
Newline before the comment.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:195
> + int expectedPid = waitpid(helper, 0, 0);
expectedPID.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:196
> + if (expectedPid != -1 && ((helper == -1) || (expectedPid == helper)))
No need for the extra parentheses.
> Source/WebKit2/WebProcess/qt/WebProcessMainQt.cpp:207
> + pid_t helper;
> + helper = chrootMe();
Put the initialization and declaration on the same line.
> Tools/ChangeLog:6
> + Reviewed by Zoltan Herczeg.
And me!
More information about the webkit-reviews
mailing list