Piotr Caban : msvcrt: Rewrite _popen function.
Alexandre Julliard
julliard at winehq.org
Mon Oct 25 16:30:07 CDT 2021
Module: wine
Branch: master
Commit: e38f3761f890ccc0b32ac16c633cdf7582511133
URL: https://source.winehq.org/git/wine.git/?a=commit;h=e38f3761f890ccc0b32ac16c633cdf7582511133
Author: Piotr Caban <piotr at 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 at codeweavers.com>
Signed-off-by: Alexandre Julliard <julliard at 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]);
More information about the wine-cvs
mailing list