win: Don't try to inherit console handles in StartHandler()

client/crashpad_client_win.cc

 // Copyright 2015 The Crashpad Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
@@ skipping 114 matching lines @@
     static const auto delete_proc_thread_attribute_list =
         GET_FUNCTION_REQUIRED(L"kernel32.dll", ::DeleteProcThreadAttributeList);
     delete_proc_thread_attribute_list(proc_thread_attribute_list);
   }
 };
 
 using ScopedProcThreadAttributeList =
     base::ScopedGeneric<PPROC_THREAD_ATTRIBUTE_LIST,
                         ScopedProcThreadAttributeListTraits>;
 
+bool IsInheritableHandle(HANDLE handle) {
+  if (!handle || handle == INVALID_HANDLE_VALUE)
+    return false;
+
+  // File handles (FILE_TYPE_DISK) and pipe handles (FILE_TYPE_PIPE) are known
+  // to be inheritable. Console handles (FILE_TYPE_CHAR) are not inheritable via
+  // PROC_THREAD_ATTRIBUTE_HANDLE_LIST. See
+  // https://crashpad.chromium.org/bug/77.
+  DWORD handle_type = GetFileType(handle);
+  return handle_type == FILE_TYPE_DISK || handle_type == FILE_TYPE_PIPE;
+}
+
 // Adds |handle| to |handle_list| if it appears valid, and is not already in
 // |handle_list|.
 //
 // Invalid handles (including INVALID_HANDLE_VALUE and null handles) cannot be
 // added to a PPROC_THREAD_ATTRIBUTE_LIST’s PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
 // If INVALID_HANDLE_VALUE appears, CreateProcess() will fail with
 // ERROR_INVALID_PARAMETER. If a null handle appears, the child process will
 // silently not inherit any handles.
 //
 // Use this function to add handles with uncertain validities.
-void AddHandleToListIfValid(std::vector<HANDLE>* handle_list, HANDLE handle) {
+void AddHandleToListIfValidAndInheritable(std::vector<HANDLE>* handle_list,
+                                          HANDLE handle) {
   // There doesn't seem to be any documentation of this, but if there's a handle
   // duplicated in this list, CreateProcess() fails with
   // ERROR_INVALID_PARAMETER.
-  if (handle && handle != INVALID_HANDLE_VALUE &&
+  if (IsInheritableHandle(handle) &&
       std::find(handle_list->begin(), handle_list->end(), handle) ==
           handle_list->end()) {
     handle_list->push_back(handle);
   }
 }
 
 }  // namespace
 
 namespace crashpad {
 
@@ skipping 51 matching lines @@
         &command_line);
   }
   AppendCommandLineArgument(
       base::UTF8ToUTF16(base::StringPrintf("--handshake-handle=0x%x",
                                            HandleToInt(pipe_write))),
       &command_line);
 
   DWORD creation_flags;
   STARTUPINFOEX startup_info = {};
   startup_info.StartupInfo.dwFlags = STARTF_USESTDHANDLES;
   startup_info.StartupInfo.hStdInput = GetStdHandle(STD_INPUT_HANDLE);

[Mark Mentovai, 2015/11/25 04:58:05]

We probably shouldn’t even be setting hStdInput/Output/Error for these
FILE_TYPE_CHAR handles. We’re not seeing errors, but they don’t seem
duplicatable or inheritable at all, so setting them here can’t actually be
working for FILE_TYPE_CHAR.

[scottmg, 2015/11/25 18:13:58]

They are duplicatable I think. I'm not clear on whether they not inheritable, or
whether they're just not inherit-restrictable.

Did you have something in mind? We need to set a value for all of the handle if
we set if for any when setting STARTF_USESTDHANDLES.

   startup_info.StartupInfo.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE);
   startup_info.StartupInfo.hStdError = GetStdHandle(STD_ERROR_HANDLE);
 
   std::vector<HANDLE> handle_list;
   scoped_ptr<uint8_t[]> proc_thread_attribute_list_storage;
   ScopedProcThreadAttributeList proc_thread_attribute_list_owner;
 
   static const auto initialize_proc_thread_attribute_list =
       GET_FUNCTION(L"kernel32.dll", ::InitializeProcThreadAttributeList);
   static const auto update_proc_thread_attribute =
@@ skipping 28 matching lines @@
     rv = initialize_proc_thread_attribute_list(
         startup_info.lpAttributeList, 1, 0, &size);
     if (!rv) {
       PLOG(ERROR) << "InitializeProcThreadAttributeList";
       return false;
     }
     proc_thread_attribute_list_owner.reset(startup_info.lpAttributeList);
 
     handle_list.reserve(4);
     handle_list.push_back(pipe_write);
-    AddHandleToListIfValid(&handle_list, startup_info.StartupInfo.hStdInput);
-    AddHandleToListIfValid(&handle_list, startup_info.StartupInfo.hStdOutput);
-    AddHandleToListIfValid(&handle_list, startup_info.StartupInfo.hStdError);
+    AddHandleToListIfValidAndInheritable(&handle_list,
+                                         startup_info.StartupInfo.hStdInput);
+    AddHandleToListIfValidAndInheritable(&handle_list,
+                                         startup_info.StartupInfo.hStdOutput);
+    AddHandleToListIfValidAndInheritable(&handle_list,
+                                         startup_info.StartupInfo.hStdError);
     rv = update_proc_thread_attribute(
         startup_info.lpAttributeList,
         0,
         PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
         &handle_list[0],
         handle_list.size() * sizeof(handle_list[0]),
         nullptr,
         nullptr);
     if (!rv) {
       PLOG(ERROR) << "UpdateProcThreadAttribute";
@@ skipping 158 matching lines @@
   DWORD wfso_result = WaitForSingleObject(g_non_crash_dump_done, INFINITE);
   PLOG_IF(ERROR, wfso_result != WAIT_OBJECT_0) << "WaitForSingleObject";
 }
 
 // static
 void CrashpadClient::DumpAndCrash(EXCEPTION_POINTERS* exception_pointers) {
   UnhandledExceptionHandler(exception_pointers);
 }
 
 }  // namespace crashpad