[PATCH v2 0/2] MR11710: msvcrt: Don't inherit std fd attributes.
-- v2: msvcrt: Don't update system std handles in msvcrt_init_io(). msvcrt: Don't inherit std fd attributes. https://gitlab.winehq.org/wine/wine/-/merge_requests/11710
From: Paul Gofman <pgofman@codeweavers.com> --- dlls/msvcrt/file.c | 83 +++++++++---------------- dlls/msvcrt/tests/file.c | 128 +++++++++++++++++++++++++++++---------- 2 files changed, 123 insertions(+), 88 deletions(-) diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c index 71972540c13..989755f3ec5 100644 --- a/dlls/msvcrt/file.c +++ b/dlls/msvcrt/file.c @@ -723,59 +723,39 @@ void msvcrt_init_io(void) STARTUPINFOA si; int i; ioinfo *fdinfo; + unsigned int count = 0; + HANDLE *handle_ptr = NULL; + BYTE *wxflag_ptr = NULL; GetStartupInfoA(&si); if (si.cbReserved2 >= sizeof(unsigned int) && si.lpReserved2 != NULL) { - BYTE* wxflag_ptr; - HANDLE* handle_ptr; - unsigned int count; - count = *(unsigned*)si.lpReserved2; wxflag_ptr = si.lpReserved2 + sizeof(unsigned); handle_ptr = (HANDLE*)(wxflag_ptr + count); - count = min(count, (si.cbReserved2 - sizeof(unsigned)) / (sizeof(HANDLE) + 1)); count = min(count, MSVCRT_MAX_FILES); - for (i = 0; i < count; i++) - { - if ((*wxflag_ptr & WX_OPEN) && GetFileType(*handle_ptr) != FILE_TYPE_UNKNOWN) - { - fdinfo = get_ioinfo_alloc_fd(i); - if(fdinfo != &MSVCRT___badioinfo) - msvcrt_set_fd(fdinfo, *handle_ptr, *wxflag_ptr); - release_ioinfo(fdinfo); - } - - wxflag_ptr++; handle_ptr++; - } } - fdinfo = get_ioinfo_alloc_fd(STDIN_FILENO); - if (!(fdinfo->wxflag & WX_OPEN) || fdinfo->handle == INVALID_HANDLE_VALUE) { - HANDLE h = GetStdHandle(STD_INPUT_HANDLE); + for (i = 0; i < 3; ++i) + { + static const DWORD std_handle[3] = { STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, STD_ERROR_HANDLE }; + HANDLE h = NULL; DWORD flags = WX_OPEN | WX_TEXT; - DWORD type = GetFileType(h); + DWORD type = FILE_TYPE_UNKNOWN; - if (type == FILE_TYPE_UNKNOWN) { - h = MSVCRT_NO_CONSOLE; - flags |= WX_TTY; - } else if ((type & 0xf) == FILE_TYPE_CHAR) { - flags |= WX_TTY; - } else if ((type & 0xf) == FILE_TYPE_PIPE) { - flags |= WX_PIPE; + if (count > i) + { + h = handle_ptr[i]; + type = GetFileType(h); + } + if (type == FILE_TYPE_UNKNOWN) + { + h = GetStdHandle(std_handle[i]); + type = GetFileType(h); } - msvcrt_set_fd(fdinfo, h, flags); - } - release_ioinfo(fdinfo); - - fdinfo = get_ioinfo_alloc_fd(STDOUT_FILENO); - if (!(fdinfo->wxflag & WX_OPEN) || fdinfo->handle == INVALID_HANDLE_VALUE) { - HANDLE h = GetStdHandle(STD_OUTPUT_HANDLE); - DWORD flags = WX_OPEN | WX_TEXT; - DWORD type = GetFileType(h); - + fdinfo = get_ioinfo_alloc_fd(i); if (type == FILE_TYPE_UNKNOWN) { h = MSVCRT_NO_CONSOLE; flags |= WX_TTY; @@ -784,29 +764,20 @@ void msvcrt_init_io(void) } else if ((type & 0xf) == FILE_TYPE_PIPE) { flags |= WX_PIPE; } - msvcrt_set_fd(fdinfo, h, flags); + release_ioinfo(fdinfo); } - release_ioinfo(fdinfo); - fdinfo = get_ioinfo_alloc_fd(STDERR_FILENO); - if (!(fdinfo->wxflag & WX_OPEN) || fdinfo->handle == INVALID_HANDLE_VALUE) { - HANDLE h = GetStdHandle(STD_ERROR_HANDLE); - DWORD flags = WX_OPEN | WX_TEXT; - DWORD type = GetFileType(h); - - if (type == FILE_TYPE_UNKNOWN) { - h = MSVCRT_NO_CONSOLE; - flags |= WX_TTY; - } else if ((type & 0xf) == FILE_TYPE_CHAR) { - flags |= WX_TTY; - } else if ((type & 0xf) == FILE_TYPE_PIPE) { - flags |= WX_PIPE; + for (i = 3; i < count; i++) + { + if ((wxflag_ptr[i] & WX_OPEN) && GetFileType(handle_ptr[i]) != FILE_TYPE_UNKNOWN) + { + fdinfo = get_ioinfo_alloc_fd(i); + if (fdinfo != &MSVCRT___badioinfo) + msvcrt_set_fd(fdinfo, handle_ptr[i], wxflag_ptr[i]); + release_ioinfo(fdinfo); } - - msvcrt_set_fd(fdinfo, h, flags); } - release_ioinfo(fdinfo); TRACE(":handles (%p)(%p)(%p)\n", get_ioinfo_nolock(STDIN_FILENO)->handle, get_ioinfo_nolock(STDOUT_FILENO)->handle, diff --git a/dlls/msvcrt/tests/file.c b/dlls/msvcrt/tests/file.c index 4942780d425..0ffe82cf667 100644 --- a/dlls/msvcrt/tests/file.c +++ b/dlls/msvcrt/tests/file.c @@ -1633,31 +1633,43 @@ static void test_file_write_read( void ) free(tempf); } -static void test_file_inherit_child(const char* fd_s, const char *handle_str) +static void test_file_inherit_child(const char* fd_s, const char *mode_str, const char *handle_str) { - HANDLE handle_value; + HANDLE handle_value, handle; int fd = atoi(fd_s); HANDLE *handle_ptr; unsigned int count; char buffer[32]; STARTUPINFOA si; - int ret; + int expected_mode = atoi(mode_str); + int ret, mode, pos; GetStartupInfoA(&si); count = *(unsigned *)si.lpReserved2; if (handle_str) { - ok(count == 3, "Got unexpected count %u.\n", count); + ok(count >= 3, "Got unexpected count %u.\n", count); sscanf(handle_str, "%p", &handle_value); handle_ptr = (HANDLE *)(si.lpReserved2 + sizeof(unsigned) + count); - ok(handle_value == handle_ptr[1], "Got unexpected handle %p.\n", handle_ptr[1]); + ok(handle_value == handle_ptr[fd], "Got unexpected handle %p.\n", handle_ptr[1]); + if (handle_value && handle_value != (HANDLE)0xdeadbeef && handle_value != INVALID_HANDLE_VALUE) + { + handle = (HANDLE)_get_osfhandle(fd); + ok(handle == handle_value, "got %p, %p.\n", handle, handle_value); + } } + mode = _setmode(fd, _O_BINARY); + ok(mode == expected_mode, "got %04x, fd %d, expected %04x.\n", mode, fd, expected_mode); + pos = lseek(fd, 0, SEEK_CUR); ret = write(fd, "Success", 8); ok( ret == 8, "Couldn't write in child process on %d (%s)\n", fd, strerror(errno)); - lseek(fd, 0, SEEK_SET); - ok(read(fd, buffer, sizeof (buffer)) == 8, "Couldn't read back the data\n"); - ok(memcmp(buffer, "Success", 8) == 0, "Couldn't read back the data\n"); + lseek(fd, pos, SEEK_SET); + ret = read(fd, buffer, sizeof (buffer)); + if (ret == -1 && errno == EBADF) + return; + ok(ret == 8, "Couldn't read back the data, got %d, errno %d\n", ret, errno); + ok(memcmp(buffer, "Success", 8) == 0, "Read back data mismatch\n"); } static void test_file_inherit_child_no(const char* fd_s) @@ -1685,7 +1697,7 @@ static void create_io_inherit_block( STARTUPINFOA *startup, unsigned int count, *(unsigned*)block = count; for (i = 0; i < count; i++) { - wxflag_ptr[i] = 0x81; + wxflag_ptr[i] = 0x1; handle_ptr[i] = handles[i]; } } @@ -1701,7 +1713,7 @@ static const char *read_file( HANDLE file ) } static void test_stdout_handle( STARTUPINFOA *startup, char *cmdline, HANDLE hstdout, BOOL expect_stdout, - const char *descr ) + const char *descr, BOOL set_invalid_stdout ) { const char *data; HANDLE hErrorFile; @@ -1717,7 +1729,7 @@ static void test_stdout_handle( STARTUPINFOA *startup, char *cmdline, HANDLE hst FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); startup->dwFlags = STARTF_USESTDHANDLES; startup->hStdInput = GetStdHandle( STD_INPUT_HANDLE ); - startup->hStdOutput = hErrorFile; + startup->hStdOutput = set_invalid_stdout ? INVALID_HANDLE_VALUE : hErrorFile; startup->hStdError = GetStdHandle( STD_ERROR_HANDLE ); CreateProcessA( NULL, cmdline, NULL, NULL, TRUE, @@ -1734,7 +1746,7 @@ static void test_stdout_handle( STARTUPINFOA *startup, char *cmdline, HANDLE hst { data = read_file( hstdout ); if (expect_stdout) - ok( !strcmp( data, "Success" ), "%s: Wrong stdout data (%s)\n", descr, data ); + ok( !strcmp( data, "Success" ), "%s: Wrong stdout data (%s)\n", descr, debugstr_a(data) ); else ok( strcmp( data, "Success" ), "%s: Stdout file shouldn't contain data\n", descr ); } @@ -1758,15 +1770,16 @@ static unsigned WINAPI read_pipe_thread(void *argument) static void test_file_inherit( const char* selfname ) { int fd; - const char* arg_v[5]; - char buffer[16]; + const char* arg_v[6]; + char buffer[16], buffer2[16]; char cmdline[MAX_PATH]; STARTUPINFOA startup; SECURITY_ATTRIBUTES sa; - HANDLE handles[3]; + HANDLE handles[4]; HANDLE thread_handle; int pipefds[2]; intptr_t ret; + int mode; fd = open ("fdopen.tst", O_CREAT | O_RDWR | O_BINARY, _S_IREAD |_S_IWRITE); ok(fd != -1, "Couldn't create test file\n"); @@ -1774,7 +1787,8 @@ static void test_file_inherit( const char* selfname ) arg_v[1] = "file"; arg_v[2] = "inherit"; arg_v[3] = buffer; sprintf(buffer, "%d", fd); - arg_v[4] = 0; + arg_v[4] = buffer2; sprintf(buffer2, "%d", O_BINARY); + arg_v[5] = 0; ret = _spawnvp(_P_WAIT, selfname, arg_v); ok(ret == 0, "_spawnvp returned %Id, errno %d\n", ret, errno); ok(tell(fd) == 8, "bad position %lu expecting 8\n", tell(fd)); @@ -1782,13 +1796,28 @@ static void test_file_inherit( const char* selfname ) ok(read(fd, buffer, sizeof (buffer)) == 8 && memcmp(buffer, "Success", 8) == 0, "Couldn't read back the data\n"); close (fd); ok(unlink("fdopen.tst") == 0, "Couldn't unlink\n"); + + mode = _setmode(1, O_BINARY); + ok(mode != -1, "got %#x.\n", mode); + arg_v[0] = get_base_name(selfname); + arg_v[1] = "file"; + arg_v[2] = "inherit"; + arg_v[3] = buffer; sprintf(buffer, "%d", 1); + /* For std handle a mode on it is not going to be inherited and set to default text mode instead. */ + arg_v[4] = buffer2; sprintf(buffer2, "%d", O_TEXT); + arg_v[5] = 0; + ret = _spawnvp(_P_WAIT, selfname, arg_v); + mode = _setmode(1, mode); + ok(ret == 0, "_spawnvp returned %Id, errno %d\n", ret, errno); + ok(mode == O_BINARY, "got %#x.\n", mode); fd = open ("fdopen.tst", O_CREAT | O_RDWR | O_BINARY | O_NOINHERIT, _S_IREAD |_S_IWRITE); ok(fd != -1, "Couldn't create test file\n"); arg_v[1] = "file"; arg_v[2] = "inherit_no"; arg_v[3] = buffer; sprintf(buffer, "%d", fd); - arg_v[4] = 0; + arg_v[4] = buffer2; sprintf(buffer2, "%d", O_BINARY); + arg_v[5] = 0; ret = _spawnvp(_P_WAIT, selfname, arg_v); ok(ret == 0, "_spawnvp returned %Id, errno %d\n", ret, errno); ok(tell(fd) == 0, "bad position %lu expecting 0\n", tell(fd)); @@ -1805,7 +1834,8 @@ static void test_file_inherit( const char* selfname ) arg_v[1] = "tests/file.c"; arg_v[2] = "inherit"; arg_v[3] = buffer; sprintf(buffer, "%d", fd); - arg_v[4] = 0; + arg_v[4] = buffer2; sprintf(buffer2, "%d", O_BINARY); + arg_v[5] = 0; ret = _spawnvp(_P_WAIT, selfname, arg_v); ok(ret == 0, "_spawnvp returned %Id, errno %d\n", ret, errno); ret = tell(fd); @@ -1824,13 +1854,13 @@ static void test_file_inherit( const char* selfname ) sa.nLength = sizeof(sa); sa.lpSecurityDescriptor = NULL; sa.bInheritHandle = TRUE; - sprintf(cmdline, "%s file inherit 1", selfname); + sprintf(cmdline, "%s file inherit 1 %d", selfname, O_TEXT); /* init an empty Reserved2, which should not be recognized as inherit-block */ ZeroMemory(&startup, sizeof(startup)); startup.cb = sizeof(startup); create_io_inherit_block( &startup, 0, NULL ); - test_stdout_handle( &startup, cmdline, 0, FALSE, "empty block" ); + test_stdout_handle( &startup, cmdline, 0, FALSE, "empty block", FALSE ); /* test with valid inheritblock */ handles[0] = GetStdHandle( STD_INPUT_HANDLE ); @@ -1838,7 +1868,7 @@ static void test_file_inherit( const char* selfname ) FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); handles[2] = GetStdHandle( STD_ERROR_HANDLE ); create_io_inherit_block( &startup, 3, handles ); - test_stdout_handle( &startup, cmdline, handles[1], TRUE, "valid block" ); + test_stdout_handle( &startup, cmdline, handles[1], TRUE, "valid block", FALSE ); CloseHandle( handles[1] ); DeleteFileA("fdopen.tst"); @@ -1847,7 +1877,7 @@ static void test_file_inherit( const char* selfname ) FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); create_io_inherit_block( &startup, 3, handles ); *(unsigned int *)startup.lpReserved2 = 0; - test_stdout_handle( &startup, cmdline, handles[1], FALSE, "zero count block" ); + test_stdout_handle( &startup, cmdline, handles[1], FALSE, "zero count block", FALSE ); CloseHandle( handles[1] ); DeleteFileA("fdopen.tst"); @@ -1856,7 +1886,7 @@ static void test_file_inherit( const char* selfname ) FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); create_io_inherit_block( &startup, 3, handles ); startup.cbReserved2 -= 3; - test_stdout_handle( &startup, cmdline, handles[1], TRUE, "small size block" ); + test_stdout_handle( &startup, cmdline, handles[1], TRUE, "small size block", FALSE ); CloseHandle( handles[1] ); DeleteFileA("fdopen.tst"); @@ -1865,7 +1895,7 @@ static void test_file_inherit( const char* selfname ) FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); create_io_inherit_block( &startup, 3, handles ); startup.cbReserved2 = sizeof(unsigned int) + sizeof(HANDLE) + sizeof(char); - test_stdout_handle( &startup, cmdline, handles[1], FALSE, "smaller size block" ); + test_stdout_handle( &startup, cmdline, handles[1], FALSE, "smaller size block", FALSE ); CloseHandle( handles[1] ); DeleteFileA("fdopen.tst"); @@ -1874,25 +1904,59 @@ static void test_file_inherit( const char* selfname ) FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); create_io_inherit_block( &startup, 3, handles ); startup.cbReserved2 += 7; - test_stdout_handle( &startup, cmdline, handles[1], TRUE, "large size block" ); + test_stdout_handle( &startup, cmdline, handles[1], TRUE, "large size block", FALSE ); CloseHandle( handles[1] ); DeleteFileA("fdopen.tst"); /* test inherit block with invalid handle */ handles[1] = INVALID_HANDLE_VALUE; create_io_inherit_block( &startup, 3, handles ); - sprintf(cmdline, "%s file inherit 1 %p", selfname, handles[1]); - test_stdout_handle( &startup, cmdline, NULL, FALSE, "INVALID_HANDLE_VALUE stdout handle" ); + sprintf(cmdline, "%s file inherit 1 %d %p", selfname, O_TEXT, handles[1]); + test_stdout_handle( &startup, cmdline, NULL, FALSE, "INVALID_HANDLE_VALUE stdout handle", FALSE ); handles[1] = NULL; create_io_inherit_block( &startup, 3, handles ); - sprintf(cmdline, "%s file inherit 1 %p", selfname, handles[1]); - test_stdout_handle( &startup, cmdline, NULL, FALSE, "NULL stdout handle" ); + sprintf(cmdline, "%s file inherit 1 %d %p", selfname, O_TEXT, handles[1]); + test_stdout_handle( &startup, cmdline, NULL, FALSE, "NULL stdout handle", FALSE ); handles[1] = (void *)0xdeadbeef; create_io_inherit_block( &startup, 3, handles ); - sprintf(cmdline, "%s file inherit 1 %p", selfname, handles[1]); - test_stdout_handle( &startup, cmdline, NULL, FALSE, "invalid stdout handle" ); + sprintf(cmdline, "%s file inherit 1 %d %p", selfname, O_TEXT, handles[1]); + test_stdout_handle( &startup, cmdline, NULL, FALSE, "invalid stdout handle", FALSE ); + + /* std handle doesn't respect open mode from inherited block, while non-std handle does. */ + handles[1] = CreateFileA( "fdopen.tst", GENERIC_READ|GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); + create_io_inherit_block( &startup, 3, handles ); + sprintf(cmdline, "%s file inherit 1 %d %p", selfname, O_TEXT, handles[1]); + winetest_push_context("valid stdout handle"); + test_stdout_handle( &startup, cmdline, handles[1], TRUE, "valid stdout handle", FALSE ); + winetest_pop_context(); + CloseHandle( handles[1] ); + DeleteFileA("fdopen.tst"); + + handles[1] = CreateFileA( "fdopen.tst", GENERIC_READ|GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); + create_io_inherit_block( &startup, 3, handles ); + sprintf(cmdline, "%s file inherit 1 %d %p", selfname, O_TEXT, handles[1]); + winetest_push_context("valid stdout handle"); + test_stdout_handle( &startup, cmdline, handles[1], TRUE, "valid CRT, invalid kernelbase", TRUE ); + winetest_pop_context(); + CloseHandle( handles[1] ); + DeleteFileA("fdopen.tst"); + + handles[3] = CreateFileA( "fdopen2.tst", GENERIC_READ|GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); + handles[1] = CreateFileA( "fdopen.tst", GENERIC_READ|GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, CREATE_ALWAYS, 0, NULL ); + create_io_inherit_block( &startup, 4, handles ); + sprintf(cmdline, "%s file inherit 3 %d %p", selfname, O_BINARY, handles[3]); + winetest_push_context("valid stdout handle"); + test_stdout_handle( &startup, cmdline, handles[3], TRUE, "non-std handle", FALSE ); + winetest_pop_context(); + CloseHandle( handles[1] ); + DeleteFileA("fdopen.tst"); + DeleteFileA("fdopen2.tst"); } static void test_invalid_stdin_child( void ) @@ -3183,7 +3247,7 @@ START_TEST(file) if (arg_c >= 3) { if (strcmp(arg_v[2], "inherit") == 0) - test_file_inherit_child(arg_v[3], arg_c > 4 ? arg_v[4] : NULL); + test_file_inherit_child(arg_v[3], arg_v[4], arg_c > 5 ? arg_v[5] : NULL); else if (strcmp(arg_v[2], "inherit_no") == 0) test_file_inherit_child_no(arg_v[3]); else if (strcmp(arg_v[2], "pipes") == 0) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11710
From: Paul Gofman <pgofman@codeweavers.com> --- dlls/msvcrt/file.c | 11 +++++++---- dlls/msvcrt/tests/file.c | 5 +++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c index 989755f3ec5..2fd300f0e7d 100644 --- a/dlls/msvcrt/file.c +++ b/dlls/msvcrt/file.c @@ -574,7 +574,7 @@ static void msvcrt_free_fd(int fd) release_ioinfo(fdinfo); } -static void msvcrt_set_fd(ioinfo *fdinfo, HANDLE hand, int flag) +static void msvcrt_set_fd_no_std_update(ioinfo *fdinfo, HANDLE hand, int flag) { fdinfo->handle = hand; fdinfo->wxflag = WX_OPEN | (flag & (WX_DONTINHERIT | WX_APPEND | WX_TEXT | WX_PIPE | WX_TTY)); @@ -583,7 +583,11 @@ static void msvcrt_set_fd(ioinfo *fdinfo, HANDLE hand, int flag) fdinfo->lookahead[2] = '\n'; ioinfo_set_unicode(fdinfo, FALSE); ioinfo_set_textmode(fdinfo, TEXTMODE_ANSI); +} +static void msvcrt_set_fd(ioinfo *fdinfo, HANDLE hand, int flag) +{ + msvcrt_set_fd_no_std_update(fdinfo, hand, flag); if (hand != MSVCRT_NO_CONSOLE) { switch (fdinfo-MSVCRT___pioinfo[0]) @@ -594,7 +598,6 @@ static void msvcrt_set_fd(ioinfo *fdinfo, HANDLE hand, int flag) } } } - /* INTERNAL: Allocate an fd slot from a Win32 HANDLE */ static int msvcrt_alloc_fd(HANDLE hand, int flag) { @@ -764,7 +767,7 @@ void msvcrt_init_io(void) } else if ((type & 0xf) == FILE_TYPE_PIPE) { flags |= WX_PIPE; } - msvcrt_set_fd(fdinfo, h, flags); + msvcrt_set_fd_no_std_update(fdinfo, h, flags); release_ioinfo(fdinfo); } @@ -774,7 +777,7 @@ void msvcrt_init_io(void) { fdinfo = get_ioinfo_alloc_fd(i); if (fdinfo != &MSVCRT___badioinfo) - msvcrt_set_fd(fdinfo, handle_ptr[i], wxflag_ptr[i]); + msvcrt_set_fd_no_std_update(fdinfo, handle_ptr[i], wxflag_ptr[i]); release_ioinfo(fdinfo); } } diff --git a/dlls/msvcrt/tests/file.c b/dlls/msvcrt/tests/file.c index 0ffe82cf667..aabd5fb49e6 100644 --- a/dlls/msvcrt/tests/file.c +++ b/dlls/msvcrt/tests/file.c @@ -1656,6 +1656,11 @@ static void test_file_inherit_child(const char* fd_s, const char *mode_str, cons { handle = (HANDLE)_get_osfhandle(fd); ok(handle == handle_value, "got %p, %p.\n", handle, handle_value); + if (fd == 1) + { + handle = GetStdHandle(STD_OUTPUT_HANDLE); + ok(handle != handle_value, "got equal handles %p.\n", handle); + } } } mode = _setmode(fd, _O_BINARY); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/11710
This helps Ragnarok: The New World launcher which otherwise complains about not finding Steam process (exaltedly titling that "Unhandled Promise Rejection"). This is javscript / asar app which uses libuv (https://github.com/libuv/libuv), in particular, for creating processes with uv_spawn(). To find Steam it executes "cmd /d /s /c tasklist" and expects the result in narrow chars, while we end up having that as wide chars. tasklist is using wsprintf which is normally converted to narrow chars in ucrtbase in default stdout stream fd state (which is O_TEXT implying such a conversion). However, when uv_spawn() creates the process with app requested 'UV_CREATE_PIPE' type of stdout, it manually fills lpReserved2 in STARTUPINFO setting stdout flags to 0x9 (WX_OPEN | WX_PIPE). Currently that results in the stdout being treated as binary and printf conversion to narrow chars does not happen. My tests show that native CRT ignores those flags for std handle (while it might still use the handle value to set for std fd, while that won't result in updating kernelbase's std handles which my second patch is about). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149329
side note: * the lpReserved2 settings that tasklist gets as the one generated by cmd.exe, not by the app * currently cmd.exe only forces the open bit, but keep other flag values from startup that doesn't look right, eg when handle has been changed I even wonder if we still need the lpReserved2 setup, as we only pass the 3 std handles to child process anyway is that something you've investigated? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149345
No, I believe my description is correct. The game starts cmd.exe with CreateProcessW() with STARTUPINFO.cbReserved / STARTUPINFO.lpReserved set as described, I dumped that fields at CreateProcess, I saw return address in CreateProcessW and the code doing that and found the source code for the library used which I linked. I know cmd.exe does it too but indeed it only copies the related attribute, stdout text mode is changed at cmd.exe start and not at tasklist.exe start by cmd. Another thing now seems suspicious to me with the second patch: I realized msvcrt.spawn does not set std handles in STARTUPINFO, and with that change in the second patch now nothing on the spawn path is going to set the system's std handles in TEB which doesn't look right at once. What if, e. g., that should be set in spawn, or there are more conditions when std handles are updated at startup. I am going to test this part a bit more (and if that ends up being too convoluted maybe even just drop that second patch for now, that is not needed for the case I am fixing, only the first one). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149394
No, I believe my description is correct.
I didn't imply otherwise. I was just pointing at another potential issue. as the current cmd code sets the Std handles with the potentially redirected streams, it doesn't look right that we reuse the O_TEXT bit that could have been set by cmd's father. I'll dig a bit more into it.
Another thing now seems suspicious to me with the second patch: I realized msvcrt.spawn does not set std handles in STARTUPINFO, and with that change in the second patch now nothing on the spawn path is going to set the system's std handles
the way msvcrt calls CreateProcess (no console flags, inherit handles = true) will automatically inherit parent's std handles (except if child process is in GUI subsystem, and it's a console handle); so they will be set to parent's std handles so if no one temper the std handles directly, the fd=0,1,2 should be in sync with the std values (both in parent and child) on the other hand, if someone changes std handles without updating fd 0,1,2 entries' osf handles in msvcrt, I don't know what would happen (and createprocess tests show there are a variety of subcases depending on CreateProcess flags, nature of handles, CUI/GUI child...) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149406
I didn't imply otherwise. I was just pointing at another potential issue. as the current cmd code sets the Std handles with the potentially redirected streams, it doesn't look right that we reuse the O_TEXT bit that could have been set by cmd's father. I'll dig a bit more into it.
As far as my tests and patch here goes, what cmd.exe currently does passing the CRT std fds flags to lpReserved2 is going to be a no-op anyway. In fact, I so far don't see any reasonable way at all how a process can force the fd flags for child's std fds. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149407
On Thu Aug 20 14:45:57 2026 +0000, Paul Gofman wrote:
I didn't imply otherwise. I was just pointing at another potential issue. as the current cmd code sets the Std handles with the potentially redirected streams, it doesn't look right that we reuse the O_TEXT bit that could have been set by cmd's father. I'll dig a bit more into it. As far as my tests and patch here goes, what cmd.exe currently does passing the CRT std fds flags to lpReserved2 is going to be a no-op anyway. In fact, I so far don't see any reasonable way at all how a process can force the fd flags for child's std fds. And cmd's own std fds are going to have default O_TEXT, regardless of what was in the caller. I thought about that a bit when making this patch and didn't immediately see why it would introduce the issue for cmd if ucrtbase behaves like that on Windows (unless I am still missing some condition under which the parent's std stream text attribute can actually be inherited).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149408
As far as my tests and patch here goes, what cmd.exe currently does passing the CRT std fds flags to lpReserved2 is going to be a no-op anyway. In fact, I so far don't see any reasonable way at all how a process can force the fd flags for child's std fds.
that's my question too (or IOW, falling back to just inheriting the std handle and let msvcrt initialize std fd from them). I just want to check the /U cmd's option to be fair I don't remember why I added this 20 years ago (and change log doesn't say much why it's needed) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149409
so if no one temper the std handles directly, the fd=0,1,2 should be in sync with the std values (both in parent and child)
Yes, I tested that additionally and it just gets inherited. I also was the one to temper std handles directly in this testing after _dup2(fd, 1) before spawn() and it is the same on WIndows and Wine with my patches: the file set in SetStdHandle() is inherited to GetStdHandle(), the CRT stdout handle is different and what was set through fd = open() / _dupd2(fd, 1). So looks like my second patch is correct without further changes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149412
Hi Paul, I'm having a hard time reviewing the patch (I'm referring to the first commit, I didn't look on the second one yet). While I like some of the cleanups you're doing you're also introducing unrelated changes. E.g. in std handles initialization loop you're no longer checking `wxflag_ptr[i] & WX_OPEN` (which is probably wrong and not covered by the tests). It would be best to move the cleanup to separate commit. It looks like WX_TEXT is treated differently than other flags. I have tested that WX_TTY is preserved in fd attributes (by setting 0x41 flag in `create_io_inherit_block`, passing file handle and calling `isatty()`). This is something that used to work and breaks with your patch. The tests you're adding will pass if WX_TEXT flag is set for std handles. Something like: ```c msvcrt_set_fd(fdinfo, *handle_ptr, *wxflag_ptr | (i < 3 ? WX_TEXT : 0)); ``` will do the job. While it would be good to test other attributes I think it's closer to what native does. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149438
Thanks. Yes touching WX_OPEN check was not intended and not tested, sorry. I will check my tests with other flags like TTY and adjust accoringly, maybe it is just that WX_TEXT which gets forcefully added indeed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/11710#note_149441
participants (4)
-
eric pouech (＠epo) -
Paul Gofman
-
Paul Gofman (＠gofman)
-
Piotr Caban (＠piotr)