Improve process launch handle sharing API.

base/process/launch.h

Index: base/process/launch.h
diff --git a/base/process/launch.h b/base/process/launch.h
index b87de5a7ddbe0b71dda346f739bfb6a1177c4988..61c027111da131212e7ed224d8b24714bbd8cdff 100644
--- a/base/process/launch.h
+++ b/base/process/launch.h
@@ -16,6 +16,7 @@
 #include "base/base_export.h"
 #include "base/environment.h"
 #include "base/macros.h"
+#include "base/optional.h"

[grt (UTC plus 2), 2017/06/27 08:01:44]

unused?

[brettw, 2017/06/28 21:42:21]

Oh yeah, Optional was my first attempt :/

 #include "base/process/process.h"
 #include "base/process/process_handle.h"
 #include "base/strings/string_piece.h"
@@ -35,11 +36,9 @@ namespace base {
 
 class CommandLine;
 
-#if defined(OS_WIN)
-typedef std::vector<HANDLE> HandlesToInheritVector;
+#if defined(OS_POSIX)
+typedef std::vector<std::pair<int, int>> FileHandleMappingVector;
 #endif
-// TODO(viettrungluu): Only define this on POSIX?
-typedef std::vector<std::pair<int, int> > FileHandleMappingVector;
 
 // Options for launching a subprocess that are passed to LaunchProcess().
 // The default constructor constructs the object with default options.
@@ -75,17 +74,31 @@ struct BASE_EXPORT LaunchOptions {
 #if defined(OS_WIN)
   bool start_hidden = false;
 
-  // If non-null, inherit exactly the list of handles in this vector (these
-  // handles must be inheritable).
-  HandlesToInheritVector* handles_to_inherit = nullptr;
-
-  // If true, the new process inherits handles from the parent. In production
-  // code this flag should be used only when running short-lived, trusted
-  // binaries, because open handles from other libraries and subsystems will
-  // leak to the child process, causing errors such as open socket hangs.
-  // Note: If |handles_to_inherit| is non-null, this flag is ignored and only
-  // those handles will be inherited.
-  bool inherit_handles = false;
+  // Windows can inherit handles when it launches child processes.
+  // See https://blogs.msdn.microsoft.com/oldnewthing/20111216-00/?p=8873
+  // for a good overview of Windows handle inheritance.
+  //
+  // Implementation note: it might be nice to implement in terms of
+  // base::Optional<>, but then the natural default state (vector not present)
+  // would be "all inheritable handles" while we want "no inheritance."
+  enum InheritMode {

[grt (UTC plus 2), 2017/06/27 08:01:44]

nit: enum class?

[brettw, 2017/06/28 21:42:21]

Done.

+    // Only those handles in |handles_to_inherit| vector are inherited. If the
+    // vector is empty, no handles are inherited. The handles in the vector must
+    // all be inheritable.
+    INHERIT_SPECIFIC,

[grt (UTC plus 2), 2017/06/27 08:01:44]

nit: kFoo is now preferred
(https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5AqwVQt-Etg/dis...)

[brettw, 2017/06/28 21:42:21]

Done.

+
+    // All handles in the current process which are inheritable are inherited.
+    // In production code this flag should be used only when running
+    // short-lived, trusted binaries, because open handles from other libraries
+    // and subsystems will leak to the child process, causing errors such as
+    // open socket hangs. There are also race conditions that can cause handle
+    // over-sharing.
+    //
+    // |handles_to_inherit| must be null.
+    INHERIT_ALL
+  };
+  InheritMode inherit_mode = INHERIT_SPECIFIC;
+  std::vector<HANDLE> handles_to_inherit;
 
   // If non-null, runs as if the user represented by the token had launched it.
   // Whether the application is visible on the interactive desktop depends on
@@ -104,10 +117,16 @@ struct BASE_EXPORT LaunchOptions {
   // the job object fails.
   HANDLE job_handle = nullptr;
 
-  // Handles for the redirection of stdin, stdout and stderr. The handles must
-  // be inheritable. Caller should either set all three of them or none (i.e.
-  // there is no way to redirect stderr without redirecting stdin). The
-  // |inherit_handles| flag must be set to true when redirecting stdio stream.
+  // Handles for the redirection of stdin, stdout and stderr. The caller should
+  // either set all three of them or none (i.e. there is no way to redirect
+  // stderr without redirecting stdin).
+  //
+  // The handles must be inheritable. Pseudo handles are used when stdout and
+  // stderr redirect to the console. In that case, GetFileType() will return
+  // FILE_TYPE_CHAR and they're automatically inherited by child processes. See
+  // https://msdn.microsoft.com/en-us/library/windows/desktop/ms682075.aspx
+  // Otherwise, the caller must ensure that the |inherit_mode| and/or
+  // |handles_to_inherit| set so that the handles are inherited.
   HANDLE stdin_handle = nullptr;
   HANDLE stdout_handle = nullptr;
   HANDLE stderr_handle = nullptr;
@@ -126,11 +145,9 @@ struct BASE_EXPORT LaunchOptions {
   // |environ|.
   bool clear_environ = false;
 
-  // If non-null, remap file descriptors according to the mapping of
-  // src fd->dest fd to propagate FDs into the child process.
-  // This pointer is owned by the caller and must live through the
-  // call to LaunchProcess().
-  const FileHandleMappingVector* fds_to_remap = nullptr;
+  // Remap file descriptors according to the mapping of src_fd->dest_fd to
+  // propagate FDs into the child process.
+  FileHandleMappingVector fds_to_remap;
 
   // Each element is an RLIMIT_* constant that should be raised to its
   // rlim_max.  This pointer is owned by the caller and must live through