Process abstraction layer for NaCl

Process abstraction layer for NaCl

Provides cross-platform support for launching new processes, obtaining
process status and waiting for process exit. Implements subset of common
functionality across Linux, OS X and Windows.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=1239
TEST=small_tests

[Mark Seaborn, 2012-08-21 17:53:03]

Before I look more closely at the details, I am missing the bigger picture:

How does this relate to src/trusted/nonnacl_util/sel_ldr_launcher?  It looks
like there will be a lot of overlap.  Will you replace sel_ldr_launcher?

How do you intend to use this interface?  Will it only be used internally by
trusted code, and if so, how?  Or will you wrap it for use by untrusted code?

http://codereview.chromium.org/10832400/diff/1/site_scons/site_tools/library_...
File site_scons/site_tools/library_deps.py (right):

http://codereview.chromium.org/10832400/diff/1/site_scons/site_tools/library_...
site_scons/site_tools/library_deps.py:32: 'nacl_platform',
Why add this dependency when the 'sel' library does not depend on this yet?

http://codereview.chromium.org/10832400/diff/1/src/trusted/platform/build.scons
File src/trusted/platform/build.scons (right):

http://codereview.chromium.org/10832400/diff/1/src/trusted/platform/build.sco...
src/trusted/platform/build.scons:2: #
Adding a "src/trusted/platform" directory is a bit confusing when we have
"src/shared/platform".

Maybe this should be a new component that's not named "platform"?

[Petr Hosek, 2012-08-23 00:43:13]

http://codereview.chromium.org/10832400/diff/1/site_scons/site_tools/library_...
File site_scons/site_tools/library_deps.py (right):

http://codereview.chromium.org/10832400/diff/1/site_scons/site_tools/library_...
site_scons/site_tools/library_deps.py:32: 'nacl_platform',
On 2012/08/21 17:53:03, Mark Seaborn wrote:
> Why add this dependency when the 'sel' library does not depend on this yet?

Removed.

[Mark Seaborn, 2012-08-24 00:22:42]

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/build...
File src/trusted/platform/build.scons (right):

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/build...
src/trusted/platform/build.scons:15: elif env.Bit('linux'):
You could just use "else" instead of two "elif"s here.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/nacl_...
File src/trusted/platform/nacl_process.h (right):

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/nacl_...
src/trusted/platform/nacl_process.h:20: #if NACL_LINUX || NACL_OSX ||
defined(__native_client__)
Why "defined(__native_client__)" when this isn't built for untrusted code?

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/nacl_...
src/trusted/platform/nacl_process.h:37: #define NACL_PROCESS_LAUNCH_NEW_GROUP   
      0x1 /* new process group */
I am sceptical you really need this feature, and you also don't have any tests
for it, so unless there's a good reason to have it, I'd suggest you drop it.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/nacl_...
src/trusted/platform/nacl_process.h:54: * Attempts to kill the process
identified by the process handle |npp,
Missing "|": "|npp|"

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/nacl_...
File src/trusted/platform/nacl_process_test.c (right):

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/nacl_...
src/trusted/platform/nacl_process_test.c:60: NaClPlatformFini();
You don't really need to call this before exiting.  It would be simpler not to
call it.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/platf...
File src/trusted/platform/platform.gyp (right):

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/platf...
src/trusted/platform/platform.gyp:12: 'nacl_process.h',
FYI, you don't need to list header files in Gyp.  They are only listed for
displaying in IDEs, and NaCl devs aren't using IDEs this way.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/platf...
src/trusted/platform/platform.gyp:54: 'target_name': 'nacl_platform',
You're missing a definition of this for 64-bit Windows.  See other Gyp files for
examples.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
File src/trusted/platform/posix/nacl_process.c (right):

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
src/trusted/platform/posix/nacl_process.c:55: static const int kSignals[] = {
This is duplicating code from nacl_signal.c.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
src/trusted/platform/posix/nacl_process.c:64: #define NACL_MACH_EXCEPTION_MASK
EXC_MASK_BAD_ACCESS
This is duplicating code from osx/mach_exception_handler.c.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
src/trusted/platform/posix/nacl_process.c:124: dir_fd = open(kFDDir, O_RDONLY |
O_DIRECTORY);
I am sceptical you really want to close all FDs.  What is your use case?  Will
you be using this from a process where you can control all the FDs that will
open in advance?

The problem with using /proc/self/fd is that it doesn't work inside an outer
sandbox.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
src/trusted/platform/posix/nacl_process.c:141: rv = syscall(__NR_getdents64,
dir_fd, buf, sizeof buf);
Why are you using the getdents syscall directly?  What is wrong with
opendir()/readdir()?

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
src/trusted/platform/posix/nacl_process.c:188: * We deliberately ignore any
errors because we do
It would be better to check for EBADF and fail if errno doesn't match that.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
src/trusted/platform/posix/nacl_process.c:218: null_fd = open("/dev/null",
O_RDONLY);
This doesn't work inside an outer sandbox...

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
src/trusted/platform/posix/nacl_process.c:278: if (npp != NULL) {
You already did CHECK(npp != NULL) earlier

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
src/trusted/platform/posix/nacl_process.c:289: int NaClProcessKill(struct
NaClProcess *npp, int exit_code, int wait) {
I don't see any tests that cover this function...

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/posix...
src/trusted/platform/posix/nacl_process.c:301: retval = kill(npp->pid, SIGTERM);
Why are you trying SIGTERM and then trying SIGKILL after a timeout?  What's
wrong with using SIGKILL to start with?  The use case for this code is not
clear.  Timeouts are often wrong when you're not dealing with a network.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/win/n...
File src/trusted/platform/win/nacl_process.c (right):

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/win/n...
src/trusted/platform/win/nacl_process.c:23: #ifndef STATUS_SUCCESS
Aren't these defined by the Windows headers?  If so, you don't need definitions
of them here.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/win/n...
src/trusted/platform/win/nacl_process.c:39: static char
*NaClEscapeShellArg(const char *str) {
I don't know whether this properly implements Windows' strange quoting rules. 
Can you link to some documentation that explains Windows' quoting rules?  Also,
you should add some tests for it.

However: do we really need to pass arbitrary strings to a process via argv?  Can
we get away with passing any arguments via IPC instead?

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/win/n...
src/trusted/platform/win/nacl_process.c:76: ZeroMemory(&startup_info, sizeof
startup_info);
Using memset() is more usual, since it's a better-known standard function.

http://codereview.chromium.org/10832400/diff/12001/src/trusted/platform/win/n...
src/trusted/platform/win/nacl_process.c:153: if (WAIT_OBJECT_0 !=
WaitForSingleObject(npp->handle, 60 * 1000)) {
Why a hard-coded timeout?  What is the use case?