From 473149f0b1f07db29a9285ef75d32a921c0425d7 Mon Sep 17 00:00:00 2001 From: Slava Egorov Date: Tue, 25 Nov 2025 11:59:21 -0800 Subject: [PATCH] [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 Commit-Queue: Slava Egorov --- BUILD.gn | 10 +- runtime/bin/BUILD.gn | 5 + runtime/bin/create_process_test_helper.cc | 138 ++++++++++++++++++ runtime/bin/process.cc | 2 +- runtime/bin/process_win.cc | 50 ++++--- runtime/bin/utils_win.cc | 12 +- runtime/tests/vm/dart/regress_61981_test.dart | 44 ++++++ 7 files changed, 237 insertions(+), 24 deletions(-) create mode 100644 runtime/bin/create_process_test_helper.cc create mode 100644 runtime/tests/vm/dart/regress_61981_test.dart diff --git a/BUILD.gn b/BUILD.gn index 25f4b6a5c3b..6f33af71f1a 100644 --- a/BUILD.gn +++ b/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" ] } } diff --git a/runtime/bin/BUILD.gn b/runtime/bin/BUILD.gn index df60370a61a..6785943045d 100644 --- a/runtime/bin/BUILD.gn +++ b/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 diff --git a/runtime/bin/create_process_test_helper.cc b/runtime/bin/create_process_test_helper.cc new file mode 100644 index 00000000000..5e37e03e2b0 --- /dev/null +++ b/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 +#include +#include +#include + +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(malloc(size)); + ZeroMemory(attribute_list, size); + if (!InitializeProcThreadAttributeList(attribute_list, 1, 0, &size)) { + return ReportErrorAndAbort(); + } + std::vector 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(&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 ... \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) diff --git a/runtime/bin/process.cc b/runtime/bin/process.cc index 027734d06fa..46918237363 100644 --- a/runtime/bin/process.cc +++ b/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, diff --git a/runtime/bin/process_win.cc b/runtime/bin/process_win.cc index 902797b79e5..01368f7cb85 100644 --- a/runtime/bin/process_win.cc +++ b/runtime/bin/process_win.cc @@ -9,6 +9,7 @@ #include // NOLINT #include // NOLINT +#include #include #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(&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; } diff --git a/runtime/bin/utils_win.cc b/runtime/bin/utils_win.cc index c73d97299bb..7a0217e0a5c 100644 --- a/runtime/bin/utils_win.cc +++ b/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(message_size, buffer_length - 1)] = 0; } FILETIME GetFiletimeFromMillis(int64_t millis) { diff --git a/runtime/tests/vm/dart/regress_61981_test.dart b/runtime/tests/vm/dart/regress_61981_test.dart new file mode 100644 index 00000000000..2fa8b7668fe --- /dev/null +++ b/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 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()); +}