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

bin/process_win.cc

 // Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include <process.h>
 
 #include "bin/builtin.h"
 #include "bin/globals.h"
 #include "bin/process.h"
 #include "bin/eventhandler.h"
@@ skipping 55 matching lines @@
       }
       delete current;
       return;
     }
     prev = current;
     current = current->next();
   }
 }
 
 
+// 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,
                         PIPE_TYPE_BYTE | PIPE_WAIT,
                         1,             // Number of pipes
                         1024,          // Out buffer size
                         1024,          // In buffer size
                         0,             // Timeout in ms
                         NULL);
 
     if (handles[kWriteHandle] == INVALID_HANDLE_VALUE) {
       fprintf(stderr, "CreateNamedPipe failed %d\n", GetLastError());
       return false;
     }
 
     handles[kReadHandle] =
         CreateFile(pipe_name,
                    GENERIC_READ,
                    0,
                    &security_attributes,
                    OPEN_EXISTING,
                    FILE_READ_ATTRIBUTES | FILE_FLAG_OVERLAPPED,
                    NULL);
     if (handles[kReadHandle] == INVALID_HANDLE_VALUE) {
       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,
                         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,
                    &security_attributes,
                    OPEN_EXISTING,
                    FILE_WRITE_ATTRIBUTES | FILE_FLAG_OVERLAPPED,
                    NULL);
     if (handles[kWriteHandle] == INVALID_HANDLE_VALUE) {
       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;
 }
 
 
 static void CloseProcessPipe(HANDLE handles[2]) {
   for (int i = kReadHandle; i < kWriteHandle; i++) {
     if (handles[i] != INVALID_HANDLE_VALUE) {
       if (!CloseHandle(handles[i])) {
         fprintf(stderr, "CloseHandle failed %d\n", GetLastError());
@@ skipping 48 matching lines @@
   HANDLE stderr_handles[2] = { INVALID_HANDLE_VALUE, INVALID_HANDLE_VALUE };
   HANDLE exit_handles[2] = { INVALID_HANDLE_VALUE, INVALID_HANDLE_VALUE };
   // TODO(sgjesse): Find a better way of generating unique pipe names
   // (e.g. UUID).
   DWORD current_pid = GetCurrentProcessId();
   static int pipe_id = 0;
   char pipe_name[80];
   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);
     return error_code;
   }
   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);
     CloseProcessPipe(stderr_handles);
     return error_code;
   }
   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);
     CloseProcessPipe(stderr_handles);
     CloseProcessPipe(exit_handles);
     return error_code;
   }
 
   // Setup info structures.
   STARTUPINFO startup_info;
@@ skipping 109 matching lines @@
       return false;
     }
   }
   return true;
 }
 
 
 void Process::Exit(intptr_t id) {
   RemoveProcess(id);
 }