Make sure that the named pipe created for signalling the exit code is not inheritable

bin/process_win.cc

Index: bin/process_win.cc
===================================================================
--- bin/process_win.cc	(revision 390)
+++ bin/process_win.cc	(working copy)
@@ -73,18 +73,29 @@
 }
 
 
+// Types of pipes to create.
+enum NamedPipeType {
+  kInheritRead,
+  kInheritWrite,
+  kInheritNone
+};
+
+
 // Create a pipe for communicating with a new process. The handles array
-// will contain the read and write ends of the pipe. Based on the is_input
-// argument either the read or the write end of the handle will be
-// non-inherited.
+// will contain the read and write ends of the pipe. Based on the type
+// one of the handles will be inheritable.
+// NOTE: If this function returns false the handled might have been allocated

[Mads Ager (google), 2011/10/13 12:40:35]

handled -> handles

[Søren Gjesse, 2011/10/13 13:33:10]

Done.

+// and the caller should make sure to close then in case of an error.

[Mads Ager (google), 2011/10/13 12:40:35]

then -> them

[Søren Gjesse, 2011/10/13 13:33:10]

Done.

 static bool CreateProcessPipe(HANDLE handles[2],
                               char* pipe_name,
-                              bool is_input) {
+                              NamedPipeType type) {
+  // Security attributes describing an inheritable handle.
   SECURITY_ATTRIBUTES security_attributes;
   security_attributes.nLength = sizeof(SECURITY_ATTRIBUTES);
   security_attributes.bInheritHandle = TRUE;
   security_attributes.lpSecurityDescriptor = NULL;
-  if (is_input) {
+
+  if (type == kInheritRead) {
     handles[kWriteHandle] =
         CreateNamedPipe(pipe_name,
                         PIPE_ACCESS_OUTBOUND | FILE_FLAG_OVERLAPPED,
@@ -112,7 +123,7 @@
       fprintf(stderr, "CreateFile failed %d\n", GetLastError());
       return false;
     }
-  } else {
+  } else if (type == kInheritWrite) {

[Mads Ager (google), 2011/10/13 12:40:35]

InheritWrite and InheritNone are identical except for the pointer to a security
structure. Maybe extract a method?

[Søren Gjesse, 2011/10/13 13:33:10]

Combined the two and used a conditional for the security descriptor argument.

     handles[kReadHandle] =
         CreateNamedPipe(pipe_name,
                         PIPE_ACCESS_INBOUND | FILE_FLAG_OVERLAPPED,
@@ -140,6 +151,35 @@
       fprintf(stderr, "CreateFile failed %d\n", GetLastError());
       return false;
     }
+  } else {
+    ASSERT(type == kInheritNone);
+    handles[kReadHandle] =
+        CreateNamedPipe(pipe_name,
+                        PIPE_ACCESS_INBOUND | FILE_FLAG_OVERLAPPED,
+                        PIPE_TYPE_BYTE | PIPE_WAIT,
+                        1,             // Number of pipes
+                        1024,          // Out buffer size
+                        1024,          // In buffer size
+                        0,             // Timeout in ms
+                        NULL);
+
+    if (handles[kReadHandle] == INVALID_HANDLE_VALUE) {
+      fprintf(stderr, "CreateNamedPipe failed %d\n", GetLastError());
+      return false;
+    }
+
+    handles[kWriteHandle] =
+        CreateFile(pipe_name,
+                   GENERIC_WRITE,
+                   0,
+                   NULL,
+                   OPEN_EXISTING,
+                   FILE_WRITE_ATTRIBUTES | FILE_FLAG_OVERLAPPED,
+                   NULL);
+    if (handles[kWriteHandle] == INVALID_HANDLE_VALUE) {
+      fprintf(stderr, "CreateFile failed %d\n", GetLastError());
+      return false;
+    }
   }
   return true;
 }
@@ -208,14 +248,14 @@
   pipe_id++;
   snprintf(pipe_name, sizeof(pipe_name),
            "\\\\.\\Pipe\\dart%d_%d_1", current_pid, pipe_id);
-  if (!CreateProcessPipe(stdin_handles, pipe_name, true)) {
+  if (!CreateProcessPipe(stdin_handles, pipe_name, kInheritRead)) {
     int error_code = SetOsErrorMessage(os_error_message, os_error_message_len);
     CloseProcessPipe(stdin_handles);

[Mads Ager (google), 2011/10/13 12:40:35]

Can't you abstract all of these closing parts into one method that take pointers
to the handle arrays and close any non-INVALID_HANDLE_VALUE handles?

[Søren Gjesse, 2011/10/13 13:33:10]

Done.

     return error_code;
   }
   snprintf(pipe_name, sizeof(pipe_name),
            "\\\\.\\Pipe\\dart%d_%d_2", current_pid, pipe_id);
-  if (!CreateProcessPipe(stdout_handles, pipe_name, false)) {
+  if (!CreateProcessPipe(stdout_handles, pipe_name, kInheritWrite)) {
     int error_code = SetOsErrorMessage(os_error_message, os_error_message_len);
     CloseProcessPipe(stdin_handles);
     CloseProcessPipe(stdout_handles);
@@ -223,7 +263,7 @@
   }
   snprintf(pipe_name, sizeof(pipe_name),
            "\\\\.\\Pipe\\dart%d_%d_3", current_pid, pipe_id);
-  if (!CreateProcessPipe(stderr_handles, pipe_name, false)) {
+  if (!CreateProcessPipe(stderr_handles, pipe_name, kInheritWrite)) {
     int error_code = SetOsErrorMessage(os_error_message, os_error_message_len);
     CloseProcessPipe(stdin_handles);
     CloseProcessPipe(stdout_handles);
@@ -232,7 +272,7 @@
   }
   snprintf(pipe_name, sizeof(pipe_name),
            "\\\\.\\Pipe\\dart%d_%d_4", current_pid, pipe_id);
-  if (!CreateProcessPipe(exit_handles, pipe_name, false)) {
+  if (!CreateProcessPipe(exit_handles, pipe_name, kInheritNone)) {

[Mads Ager (google), 2011/10/13 12:40:35]

kDontInheritWrite?

Or maybe kReadInherit, kWriteInherit, kWrite?

I would like to have the word write appear here somewhere. You are creating the
process pipe for writing.

[Søren Gjesse, 2011/10/13 13:33:10]

The pipe is always created with both read and write ends. The type only
specifies whether one of these ends should be inheritable by the child. So I
think the current names are OK.

     int error_code = SetOsErrorMessage(os_error_message, os_error_message_len);
     CloseProcessPipe(stdin_handles);
     CloseProcessPipe(stdout_handles);