[io] Fix Process::Exec stdio handle inheritance

The code for inheriting stdio handles was not taking into account that
some of the stdio handles might be the same causing them to be
duplicated in the list of inherited handles. However passing duplicate
handles in PROC_THREAD_ATTRIBUTE_HANDLE_LIST causes CreateProcess to
return E_INVALIDARG.

This CL fixes the code to avoid duplicate handles.

We also fix printing of errors, which was broken when Process::Exec
was used without Dart scope causing it to print (null) instead of
actual error message.

Fixes https://github.com/dart-lang/sdk/issues/61981

TEST=vm/dart/regress_61981

Fixed: 61981
Change-Id: I93a6b56476f5020de9ebf894614cee51586b6751
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/464382
Reviewed-by: Alexander Aprelev <aam@google.com>
Commit-Queue: Slava Egorov <vegorov@google.com>

BUILD.gn

@@ -51,8 +51,8 @@ group("runtime") {
     "samples/embedder:kernel",
     "samples/ffi/http:fake_http",
     "samples/ffi/httpIG:fake_httpIG",
-    "utils/dartdev:dartdev",
-    "utils/kernel-service:kernel-service",
+    "utils/dartdev",
+    "utils/kernel-service",
   ]
 
   # The following dependencies allow dartdev to start the resident frontend
@@ -68,8 +68,8 @@ group("runtime") {
     ]
   } else {
     deps += [
-      "utils/dds:dds",
-      "utils/dtd:dtd",
+      "utils/dds",
+      "utils/dtd",
       "utils/kernel-service:frontend_server",
     ]
   }
@@ -104,6 +104,8 @@ group("runtime") {
     deps += [ "runtime/bin:abstract_socket_test" ]
   } else if (is_fuchsia) {
     deps += [ ":fuchsia_test_package" ]
+  } else if (is_win) {
+    deps += [ "runtime/bin:create_process_test_helper" ]
   }
 }
 

runtime/bin/BUILD.gn

@@ -1128,6 +1128,11 @@ executable("abstract_socket_test") {
   include_dirs = [ ".." ]
 }
 
