Module: wine Branch: master Commit: e38f3761f890ccc0b32ac16c633cdf7582511133 URL: https://source.winehq.org/git/wine.git/?a=commit;h=e38f3761f890ccc0b32ac16c6...
Author: Piotr Caban piotr@codeweavers.com Date: Mon Oct 25 18:29:00 2021 +0200
msvcrt: Rewrite _popen function.
Old implementation was not thread safe, incorrectly copied file descriptors to child process and was leaking parent pipe fd to child process.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51719 Signed-off-by: Piotr Caban piotr@codeweavers.com Signed-off-by: Alexandre Julliard julliard@winehq.org
---
dlls/msvcrt/process.c | 217 ++++++++++++++++++++++++++--------------------- dlls/msvcrt/tests/misc.c | 50 +++++++++-- 2 files changed, 166 insertions(+), 101 deletions(-)
diff --git a/dlls/msvcrt/process.c b/dlls/msvcrt/process.c index f2240270179..630e378df87 100644 --- a/dlls/msvcrt/process.c +++ b/dlls/msvcrt/process.c @@ -1040,113 +1040,140 @@ void msvcrt_free_popen_data(void) */ FILE* CDECL _wpopen(const wchar_t* command, const wchar_t* mode) { - FILE *ret; - BOOL readPipe = TRUE; - int textmode, fds[2], fdToDup, fdToOpen, fdStdHandle = -1, fmode; - const wchar_t *p; - wchar_t *comspec, *fullcmd; - unsigned int len; - struct popen_handle *container; - DWORD i; - - TRACE("(command=%s, mode=%s)\n", debugstr_w(command), debugstr_w(mode)); - - if (!command || !mode) - return NULL; - - _get_fmode(&fmode); - textmode = fmode & (_O_BINARY | _O_TEXT); - for (p = mode; *p; p++) - { - switch (*p) + wchar_t *comspec, *fullcmd, fullname[MAX_PATH]; + int textmode, fds[2], fd_parent, fd_child; + struct popen_handle *container; + PROCESS_INFORMATION pi; + BOOL read_pipe = TRUE; + const wchar_t *p; + unsigned int len; + STARTUPINFOW si; + FILE *ret; + DWORD i; + BOOL r; + + TRACE("(command=%s, mode=%s)\n", debugstr_w(command), debugstr_w(mode)); + + if (!command || !mode) + return NULL; + + _get_fmode(&textmode); + textmode &= _O_BINARY | _O_TEXT; + textmode |= _O_NOINHERIT; + for (p = mode; *p; p++) { - case 'W': - case 'w': - readPipe = FALSE; - break; - case 'B': - case 'b': - textmode |= _O_BINARY; - textmode &= ~_O_TEXT; - break; - case 'T': - case 't': - textmode |= _O_TEXT; - textmode &= ~_O_BINARY; - break; + switch (*p) + { + case 'W': + case 'w': + read_pipe = FALSE; + break; + case 'B': + case 'b': + textmode |= _O_BINARY; + textmode &= ~_O_TEXT; + break; + case 'T': + case 't': + textmode |= _O_TEXT; + textmode &= ~_O_BINARY; + break; + } } - } - if (_pipe(fds, 0, textmode) == -1) - return NULL; + if (_pipe(fds, 0, textmode) == -1) + return NULL;
- fdToDup = readPipe ? 1 : 0; - fdToOpen = readPipe ? 0 : 1; + if (read_pipe) + { + fd_parent = fds[0]; + fd_child = _dup(fds[1]); + _close(fds[1]); + } + else + { + fd_parent = fds[1]; + fd_child = _dup(fds[0]); + _close(fds[0]); + } + if (fd_child == -1) + { + _close(fd_parent); + return NULL; + } + ret = _wfdopen(fd_parent, mode); + if (!ret) + { + _close(fd_child); + return NULL; + }
- _lock(_POPEN_LOCK); - for(i=0; i<popen_handles_size; i++) - { - if (!popen_handles[i].f) - break; - } - if (i==popen_handles_size) - { - i = (popen_handles_size ? popen_handles_size*2 : 8); - container = realloc(popen_handles, i*sizeof(*container)); - if (!container) goto error; - - popen_handles = container; - container = popen_handles+popen_handles_size; - memset(container, 0, (i-popen_handles_size)*sizeof(*container)); - popen_handles_size = i; - } - else container = popen_handles+i; + _lock(_POPEN_LOCK); + for (i = 0; i < popen_handles_size; i++) + { + if (!popen_handles[i].f) + break; + } + if (i == popen_handles_size) + { + i = (popen_handles_size ? popen_handles_size * 2 : 8); + container = realloc(popen_handles, i * sizeof(*container)); + if (!container) goto error; + + popen_handles = container; + container = popen_handles + popen_handles_size; + memset(container, 0, (i - popen_handles_size) * sizeof(*container)); + popen_handles_size = i; + } + else container = popen_handles + i;
- if ((fdStdHandle = _dup(fdToDup)) == -1) - goto error; - if (_dup2(fds[fdToDup], fdToDup) != 0) - goto error; + if (!(comspec = msvcrt_get_comspec())) goto error; + len = wcslen(comspec) + wcslen(command) + 5;
- _close(fds[fdToDup]); + if (!(fullcmd = HeapAlloc(GetProcessHeap(), 0, len * sizeof(wchar_t)))) + { + HeapFree(GetProcessHeap(), 0, comspec); + goto error; + }
- if (!(comspec = msvcrt_get_comspec())) goto error; - len = wcslen(comspec) + wcslen(command) + 5; + wcscpy(fullcmd, comspec); + wcscat(fullcmd, L" /c "); + wcscat(fullcmd, command); + msvcrt_search_executable(comspec, fullname, 1);
- if (!(fullcmd = HeapAlloc(GetProcessHeap(), 0, len * sizeof(wchar_t)))) - { + memset(&si, 0, sizeof(si)); + si.cb = sizeof(si); + si.dwFlags = STARTF_USESTDHANDLES; + if (read_pipe) + { + si.hStdInput = (HANDLE)_get_osfhandle(STDIN_FILENO); + si.hStdOutput = (HANDLE)_get_osfhandle(fd_child); + } + else + { + si.hStdInput = (HANDLE)_get_osfhandle(fd_child); + si.hStdOutput = (HANDLE)_get_osfhandle(STDOUT_FILENO); + } + si.hStdError = (HANDLE)_get_osfhandle(STDERR_FILENO); + r = CreateProcessW(fullname, fullcmd, NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi); HeapFree(GetProcessHeap(), 0, comspec); - goto error; - } - - wcscpy(fullcmd, comspec); - wcscat(fullcmd, L" /c "); - wcscat(fullcmd, command); - - if ((container->proc = (HANDLE)msvcrt_spawn(_P_NOWAIT, comspec, fullcmd, NULL, 1)) - == INVALID_HANDLE_VALUE) - { - _close(fds[fdToOpen]); - ret = NULL; - } - else - { - ret = _wfdopen(fds[fdToOpen], mode); - if (!ret) - _close(fds[fdToOpen]); + HeapFree(GetProcessHeap(), 0, fullcmd); + if (!r) + { + msvcrt_set_errno(GetLastError()); + goto error; + } + CloseHandle(pi.hThread); + _close(fd_child); + container->proc = pi.hProcess; container->f = ret; - } - _unlock(_POPEN_LOCK); - HeapFree(GetProcessHeap(), 0, comspec); - HeapFree(GetProcessHeap(), 0, fullcmd); - _dup2(fdStdHandle, fdToDup); - _close(fdStdHandle); - return ret; + _unlock(_POPEN_LOCK); + return ret;
error: - _unlock(_POPEN_LOCK); - if (fdStdHandle != -1) _close(fdStdHandle); - _close(fds[0]); - _close(fds[1]); - return NULL; + _unlock(_POPEN_LOCK); + _close(fd_child); + fclose(ret); + return NULL; }
/********************************************************************* diff --git a/dlls/msvcrt/tests/misc.c b/dlls/msvcrt/tests/misc.c index 81c23dd23f2..06555e209b7 100644 --- a/dlls/msvcrt/tests/misc.c +++ b/dlls/msvcrt/tests/misc.c @@ -20,6 +20,8 @@
#include "wine/test.h" #include <errno.h> +#include <fcntl.h> +#include <io.h> #include <stdio.h> #include <math.h> #include <process.h> @@ -330,21 +332,42 @@ static void test__set_errno(void) ok(errno == 0xdeadbeef, "Expected errno to be 0xdeadbeef, got %d\n", errno); }
-static void test__popen_child(void) +static void test__popen_child(int fd) { /* don't execute any tests here */ /* ExitProcess is used to set return code of _pclose */ printf("child output\n"); + if ((HANDLE)_get_osfhandle(fd) != INVALID_HANDLE_VALUE) + ExitProcess(1); ExitProcess(0x37); }
+static void test__popen_read_child(void) +{ + char buf[1024], *rets; + + rets = fgets(buf, sizeof(buf), stdin); + if (strcmp(buf, "child-to-parent\n") != 0) + ExitProcess(1); + + rets = fgets(buf, sizeof(buf), stdin); + if (rets) + ExitProcess(2); + ExitProcess(3); +} + static void test__popen(const char *name) { FILE *pipe; - char buf[1024]; - int ret; + char *tempf, buf[1024]; + int ret, fd;
- sprintf(buf, ""%s" misc popen", name); + tempf = _tempnam(".", "wne"); + ok(tempf != NULL, "_tempnam failed\n"); + fd = _open(tempf, _O_CREAT | _O_WRONLY); + ok(fd != -1, "open failed\n"); + + sprintf(buf, ""%s" misc popen %d", name, fd); pipe = _popen(buf, "r"); ok(pipe != NULL, "_popen failed with error: %d\n", errno);
@@ -353,12 +376,25 @@ static void test__popen(const char *name)
ret = _pclose(pipe); ok(ret == 0x37, "_pclose returned %x, expected 0x37\n", ret); + _close(fd); + _unlink(tempf); + free(tempf);
errno = 0xdeadbeef; ret = _pclose((FILE*)0xdeadbeef); ok(ret == -1, "_pclose returned %x, expected -1\n", ret); if(p_set_errno) ok(errno == EBADF, "errno = %d\n", errno); + + sprintf(buf, ""%s" misc popen_read", name); + pipe = _popen(buf, "w"); + ok(pipe != NULL, "_popen failed with error: %d\n", errno); + + ret = fputs("child-to-parent\n", pipe); + ok(ret != EOF, "fputs returned %x\n", ret); + + ret = _pclose(pipe); + ok(ret == 0x3, "_pclose returned %x, expected 0x3\n", ret); }
static void test__invalid_parameter(void) @@ -711,8 +747,10 @@ START_TEST(misc)
arg_c = winetest_get_mainargs(&arg_v); if(arg_c >= 3) { - if(!strcmp(arg_v[2], "popen")) - test__popen_child(); + if (!strcmp(arg_v[2], "popen_read")) + test__popen_read_child(); + else if(arg_c == 4 && !strcmp(arg_v[2], "popen")) + test__popen_child(atoi(arg_v[3])); else ok(0, "invalid argument '%s'\n", arg_v[2]);