Bug 1330496 - Part 1: Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP. r?bobowen draft
authorChris Peterson <cpeterson@mozilla.com>
Tue, 10 Jan 2017 23:50:16 -0800
changeset 459513 44e03181133ceff8c0777c05bf564296733b5b64
parent 459458 13603af3862d9583ed2feefb06e0988c2d7fed8c
child 459514 18de43b5a42c1c8bdf2bc9783453df5ac928a15c
child 459516 5de185791d7cd032b6359d4a82205fcfeb7c21f8
push id41248
push usercpeterson@mozilla.com
push dateThu, 12 Jan 2017 03:23:49 +0000
reviewersbobowen
bugs1330496
milestone53.0a1
Bug 1330496 - Part 1: Remove MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA support for inheriting stdout/stderr handles on XP. r?bobowen MozReview-Commit-ID: B7qJdK2sjv5
ipc/chromium/src/base/process_util_win.cc
security/sandbox/chromium/sandbox/win/src/broker_services.cc
--- a/ipc/chromium/src/base/process_util_win.cc
+++ b/ipc/chromium/src/base/process_util_win.cc
@@ -14,19 +14,16 @@
 #include <winternl.h>
 #include <psapi.h>
 
 #include "base/histogram.h"
 #include "base/logging.h"
 #include "base/win_util.h"
 
 #include <algorithm>