+executable("create_process_test_helper") {
+  sources = [ "create_process_test_helper.cc" ]
+  include_dirs = [ ".." ]
+}
+
 source_set("run_vm_tests_set") {
   if (target_os == "fuchsia") {
     testonly = true

runtime/bin/create_process_test_helper.cc

@@ -0,0 +1,138 @@
+// Copyright (c) 2025, 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.
+
+// This is a utility program for testing that Dart binary correctly handles
+// situations when stdout and stderr handles are the same.
+//
+// This can happen in certain terminal emulators, e.g. one used by GitBash
+// see https://github.com/dart-lang/sdk/issues/61981 for an example.
+
+#include "platform/globals.h"
+#if defined(DART_HOST_OS_WINDOWS)
+
+#include <cstdio>
+#include <source_location>
+#include <sstream>
+#include <vector>
+
+struct StdioHandles {
+  HANDLE in;
+  HANDLE out;
+  HANDLE err;
+};
+
+static void ReportErrorAndAbort(
+    std::source_location location = std::source_location::current()) {
+  const auto code = GetLastError();
+
+  wchar_t buffer[512];
+  auto message_size =
+      FormatMessageW(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS,
+                     nullptr, code, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                     buffer, ARRAY_SIZE(buffer), nullptr);
+  if (message_size == 0) {
+    _snwprintf(buffer, ARRAY_SIZE(buffer), L"OS Error %d", code);
+  }
+  fprintf(stderr, "error at %s:%d: %ls", location.file_name(), location.line(),
+          buffer);
+  abort();
+}
+
+static void LaunchProcessWith(wchar_t* command_line,
+                              const StdioHandles& stdio_handles) {
+  fprintf(stderr, "LAUNCHING %ls\n", command_line);
+
+  // Setup info
+  STARTUPINFOEXW startup_info;
+  ZeroMemory(&startup_info, sizeof(startup_info));
+  startup_info.StartupInfo.cb = sizeof(startup_info);
+
+  // Setup the handles to inherit. We only want to inherit the three
+  // handles for stdin, stdout and stderr.
+  startup_info.StartupInfo.hStdInput = stdio_handles.in;
+  startup_info.StartupInfo.hStdOutput = stdio_handles.out;
+  startup_info.StartupInfo.hStdError = stdio_handles.err;
+  startup_info.StartupInfo.dwFlags = STARTF_USESTDHANDLES;
+  SIZE_T size = 0;
+  // The call to determine the size of an attribute list always fails with
+  // ERROR_INSUFFICIENT_BUFFER and that error should be ignored.
+  if (!InitializeProcThreadAttributeList(nullptr, 1, 0, &size) &&
+      (GetLastError() != ERROR_INSUFFICIENT_BUFFER)) {
+    return ReportErrorAndAbort();
+  }
+  auto attribute_list =
+      reinterpret_cast<LPPROC_THREAD_ATTRIBUTE_LIST>(malloc(size));
+  ZeroMemory(attribute_list, size);
+  if (!InitializeProcThreadAttributeList(attribute_list, 1, 0, &size)) {
+    return ReportErrorAndAbort();
+  }
+  std::vector<HANDLE> inherited_handles = {stdio_handles.in};
+  if (stdio_handles.out != stdio_handles.in) {
+    inherited_handles.push_back(stdio_handles.out);
+  }
+  if (stdio_handles.err != stdio_handles.out &&
+      stdio_handles.err != stdio_handles.in) {
+    inherited_handles.push_back(stdio_handles.err);
+  }
+  if (!UpdateProcThreadAttribute(
+          attribute_list, 0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
+          inherited_handles.data(), inherited_handles.size() * sizeof(HANDLE),
+          nullptr, nullptr)) {
+    return ReportErrorAndAbort();
+  }
+  startup_info.lpAttributeList = attribute_list;
+
+  PROCESS_INFORMATION process_info;
+  ZeroMemory(&process_info, sizeof(process_info));
+
+  // Create process.
+  BOOL result = CreateProcessW(
+      /*lpApplicationName=*/nullptr, command_line,
+      /*lpProcessAttributes=*/nullptr,
+      /*lpThreadAttributes=*/nullptr,
+      /*bInheritHandles=*/TRUE,
+      /*dwCreationFlags=*/EXTENDED_STARTUPINFO_PRESENT,
+      /*lpEnvironment=*/nullptr,
+      /*lpCurrentDirectory=*/nullptr,
+      reinterpret_cast<STARTUPINFOW*>(&startup_info), &process_info);
+
+  if (result == 0) {
+    return ReportErrorAndAbort();
+  }
+
+  WaitForSingleObject(process_info.hProcess, INFINITE);
+  CloseHandle(process_info.hProcess);
+  CloseHandle(process_info.hThread);
+}
+
+int main(int argc, char* argv[]) {
+  if (argc <= 1) {
+    fprintf(stderr, "Usage: %s <executable> <arg0> ... <argN>\n", argv[0]);
+    return -1;
+  }
+
+  // Generate command line. Assume that it does not contain any white-space
+  // in the arguments.
+  std::wstringstream wstr;
+  for (int i = 1; i < argc; i++) {
+    wstr << (i > 1 ? " " : "") << argv[i];
+  }
+
+  HANDLE stdin_handle = GetStdHandle(STD_INPUT_HANDLE);
+  HANDLE stdout_handle = GetStdHandle(STD_OUTPUT_HANDLE);
+  LaunchProcessWith(
+      wstr.str().data(),
+      {.in = stdin_handle, .out = stdout_handle, .err = stdout_handle});
+  CloseHandle(stdin_handle);
+  CloseHandle(stdout_handle);
+  return 0;
+}
+
+#else
+
+int main() {
+  return -1;
+}
+
+#endif  // defined(DART_HOST_OS_WINDOWS)

runtime/bin/process.cc

@@ -155,7 +155,7 @@ void FUNCTION_NAME(Process_Start)(Dart_NativeArguments args) {
   Dart_Handle stderr_handle = Dart_GetNativeArgument(args, 9);
   Dart_Handle exit_handle = Dart_GetNativeArgument(args, 10);
   intptr_t pid = -1;
-  char* os_error_message = nullptr;  // Scope allocated by Process::Start.
+  char* os_error_message = nullptr;
 
   int error_code = Process::Start(
       namespc, path, string_args, args_length, working_directory,

runtime/bin/process_win.cc

@@ -9,6 +9,7 @@
 
 #include <process.h>  // NOLINT
 #include <psapi.h>    // NOLINT
+#include <source_location>
 #include <vector>
 
 #include "bin/builtin.h"
@@ -294,12 +295,15 @@ static void CloseProcessPipes(HANDLE handles1[2],
   CloseProcessPipe(handles4);
 }
 
-static int SetOsErrorMessage(char** os_error_message) {
+static int SetOsErrorMessage(
+    CStringUniquePtr* os_error_message,
+    std::source_location location = std::source_location::current()) {
   int error_code = GetLastError();
   const int kMaxMessageLength = 256;
   wchar_t message[kMaxMessageLength];
   FormatMessageIntoBuffer(error_code, message, kMaxMessageLength);
-  *os_error_message = StringUtilsWin::WideToUtf8(message);
+  os_error_message->reset(Utils::SCreate(
+      "%ls (at %s:%d)", message, location.file_name(), location.line()));
   return error_code;
 }
 
@@ -356,7 +360,7 @@ class ProcessStarter {
                  intptr_t* err,
                  intptr_t* id,
                  intptr_t* exit_handler,
-                 char** os_error_message)
+                 CStringUniquePtr* os_error_message)
       : path_(path),
         working_directory_(working_directory),
         mode_(mode),
@@ -617,7 +621,13 @@ class ProcessStarter {
     if (!InitializeProcThreadAttributeList(attribute_list_, 1, 0, &size)) {
       return CleanupAndReturnError();
     }
-    inherited_handles_ = {stdin_handle, stdout_handle, stderr_handle};
+    inherited_handles_ = {stdin_handle};
+    if (stdout_handle != stdin_handle) {
+      inherited_handles_.push_back(stdout_handle);
+    }
+    if (stderr_handle != stdout_handle && stderr_handle != stdin_handle) {
+      inherited_handles_.push_back(stderr_handle);
+    }
     if (!UpdateProcThreadAttribute(
             attribute_list_, 0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
             inherited_handles_.data(),
@@ -707,8 +717,9 @@ class ProcessStarter {
     return 0;
   }
 
-  int CleanupAndReturnError() {
-    int error_code = SetOsErrorMessage(os_error_message_);
+  int CleanupAndReturnError(
+      std::source_location location = std::source_location::current()) {
+    int error_code = SetOsErrorMessage(os_error_message_, location);
     CloseProcessPipes(stdin_handles_, stdout_handles_, stderr_handles_,
                       exit_handles_);
     return error_code;
@@ -734,7 +745,7 @@ class ProcessStarter {
   intptr_t* err_;
   intptr_t* id_;
   intptr_t* exit_handler_;
-  char** os_error_message_;
+  CStringUniquePtr* os_error_message_;
 
  private:
   DISALLOW_ALLOCATION();
@@ -755,10 +766,15 @@ int Process::Start(Namespace* namespc,
                    intptr_t* id,
                    intptr_t* exit_handler,
                    char** os_error_message) {
+  CStringUniquePtr error;
   ProcessStarter starter(path, arguments, arguments_length, working_directory,
                          environment, environment_length, mode, in, out, err,
-                         id, exit_handler, os_error_message);
-  return starter.Start();
+                         id, exit_handler, &error);
+  const auto result = starter.Start();
+  if (result != 0 && error != nullptr) {
+    *os_error_message = DartUtils::ScopedCopyCString(error.get());
+  }
+  return result;
 }
 
 class BufferList : public BufferListBase {
@@ -981,7 +997,7 @@ int Process::Exec(Namespace* namespc,
   HANDLE hjob = CreateJobObject(nullptr, nullptr);
   if (hjob == nullptr) {
     BufferFormatter f(errmsg, errmsg_len);
-    f.Printf("Process::Exec - CreateJobObject failed %d\n", GetLastError());
+    f.Printf("Process::Exec - CreateJobObject failed %d", GetLastError());
     return -1;
   }
   JOBOBJECT_EXTENDED_LIMIT_INFORMATION info;
@@ -991,7 +1007,7 @@ int Process::Exec(Namespace* namespc,
                                  sizeof(JOBOBJECT_EXTENDED_LIMIT_INFORMATION),
                                  &qresult)) {
     BufferFormatter f(errmsg, errmsg_len);
-    f.Printf("Process::Exec - QueryInformationJobObject failed %d\n",
+    f.Printf("Process::Exec - QueryInformationJobObject failed %d",
              GetLastError());
     return -1;
   }
@@ -1005,7 +1021,7 @@ int Process::Exec(Namespace* namespc,
   if (!SetInformationJobObject(hjob, JobObjectExtendedLimitInformation, &info,
                                sizeof(JOBOBJECT_EXTENDED_LIMIT_INFORMATION))) {
     BufferFormatter f(errmsg, errmsg_len);
-    f.Printf("Process::Exec - SetInformationJobObject failed %d\n",
+    f.Printf("Process::Exec - SetInformationJobObject failed %d",
              GetLastError());
     return -1;
   }
@@ -1015,7 +1031,7 @@ int Process::Exec(Namespace* namespc,
   // we haven't spawned any children yet this race is harmless)
   if (!AssignProcessToJobObject(hjob, GetCurrentProcess())) {
     BufferFormatter f(errmsg, errmsg_len);
-    f.Printf("Process::Exec - AssignProcessToJobObject failed %d\n",
+    f.Printf("Process::Exec - AssignProcessToJobObject failed %d",
              GetLastError());
     return -1;
   }
@@ -1028,14 +1044,14 @@ int Process::Exec(Namespace* namespc,
   // as the value passed in 'path', we strip that off when starting the
   // process.
   intptr_t pid = -1;
-  char* os_error_message = nullptr;  // Scope allocated by Process::Start.
+  CStringUniquePtr os_error_message;
   ProcessStarter starter(path, &(arguments[1]), (arguments_length - 1),
                          working_directory, nullptr, 0, kInheritStdio, nullptr,
                          nullptr, nullptr, &pid, nullptr, &os_error_message);
   int result = starter.StartForExec(hjob);
   if (result != 0) {
     BufferFormatter f(errmsg, errmsg_len);
-    f.Printf("Process::Exec - %s\n", os_error_message);
+    f.Printf("ProcessStarter::StartForExec failed: %s", os_error_message.get());
     return -1;
   }
 
@@ -1047,14 +1063,14 @@ int Process::Exec(Namespace* namespc,
   DWORD wait_result = WaitForSingleObject(child_process, INFINITE);
   if (wait_result != WAIT_OBJECT_0) {
     BufferFormatter f(errmsg, errmsg_len);
-    f.Printf("Process::Exec - WaitForSingleObject failed %d\n", GetLastError());
+    f.Printf("Process::Exec - WaitForSingleObject failed %d", GetLastError());
     CloseHandle(child_process);
     return -1;
   }
   int retval;
   if (!GetExitCodeProcess(child_process, reinterpret_cast<DWORD*>(&retval))) {
     BufferFormatter f(errmsg, errmsg_len);
-    f.Printf("Process::Exec - GetExitCodeProcess failed %d\n", GetLastError());
+    f.Printf("Process::Exec - GetExitCodeProcess failed %d", GetLastError());
     CloseHandle(child_process);
     return -1;
   }

runtime/bin/utils_win.cc

@@ -46,9 +46,17 @@ void FormatMessageIntoBuffer(DWORD code, wchar_t* buffer, int buffer_length) {
                        code, GetLastError());
     }
     _snwprintf(buffer, buffer_length, L"OS Error %d", code);
+    return;
   }
-  // Ensure string termination.
-  buffer[buffer_length - 1] = 0;
+
+  // Strip trailing whitespace and a dot.
+  while (message_size > 0 && iswspace(buffer[message_size - 1])) {
+    message_size--;
+  }
+  if (message_size > 0 && buffer[message_size - 1] == L'.') {
+    message_size--;
+  }
+  buffer[Utils::Minimum<int>(message_size, buffer_length - 1)] = 0;
 }
 
 FILETIME GetFiletimeFromMillis(int64_t millis) {

runtime/tests/vm/dart/regress_61981_test.dart

@@ -0,0 +1,44 @@
+// Copyright (c) 2025, 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.
+
+// Test that Dart binary correctly handles situations when stdout and stderr
+// handles are the same.
+//
+// This can happen in certain terminal emulators, e.g. one used by GitBash
+// see https://github.com/dart-lang/sdk/issues/61981 for an example.
+
+import 'dart:io';
+
+import 'package:expect/expect.dart';
+import 'package:path/path.dart' as p;
+
+void main(List<String> args) {
+  if (args case ['child-process']) {
+    print('OK');
+    return;
+  }
+
+  if (!Platform.isWindows ||
+      p.basenameWithoutExtension(Platform.executable) != 'dart') {
+    return;
+  }
+
+  final createProcessHelper = p.join(
+    p.dirname(Platform.executable),
+    'create_process_test_helper.exe',
+  );
+
+  final result = Process.runSync(createProcessHelper, [
+    Platform.executable,
+    'run',
+    Platform.script.toFilePath(),
+    'child-process',
+  ]);
+  if (result.exitCode != 0) {
+    print(result.stdout);
+    print(result.stderr);
+    Expect.fail('process exited with ${result.exitCode}');
+  }
+  Expect.equals('OK', (result.stdout as String).trim());
+}