From aa69f34d53dfa0897aa00a7a1f0e97b3f6947d9b Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Mon, 13 Aug 2012 22:11:07 +0200 Subject: [PATCH] windows: report spawn errors to the exit callback Formerly spawn errors would be reported as a message printed to the process' stderr, to match unix behaviour. Unix has now been fixed to be more sensible, so this hack can now be removed. This also fixes a race condition that could occur when the user closes a process handle before the exit callback has been made. --- include/uv-private/uv-win.h | 8 +- src/win/internal.h | 5 +- src/win/pipe.c | 21 +- src/win/process-stdio.c | 66 +++--- src/win/process.c | 425 ++++++++++++++++++------------------ src/win/req-inl.h | 4 - test/test-spawn.c | 11 +- 7 files changed, 268 insertions(+), 272 deletions(-) diff --git a/include/uv-private/uv-win.h b/include/uv-private/uv-win.h index 9e0db118..dd418c3b 100644 --- a/include/uv-private/uv-win.h +++ b/include/uv-private/uv-win.h @@ -252,7 +252,6 @@ RB_HEAD(uv_timer_tree_s, uv_timer_s); UV_FS_EVENT_REQ, \ UV_POLL_REQ, \ UV_PROCESS_EXIT, \ - UV_PROCESS_CLOSE, \ UV_READ, \ UV_UDP_RECV, \ UV_WAKEUP, @@ -462,15 +461,12 @@ RB_HEAD(uv_timer_tree_s, uv_timer_s); struct uv_process_exit_s { \ UV_REQ_FIELDS \ } exit_req; \ - struct uv_process_close_s { \ - UV_REQ_FIELDS \ - } close_req; \ BYTE* child_stdio_buffer; \ + uv_err_t spawn_error; \ int exit_signal; \ - DWORD spawn_errno; \ HANDLE wait_handle; \ HANDLE process_handle; \ - HANDLE close_handle; + volatile char exit_cb_pending; #define UV_FS_PRIVATE_FIELDS \ int flags; \ diff --git a/src/win/internal.h b/src/win/internal.h index f46f6133..25b4e937 100644 --- a/src/win/internal.h +++ b/src/win/internal.h @@ -134,7 +134,7 @@ void uv_udp_endgame(uv_loop_t* loop, uv_udp_t* handle); /* * Pipes */ -int uv_stdio_pipe_server(uv_loop_t* loop, uv_pipe_t* handle, DWORD access, +uv_err_t uv_stdio_pipe_server(uv_loop_t* loop, uv_pipe_t* handle, DWORD access, char* name, size_t nameSize); int uv_pipe_listen(uv_pipe_t* handle, int backlog, uv_connection_cb cb); @@ -235,7 +235,6 @@ void uv_process_async_wakeup_req(uv_loop_t* loop, uv_async_t* handle, * Spawn */ void uv_process_proc_exit(uv_loop_t* loop, uv_process_t* handle); -void uv_process_proc_close(uv_loop_t* loop, uv_process_t* handle); void uv_process_close(uv_loop_t* loop, uv_process_t* handle); void uv_process_endgame(uv_loop_t* loop, uv_process_t* handle); @@ -294,7 +293,7 @@ uv_err_code uv_translate_sys_error(int sys_errno); /* * Process stdio handles. */ -int uv__stdio_create(uv_loop_t* loop, uv_process_options_t* options, +uv_err_t uv__stdio_create(uv_loop_t* loop, uv_process_options_t* options, BYTE** buffer_ptr); void uv__stdio_destroy(BYTE* buffer); void uv__stdio_noinherit(BYTE* buffer); diff --git a/src/win/pipe.c b/src/win/pipe.c index e0d08e1c..b41fc2cb 100644 --- a/src/win/pipe.c +++ b/src/win/pipe.c @@ -157,11 +157,11 @@ static HANDLE open_named_pipe(WCHAR* name, DWORD* duplex_flags) { } -int uv_stdio_pipe_server(uv_loop_t* loop, uv_pipe_t* handle, DWORD access, +uv_err_t uv_stdio_pipe_server(uv_loop_t* loop, uv_pipe_t* handle, DWORD access, char* name, size_t nameSize) { HANDLE pipeHandle; int errorno; - int err; + uv_err_t err; char* ptr = (char*)handle; for (;;) { @@ -179,9 +179,8 @@ int uv_stdio_pipe_server(uv_loop_t* loop, uv_pipe_t* handle, DWORD access, errorno = GetLastError(); if (errorno != ERROR_PIPE_BUSY && errorno != ERROR_ACCESS_DENIED) { - uv__set_sys_error(loop, errorno); - err = -1; - goto done; + err = uv__new_sys_error(errorno); + goto error; } /* Pipe name collision. Increment the pointer and try again. */ @@ -192,17 +191,17 @@ int uv_stdio_pipe_server(uv_loop_t* loop, uv_pipe_t* handle, DWORD access, loop->iocp, (ULONG_PTR)handle, 0) == NULL) { - uv__set_sys_error(loop, GetLastError()); - err = -1; - goto done; + err = uv__new_sys_error(GetLastError()); + goto error; } uv_pipe_connection_init(handle); handle->handle = pipeHandle; - err = 0; -done: - if (err && pipeHandle != INVALID_HANDLE_VALUE) { + return uv_ok_; + + error: + if (pipeHandle != INVALID_HANDLE_VALUE) { CloseHandle(pipeHandle); } diff --git a/src/win/process-stdio.c b/src/win/process-stdio.c index ad845a06..77777404 100644 --- a/src/win/process-stdio.c +++ b/src/win/process-stdio.c @@ -94,13 +94,14 @@ void uv_disable_stdio_inheritance(void) { } -static int uv__create_stdio_pipe_pair(uv_loop_t* loop, uv_pipe_t* server_pipe, - HANDLE* child_pipe_ptr, unsigned int flags) { +static uv_err_t uv__create_stdio_pipe_pair(uv_loop_t* loop, + uv_pipe_t* server_pipe, HANDLE* child_pipe_ptr, unsigned int flags) { char pipe_name[64]; SECURITY_ATTRIBUTES sa; DWORD server_access = 0; DWORD client_access = 0; HANDLE child_pipe = INVALID_HANDLE_VALUE; + uv_err_t err; if (flags & UV_READABLE_PIPE) { server_access |= PIPE_ACCESS_OUTBOUND; @@ -112,13 +113,13 @@ static int uv__create_stdio_pipe_pair(uv_loop_t* loop, uv_pipe_t* server_pipe, } /* Create server pipe handle. */ - if (uv_stdio_pipe_server(loop, - server_pipe, - server_access, - pipe_name, - sizeof(pipe_name)) < 0) { + err = uv_stdio_pipe_server(loop, + server_pipe, + server_access, + pipe_name, + sizeof(pipe_name)); + if (err.code != UV_OK) goto error; - } /* Create child pipe handle. */ sa.nLength = sizeof sa; @@ -133,7 +134,7 @@ static int uv__create_stdio_pipe_pair(uv_loop_t* loop, uv_pipe_t* server_pipe, server_pipe->ipc ? FILE_FLAG_OVERLAPPED : 0, NULL); if (child_pipe == INVALID_HANDLE_VALUE) { - uv__set_sys_error(loop, GetLastError()); + err = uv__new_sys_error(GetLastError()); goto error; } @@ -157,13 +158,13 @@ static int uv__create_stdio_pipe_pair(uv_loop_t* loop, uv_pipe_t* server_pipe, /* both ends of the pipe created. */ if (!ConnectNamedPipe(server_pipe->handle, NULL)) { if (GetLastError() != ERROR_PIPE_CONNECTED) { - uv__set_sys_error(loop, GetLastError()); + err = uv__new_sys_error(GetLastError()); goto error; } } *child_pipe_ptr = child_pipe; - return 0; + return uv_ok_; error: if (server_pipe->handle != INVALID_HANDLE_VALUE) { @@ -174,7 +175,7 @@ static int uv__create_stdio_pipe_pair(uv_loop_t* loop, uv_pipe_t* server_pipe, CloseHandle(child_pipe); } - return -1; + return err; } @@ -227,7 +228,7 @@ static int uv__duplicate_fd(uv_loop_t* loop, int fd, HANDLE* dup) { } -static int uv__create_nul_handle(uv_loop_t* loop, HANDLE* handle_ptr, +uv_err_t uv__create_nul_handle(HANDLE* handle_ptr, DWORD access) { HANDLE handle; SECURITY_ATTRIBUTES sa; @@ -244,36 +245,34 @@ static int uv__create_nul_handle(uv_loop_t* loop, HANDLE* handle_ptr, 0, NULL); if (handle == INVALID_HANDLE_VALUE) { - uv__set_sys_error(loop, GetLastError()); - return -1; + return uv__new_sys_error(GetLastError()); } *handle_ptr = handle; - return 0; + return uv_ok_; } -int uv__stdio_create(uv_loop_t* loop, uv_process_options_t* options, +uv_err_t uv__stdio_create(uv_loop_t* loop, uv_process_options_t* options, BYTE** buffer_ptr) { BYTE* buffer; int count, i; + uv_err_t err; count = options->stdio_count; if (count < 0 || count > 255) { /* Only support FDs 0-255 */ - uv__set_artificial_error(loop, UV_ENOTSUP); - return -1; + return uv__new_artificial_error(UV_ENOTSUP); } else if (count < 3) { /* There should always be at least 3 stdio handles. */ count = 3; } /* Allocate the child stdio buffer */ - buffer = malloc(CHILD_STDIO_SIZE(count)); + buffer = (BYTE*) malloc(CHILD_STDIO_SIZE(count)); if (buffer == NULL) { - uv__set_artificial_error(loop, UV_ENOMEM); - return -1; + return uv__new_artificial_error(UV_ENOMEM); } /* Prepopulate the buffer with INVALID_HANDLE_VALUE handles so we can */ @@ -304,11 +303,12 @@ int uv__stdio_create(uv_loop_t* loop, uv_process_options_t* options, if (i <= 2) { DWORD access = (i == 0) ? FILE_GENERIC_READ : FILE_GENERIC_WRITE | FILE_READ_ATTRIBUTES; - if (uv__create_nul_handle(loop, - &CHILD_STDIO_HANDLE(buffer, i), - access) < 0) { + + err = uv__create_nul_handle(&CHILD_STDIO_HANDLE(buffer, i), + access); + if (err.code != UV_OK) goto error; - } + CHILD_STDIO_CRT_FLAGS(buffer, i) = FOPEN | FDEV; } break; @@ -326,12 +326,12 @@ int uv__stdio_create(uv_loop_t* loop, uv_process_options_t* options, assert(!(fdopt.data.stream->flags & UV_HANDLE_CONNECTION)); assert(!(fdopt.data.stream->flags & UV_HANDLE_PIPESERVER)); - if (uv__create_stdio_pipe_pair(loop, - parent_pipe, - &child_pipe, - fdopt.flags) < 0) { + err = uv__create_stdio_pipe_pair(loop, + parent_pipe, + &child_pipe, + fdopt.flags); + if (err.code != UV_OK) goto error; - } CHILD_STDIO_HANDLE(buffer, i) = child_pipe; CHILD_STDIO_CRT_FLAGS(buffer, i) = FOPEN | FPIPE; @@ -430,11 +430,11 @@ int uv__stdio_create(uv_loop_t* loop, uv_process_options_t* options, } *buffer_ptr = buffer; - return 0; + return uv_ok_; error: uv__stdio_destroy(buffer); - return -1; + return err; } diff --git a/src/win/process.c b/src/win/process.c index 7479341a..a65a8aef 100644 --- a/src/win/process.c +++ b/src/win/process.c @@ -45,35 +45,52 @@ typedef struct env_var { #define E_V(str) { str "=", L##str, sizeof(str), 0, 0 } -#define UTF8_TO_UTF16(s, t) \ - size = uv_utf8_to_utf16(s, NULL, 0) * sizeof(wchar_t); \ - t = (wchar_t*)malloc(size); \ - if (!t) { \ - uv_fatal_error(ERROR_OUTOFMEMORY, "malloc"); \ - } \ - if (!uv_utf8_to_utf16(s, t, size / sizeof(wchar_t))) { \ - uv__set_sys_error(loop, GetLastError()); \ - err = -1; \ - goto done; \ +static uv_err_t uv_utf8_to_utf16_alloc(const char* s, WCHAR** ws_ptr) { + int ws_len, r; + WCHAR* ws; + + ws_len = MultiByteToWideChar(CP_UTF8, + 0, + s, + -1, + NULL, + 0); + if (ws_len <= 0) { + return uv__new_sys_error(GetLastError()); } + ws = (WCHAR*) malloc(ws_len * sizeof(WCHAR)); + if (ws == NULL) { + return uv__new_artificial_error(UV_ENOMEM); + } + + r = MultiByteToWideChar(CP_UTF8, + 0, + s, + -1, + ws, + ws_len); + assert(r == ws_len); + + *ws_ptr = ws; + return uv_ok_; +} + static void uv_process_init(uv_loop_t* loop, uv_process_t* handle) { uv__handle_init(loop, (uv_handle_t*) handle, UV_PROCESS); handle->exit_cb = NULL; handle->pid = 0; + handle->spawn_error = uv_ok_; handle->exit_signal = 0; handle->wait_handle = INVALID_HANDLE_VALUE; handle->process_handle = INVALID_HANDLE_VALUE; - handle->close_handle = INVALID_HANDLE_VALUE; handle->child_stdio_buffer = NULL; + handle->exit_cb_pending = 0; uv_req_init(loop, (uv_req_t*)&handle->exit_req); handle->exit_req.type = UV_PROCESS_EXIT; handle->exit_req.data = handle; - uv_req_init(loop, (uv_req_t*)&handle->close_req); - handle->close_req.type = UV_PROCESS_CLOSE; - handle->close_req.data = handle; } @@ -435,61 +452,92 @@ wchar_t* quote_cmd_arg(const wchar_t *source, wchar_t *target) { } -wchar_t* make_program_args(char** args, int verbatim_arguments) { - wchar_t* dst; - wchar_t* ptr; +uv_err_t make_program_args(char** args, int verbatim_arguments, WCHAR** dst_ptr) { char** arg; - size_t size = 0; - size_t len; + WCHAR* dst = NULL; + WCHAR* temp_buffer = NULL; + size_t dst_len = 0; + size_t temp_buffer_len = 0; + WCHAR* pos; int arg_count = 0; - wchar_t* buffer; - int arg_size; - int buffer_size = 0; + uv_err_t err = uv_ok_; /* Count the required size. */ for (arg = args; *arg; arg++) { - arg_size = uv_utf8_to_utf16(*arg, NULL, 0) * sizeof(wchar_t); - size += arg_size; - buffer_size = arg_size > buffer_size ? arg_size : buffer_size; + DWORD arg_len; + + arg_len = MultiByteToWideChar(CP_UTF8, + 0, + *arg, + -1, + NULL, + 0); + if (arg_len == 0) { + return uv__new_sys_error(GetLastError()); + } + + dst_len += arg_len; + + if (arg_len > temp_buffer_len) + temp_buffer_len = arg_len; + arg_count++; } - /* Adjust for potential quotes. Also assume the worst-case scenario + /* Adjust for potential quotes. Also assume the worst-case scenario */ /* that every character needs escaping, so we need twice as much space. */ - size = size * 2 + arg_count * 2; + dst_len = dst_len * 2 + arg_count * 2; - dst = (wchar_t*)malloc(size); - if (!dst) { - uv_fatal_error(ERROR_OUTOFMEMORY, "malloc"); + /* Allocate buffer for the final command line. */ + dst = (WCHAR*) malloc(dst_len * sizeof(WCHAR)); + if (dst == NULL) { + err = uv__new_artificial_error(UV_ENOMEM); + goto error; } - buffer = (wchar_t*)malloc(buffer_size); - if (!buffer) { - uv_fatal_error(ERROR_OUTOFMEMORY, "malloc"); + /* Allocate temporary working buffer. */ + temp_buffer = (WCHAR*) malloc(temp_buffer_len * sizeof(WCHAR)); + if (temp_buffer == NULL) { + err = uv__new_artificial_error(UV_ENOMEM); + goto error; } - ptr = dst; + pos = dst; for (arg = args; *arg; arg++) { - len = uv_utf8_to_utf16(*arg, buffer, (size_t)(size - (ptr - dst))); - if (!len) { + DWORD arg_len; + + /* Convert argument to wide char. */ + arg_len = MultiByteToWideChar(CP_UTF8, + 0, + *arg, + -1, + temp_buffer, + dst + dst_len - pos); + if (arg_len == 0) { goto error; } + if (verbatim_arguments) { - wcscpy(ptr, buffer); - ptr += len - 1; + /* Copy verbatim. */ + wcscpy(pos, temp_buffer); + pos += arg_len - 1; } else { - ptr = quote_cmd_arg(buffer, ptr); + /* Quote/escape, if needed. */ + pos = quote_cmd_arg(temp_buffer, pos); } - *ptr++ = *(arg + 1) ? L' ' : L'\0'; + + *pos++ = *(arg + 1) ? L' ' : L'\0'; } - free(buffer); - return dst; + free(temp_buffer); + + *dst_ptr = dst; + return uv_ok_; error: free(dst); - free(buffer); - return NULL; + free(temp_buffer); + return err; } @@ -597,74 +645,17 @@ wchar_t* make_program_env(char** env_block) { * a child process has exited. */ static void CALLBACK exit_wait_callback(void* data, BOOLEAN didTimeout) { - uv_process_t* process = (uv_process_t*)data; - uv_loop_t* loop = process->loop; - - assert(didTimeout == FALSE); - assert(process); - - /* Post completed */ - POST_COMPLETION_FOR_REQ(loop, &process->exit_req); -} - - -/* - * Called on Windows thread-pool thread to indicate that - * UnregisterWaitEx has completed. - */ -static void CALLBACK close_wait_callback(void* data, BOOLEAN didTimeout) { - uv_process_t* process = (uv_process_t*)data; - uv_loop_t* loop = process->loop; - - assert(didTimeout == FALSE); - assert(process); - - /* Post completed */ - POST_COMPLETION_FOR_REQ(loop, &process->close_req); -} - - -/* - * Called on windows thread pool when CreateProcess failed. It writes an error - * message to the process' intended stderr and then posts a PROCESS_EXIT - * packet to the completion port. - */ -static DWORD WINAPI spawn_failure(void* data) { - char syscall[] = "CreateProcessW: "; - char unknown[] = "unknown error\n"; uv_process_t* process = (uv_process_t*) data; uv_loop_t* loop = process->loop; - HANDLE child_stderr = uv__stdio_handle(process->child_stdio_buffer, 2); - char* buf = NULL; - DWORD count, written; - if (child_stderr != INVALID_HANDLE_VALUE) { - WriteFile(child_stderr, syscall, sizeof(syscall) - 1, &written, NULL); + assert(didTimeout == FALSE); + assert(process); + assert(!process->exit_cb_pending); - count = FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | - FORMAT_MESSAGE_FROM_SYSTEM | - FORMAT_MESSAGE_IGNORE_INSERTS, - NULL, - process->spawn_errno, - MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), - (LPSTR) &buf, - 0, - NULL); - - if (buf != NULL && count > 0) { - WriteFile(child_stderr, buf, count, &written, NULL); - LocalFree(buf); - } else { - WriteFile(child_stderr, unknown, sizeof(unknown) - 1, &written, NULL); - } - - FlushFileBuffers(child_stderr); - } + process->exit_cb_pending = 1; /* Post completed */ POST_COMPLETION_FOR_REQ(loop, &process->exit_req); - - return 0; } @@ -672,8 +663,13 @@ static DWORD WINAPI spawn_failure(void* data) { void uv_process_proc_exit(uv_loop_t* loop, uv_process_t* handle) { DWORD exit_code; - /* FIXME: race condition. */ + assert(handle->exit_cb_pending); + handle->exit_cb_pending = 0; + + /* If we're closing, don't call the exit callback. Just schedule a close */ + /* callback now. */ if (handle->flags & UV_HANDLE_CLOSING) { + uv_want_endgame(loop, (uv_handle_t*) handle); return; } @@ -683,68 +679,65 @@ void uv_process_proc_exit(uv_loop_t* loop, uv_process_t* handle) { handle->wait_handle = INVALID_HANDLE_VALUE; } - if (handle->process_handle == INVALID_HANDLE_VALUE || - !GetExitCodeProcess(handle->process_handle, &exit_code)) { - /* The process never even started in the first place, or we were unable */ - /* to obtain the exit code. */ - exit_code = 127; - } - /* Set the handle to inactive: no callbacks will be made after the exit */ /* callback.*/ uv__handle_stop(handle); + if (handle->spawn_error.code != UV_OK) { + /* Spawning failed. */ + exit_code = (DWORD) -1; + } else if (!GetExitCodeProcess(handle->process_handle, &exit_code)) { + /* Unable to to obtain the exit code. This should never happen. */ + exit_code = (DWORD) -1; + } + /* Fire the exit callback. */ if (handle->exit_cb) { + loop->last_err = handle->spawn_error; handle->exit_cb(handle, exit_code, handle->exit_signal); } } -/* Called on main thread after UnregisterWaitEx finishes. */ -void uv_process_proc_close(uv_loop_t* loop, uv_process_t* handle) { - uv_want_endgame(loop, (uv_handle_t*)handle); -} - - void uv_process_close(uv_loop_t* loop, uv_process_t* handle) { uv__handle_start(handle); if (handle->wait_handle != INVALID_HANDLE_VALUE) { - handle->close_handle = CreateEvent(NULL, FALSE, FALSE, NULL); - UnregisterWaitEx(handle->wait_handle, handle->close_handle); - handle->wait_handle = NULL; + /* This blocks until either the wait was cancelled, or the callback has */ + /* completed. */ + BOOL r = UnregisterWaitEx(handle->wait_handle, INVALID_HANDLE_VALUE); + if (!r) { + /* This should never happen, and if it happens, we can't recover... */ + uv_fatal_error(GetLastError(), "UnregisterWaitEx"); + } - RegisterWaitForSingleObject(&handle->wait_handle, handle->close_handle, - close_wait_callback, (void*)handle, INFINITE, - WT_EXECUTEINWAITTHREAD | WT_EXECUTEONLYONCE); - } else { + handle->wait_handle = INVALID_HANDLE_VALUE; + } + + if (!handle->exit_cb_pending) { uv_want_endgame(loop, (uv_handle_t*)handle); } } void uv_process_endgame(uv_loop_t* loop, uv_process_t* handle) { - if (handle->flags & UV_HANDLE_CLOSING) { - assert(!(handle->flags & UV_HANDLE_CLOSED)); - uv__handle_stop(handle); + assert(!handle->exit_cb_pending); + assert(handle->flags & UV_HANDLE_CLOSING); + assert(!(handle->flags & UV_HANDLE_CLOSED)); - /* Clean-up the process handle. */ - CloseHandle(handle->process_handle); + uv__handle_stop(handle); - /* Clean up the child stdio ends that may have been left open. */ - if (handle->child_stdio_buffer != NULL) { - uv__stdio_destroy(handle->child_stdio_buffer); - } + /* Clean-up the process handle. */ + CloseHandle(handle->process_handle); - uv__handle_close(handle); - } + uv__handle_close(handle); } int uv_spawn(uv_loop_t* loop, uv_process_t* process, uv_process_options_t options) { - int i, size, err = 0, keep_child_stdio_open = 0; + int i; + uv_err_t err = uv_ok_; wchar_t* path = NULL; BOOL result; wchar_t* application_path = NULL, *application = NULL, *arguments = NULL, @@ -758,6 +751,12 @@ int uv_spawn(uv_loop_t* loop, uv_process_t* process, return -1; } + if (options.file == NULL || + options.args == NULL) { + uv__set_artificial_error(loop, UV_EINVAL); + return -1; + } + assert(options.file != NULL); assert(!(options.flags & ~(UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS | UV_PROCESS_DETACHED | @@ -765,57 +764,84 @@ int uv_spawn(uv_loop_t* loop, uv_process_t* process, UV_PROCESS_SETUID))); uv_process_init(loop, process); - process->exit_cb = options.exit_cb; - UTF8_TO_UTF16(options.file, application); - arguments = options.args ? make_program_args(options.args, - options.flags & UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS) : NULL; - env = options.env ? make_program_env(options.env) : NULL; + + err = uv_utf8_to_utf16_alloc(options.file, &application); + if (err.code != UV_OK) + goto done; + + err = make_program_args(options.args, + options.flags & UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS, + &arguments); + if (err.code != UV_OK) + goto done; if (options.cwd) { - UTF8_TO_UTF16(options.cwd, cwd); - } else { - size = GetCurrentDirectoryW(0, NULL) * sizeof(wchar_t); - if (size) { - cwd = (wchar_t*)malloc(size); - if (!cwd) { - uv__set_artificial_error(loop, UV_ENOMEM); - err = -1; - goto done; - } + /* Explicit cwd */ + err = uv_utf8_to_utf16_alloc(options.cwd, &cwd); + if (err.code != UV_OK) + goto done; - GetCurrentDirectoryW(size, cwd); - } else { - uv__set_sys_error(loop, GetLastError()); - err = -1; + } else { + /* Inherit cwd */ + DWORD cwd_len, r; + + cwd_len = GetCurrentDirectoryW(0, NULL); + if (!cwd_len) { + err = uv__new_sys_error(GetLastError()); + goto done; + } + + cwd = (WCHAR*) malloc(cwd_len * sizeof(WCHAR)); + if (cwd == NULL) { + err = uv__new_artificial_error(UV_ENOMEM); + goto done; + } + + r = GetCurrentDirectoryW(cwd_len, cwd); + if (r == 0 || r >= cwd_len) { + err = uv__new_sys_error(GetLastError()); goto done; } } - /* Get PATH env. variable. */ - size = GetEnvironmentVariableW(L"PATH", NULL, 0) + 1; - path = (wchar_t*)malloc(size * sizeof(wchar_t)); - if (!path) { - uv_fatal_error(ERROR_OUTOFMEMORY, "malloc"); + /* Get PATH environment variable. */ + { + DWORD path_len, r; + + path_len = GetEnvironmentVariableW(L"PATH", NULL, 0); + if (path_len == 0) { + err = uv__new_sys_error(GetLastError()); + goto done; + } + + + path = (WCHAR*) malloc(path_len * sizeof(WCHAR)); + if (path == NULL) { + err = uv__new_artificial_error(UV_ENOMEM); + goto done; + } + + r = GetEnvironmentVariableW(L"PATH", path, path_len); + if (r == 0 || r >= path_len) { + err = uv__new_sys_error(GetLastError()); + goto done; + } } - GetEnvironmentVariableW(L"PATH", path, size * sizeof(wchar_t)); - path[size - 1] = L'\0'; application_path = search_path(application, cwd, path); - - if (!application_path) { - /* CreateProcess will fail, but this allows us to pass this error to */ - /* the user asynchronously. */ - application_path = application; + if (application_path == NULL) { + /* Not found. */ + err = uv__new_artificial_error(UV_ENOENT); + goto done; } - if (uv__stdio_create(loop, &options, &process->child_stdio_buffer) < 0) { - err = -1; - goto done; - } + err = uv__stdio_create(loop, &options, &process->child_stdio_buffer); + if (err.code != UV_OK) + goto done; startup.cb = sizeof(startup); startup.lpReserved = NULL; @@ -868,60 +894,37 @@ int uv_spawn(uv_loop_t* loop, uv_process_t* process, CloseHandle(info.hThread); } else { - /* CreateProcessW failed, but this failure should be delivered */ - /* asynchronously to retain unix compatibility. So pretend spawn */ - /* succeeded, and start a thread instead that prints an error */ - /* to the child's intended stderr. */ - process->spawn_errno = GetLastError(); - keep_child_stdio_open = 1; - if (!QueueUserWorkItem(spawn_failure, process, WT_EXECUTEDEFAULT)) { - uv_fatal_error(GetLastError(), "QueueUserWorkItem"); - } + /* CreateProcessW failed. */ + err = uv__new_sys_error(GetLastError()); } done: free(application); - if (application_path != application) { - free(application_path); - } + free(application_path); free(arguments); free(cwd); free(env); free(path); - /* Under normal circumstances we should close the stdio handles now - the */ - /* the child now has its own duplicates, or something went horribly wrong */ - /* The only exception is when CreateProcess has failed, then we actually */ - /* need to keep the stdio handles to report the error asynchronously. */ - if (process->child_stdio_buffer == NULL) { - /* Something went wrong before child stdio was initialized. */ - } else if (!keep_child_stdio_open) { + process->spawn_error = err; + + if (process->child_stdio_buffer != NULL) { + /* Clean up child stdio handles. */ uv__stdio_destroy(process->child_stdio_buffer); process->child_stdio_buffer = NULL; - } else { - /* We're keeping the handles open, the thread pool is going to have */ - /* it's way with them. But at least make them non-inheritable. */ - uv__stdio_noinherit(process->child_stdio_buffer); } - if (err == 0) { - /* Spawn was succesful. The handle will be active until the exit */ - /* is made or the handle is closed, whichever happens first. */ - uv__handle_start(process); - } else { - /* Spawn was not successful. Clean up. */ - if (process->wait_handle != INVALID_HANDLE_VALUE) { - UnregisterWait(process->wait_handle); - process->wait_handle = INVALID_HANDLE_VALUE; - } + /* Make the handle active. It will remain active until the exit callback */ + /* is made or the handle is closed, whichever happens first. */ + uv__handle_start(process); - if (process->process_handle != INVALID_HANDLE_VALUE) { - CloseHandle(process->process_handle); - process->process_handle = INVALID_HANDLE_VALUE; - } + /* If an error happened, queue the exit req. */ + if (err.code != UV_OK) { + process->exit_cb_pending = 1; + uv_insert_pending_req(loop, (uv_req_t*) &process->exit_req); } - return err; + return 0; } diff --git a/src/win/req-inl.h b/src/win/req-inl.h index 643171b2..541fb3c2 100644 --- a/src/win/req-inl.h +++ b/src/win/req-inl.h @@ -199,10 +199,6 @@ INLINE static void uv_process_reqs(uv_loop_t* loop) { uv_process_proc_exit(loop, (uv_process_t*) req->data); break; - case UV_PROCESS_CLOSE: - uv_process_proc_close(loop, (uv_process_t*) req->data); - break; - case UV_FS: uv_process_fs_req(loop, (uv_fs_t*) req); break; diff --git a/test/test-spawn.c b/test/test-spawn.c index a9232150..2feaf6db 100644 --- a/test/test-spawn.c +++ b/test/test-spawn.c @@ -589,8 +589,8 @@ TEST_IMPL(spawn_detect_pipe_name_collisions_on_windows) { } -wchar_t* make_program_args(char** args, int verbatim_arguments); -wchar_t* quote_cmd_arg(const wchar_t *source, wchar_t *target); +uv_err_t make_program_args(char** args, int verbatim_arguments, WCHAR** dst_ptr); +WCHAR* quote_cmd_arg(const wchar_t *source, wchar_t *target); TEST_IMPL(argument_escaping) { const wchar_t* test_str[] = { @@ -611,6 +611,7 @@ TEST_IMPL(argument_escaping) { size_t total_size = 0; int i; int num_args; + uv_err_t result; char* verbatim[] = { "cmd.exe", @@ -649,8 +650,10 @@ TEST_IMPL(argument_escaping) { free(test_output[i]); } - verbatim_output = make_program_args(verbatim, 1); - non_verbatim_output = make_program_args(verbatim, 0); + result = make_program_args(verbatim, 1, &verbatim_output); + ASSERT(result.code == UV_OK); + result = make_program_args(verbatim, 0, &non_verbatim_output); + ASSERT(result.code == UV_OK); wprintf(L" verbatim_output: %s\n", verbatim_output); wprintf(L"non_verbatim_output: %s\n", non_verbatim_output);