-#include "prenv.h"
-
-#include "mozilla/WindowsVersion.h"
 
 namespace {
 
 // System pagesize. This value remains constant on x86/64 architectures.
 const int PAGESIZE_KB = 4;
 
 // HeapSetInformation function pointer.
 typedef BOOL (WINAPI* HeapSetFn)(HANDLE, HEAP_INFORMATION_CLASS, PVOID, SIZE_T);
@@ -277,19 +274,17 @@ void FreeThreadAttributeList(LPPROC_THRE
 bool LaunchApp(const std::wstring& cmdline,
                bool wait, bool start_hidden, ProcessHandle* process_handle) {
 
   // We want to inherit the std handles so dump() statements and assertion
   // messages in the child process can be seen - but we *do not* want to
   // blindly have all handles inherited.  Vista and later has a technique
   // where only specified handles are inherited - so we use this technique if
   // we can.  If that technique isn't available (or it fails), we just don't
-  // inherit anything.  This can cause us a problem for Windows XP testing,
-  // because we sometimes need the handles to get inherited for test logging to
-  // work. So we also inherit when a specific environment variable is set.
+  // inherit anything.
   DWORD dwCreationFlags = 0;
   BOOL bInheritHandles = FALSE;
   // We use a STARTUPINFOEX, but if we can't do the thread attribute thing, we
   // just pass the size of a STARTUPINFO.
   STARTUPINFOEX startup_info_ex;
   ZeroMemory(&startup_info_ex, sizeof(startup_info_ex));
   STARTUPINFO &startup_info = startup_info_ex.StartupInfo;
   startup_info.cb = sizeof(startup_info);
@@ -297,50 +292,39 @@ bool LaunchApp(const std::wstring& cmdli
   startup_info.wShowWindow = start_hidden ? SW_HIDE : SW_SHOW;
 
   // Per the comment in CreateThreadAttributeList, lpAttributeList will contain
   // a pointer to handlesToInherit, so make sure they have the same lifetime.
   LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList = NULL;
   HANDLE handlesToInherit[2];
   int handleCount = 0;
 
-  // Don't even bother trying pre-Vista...
-  if (mozilla::IsVistaOrLater()) {
-    // setup our handle array first - if we end up with no handles that can
-    // be inherited we can avoid trying to do the ThreadAttributeList dance...
-    HANDLE stdOut = ::GetStdHandle(STD_OUTPUT_HANDLE);
-    HANDLE stdErr = ::GetStdHandle(STD_ERROR_HANDLE);
+  // setup our handle array first - if we end up with no handles that can
+  // be inherited we can avoid trying to do the ThreadAttributeList dance...
+  HANDLE stdOut = ::GetStdHandle(STD_OUTPUT_HANDLE);
+  HANDLE stdErr = ::GetStdHandle(STD_ERROR_HANDLE);
 
-    if (IsInheritableHandle(stdOut))
-      handlesToInherit[handleCount++] = stdOut;
-    if (stdErr != stdOut && IsInheritableHandle(stdErr))
-      handlesToInherit[handleCount++] = stdErr;
+  if (IsInheritableHandle(stdOut))
+    handlesToInherit[handleCount++] = stdOut;
+  if (stdErr != stdOut && IsInheritableHandle(stdErr))
+    handlesToInherit[handleCount++] = stdErr;
 
-    if (handleCount) {
-      lpAttributeList = CreateThreadAttributeList(handlesToInherit, handleCount);
-      if (lpAttributeList) {
-        // it's safe to inherit handles, so arrange for that...
-        startup_info.cb = sizeof(startup_info_ex);
-        startup_info.dwFlags |= STARTF_USESTDHANDLES;
-        startup_info.hStdOutput = stdOut;
-        startup_info.hStdError = stdErr;
-        startup_info.hStdInput = INVALID_HANDLE_VALUE;
-        startup_info_ex.lpAttributeList = lpAttributeList;
-        dwCreationFlags |= EXTENDED_STARTUPINFO_PRESENT;
-        bInheritHandles = TRUE;
-      }
+  if (handleCount) {
+    lpAttributeList = CreateThreadAttributeList(handlesToInherit, handleCount);
+    if (lpAttributeList) {
+      // it's safe to inherit handles, so arrange for that...
+      startup_info.cb = sizeof(startup_info_ex);
+      startup_info.dwFlags |= STARTF_USESTDHANDLES;
+      startup_info.hStdOutput = stdOut;
+      startup_info.hStdError = stdErr;
+      startup_info.hStdInput = INVALID_HANDLE_VALUE;
+      startup_info_ex.lpAttributeList = lpAttributeList;
+      dwCreationFlags |= EXTENDED_STARTUPINFO_PRESENT;
+      bInheritHandles = TRUE;
     }
-  } else if (PR_GetEnv("MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA")) {
-    // Even if we can't limit what gets inherited, we sometimes want to inherit
-    // stdout/err for testing purposes.
-    startup_info.dwFlags |= STARTF_USESTDHANDLES;
-    startup_info.hStdOutput = ::GetStdHandle(STD_OUTPUT_HANDLE);
-    startup_info.hStdError = ::GetStdHandle(STD_ERROR_HANDLE);
-    startup_info.hStdInput = INVALID_HANDLE_VALUE;
-    bInheritHandles = TRUE;
   }
 
   PROCESS_INFORMATION process_info;
   BOOL createdOK = CreateProcess(NULL,
                      const_cast<wchar_t*>(cmdline.c_str()), NULL, NULL,
                      bInheritHandles, dwCreationFlags, NULL, NULL,
                      &startup_info, &process_info);
   if (lpAttributeList)
--- a/security/sandbox/chromium/sandbox/win/src/broker_services.cc
+++ b/security/sandbox/chromium/sandbox/win/src/broker_services.cc
@@ -404,24 +404,16 @@ ResultCode BrokerServicesBase::SpawnTarg
       startup_info.startup_info()->dwFlags |= STARTF_USESTDHANDLES;
       startup_info.startup_info()->hStdInput = INVALID_HANDLE_VALUE;
       startup_info.startup_info()->hStdOutput = stdout_handle;
       startup_info.startup_info()->hStdError = stderr_handle;
       // Allowing inheritance of handles is only secure now that we
       // have limited which handles will be inherited.
       inherit_handles = true;
     }
-  } else if (getenv("MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA")) {
-    // On pre-Vista versions even if we can't limit what gets inherited, we
-    // sometimes want to inherit stdout/err for testing purposes.
-    startup_info.startup_info()->dwFlags |= STARTF_USESTDHANDLES;
-    startup_info.startup_info()->hStdInput = INVALID_HANDLE_VALUE;
-    startup_info.startup_info()->hStdOutput = policy_base->GetStdoutHandle();
-    startup_info.startup_info()->hStdError = policy_base->GetStderrHandle();
-    inherit_handles = true;
   }
 
   // Construct the thread pool here in case it is expensive.
   // The thread pool is shared by all the targets
   if (NULL == thread_pool_)
     thread_pool_ = new Win2kThreadPool();
 
   // Create the TargetProces object and spawn the target suspended. Note that