[PATCH v2 0/5] MR7608: server: Avoid delegating console waits to other objects.
Some console objects currently do several unique things: * Delegate waits onto the queue of another object. This is not really a problem for in-process waits, since we can just return the sync object for the delegate. However, it's also unnecessary, adds to the complexity of the server logic, and is one out of one places where this is done. * Make the wait state dependent on the process. This is difficult to emulate with ntsync and would require creating separate server objects for each process, hacking into duplicate_handle. * Fail a wait entirely in certain circumstances. This is pretty much impossible to emulate with in-process waits. Although ntsync has been in development for some time, I have regrettably failed to notice these problems until now. Fortunately, none of these behaviours happen on modern Windows. Although I/O on unbound handles delegates to the console of the current process, the signaled state does not. As the tests here show, the signaled state of a handle depends on the active console of the process in which the handle was created. If that console no longer exists, the signaled state is no longer updated [with one rather inexplicable exception]. Crucially, in current Windows waits never fail, and the state of an object is the same across all process which hold handles to it. Therefore this patch brings our behaviour to closer match current Windows. In theory these are fds and should use default_fd_signaled(). However, the points at which the handles are signaled are completely different, and I/O does not trigger console handles to become signaled when it normally would. Therefore for the time being I've kept the code using custom signaled ops. There is one other oddity related to consoles, which is the existence of console_add_queue(), which seeks to lazily create an input thread when a console is first waited on. This is one out of two places, after this patch, when the wait process is hijacked (the other being message queues). Fortunately this is easy to handle for in-process synchronization objects, by queueing the ioctl from the callback used to retrieve the in-process synchronization object itself. -- v2: server: Track unbound output signaled state based on its original console. server: Track unbound input signaled state based on its original console. server: Allow waiting on an orphaned screen buffer. server: Fail to create an unbound input/output when there is no console. kernel32/tests: Add more tests for orphaned console handles. https://gitlab.winehq.org/wine/wine/-/merge_requests/7608
From: Elizabeth Figura <zfigura(a)codeweavers.com> --- dlls/kernel32/tests/console.c | 244 +++++++++++++++++++++++++++++++++- 1 file changed, 240 insertions(+), 4 deletions(-) diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c index 03491b279af..dd879725658 100644 --- a/dlls/kernel32/tests/console.c +++ b/dlls/kernel32/tests/console.c @@ -4124,12 +4124,132 @@ static void test_GetConsoleScreenBufferInfoEx(HANDLE std_output) ok(GetLastError() == 0xdeadbeef, "got %lu, expected 0xdeadbeef\n", GetLastError()); } -static void test_FreeConsole(void) +/* Flush events and check that there was a double-click event queued. + * Windows may also queue focus change or mouse move events. */ +static void check_mouse_event(HANDLE handle) { - HANDLE handle, unbound_output = NULL, unbound_input = NULL; + BOOL ret, got_mouse_event = FALSE; + INPUT_RECORD ir; + DWORD count; + + while (PeekConsoleInputW(handle, &ir, 1, &count) && count) + { + ret = ReadConsoleInputW(handle, &ir, 1, &count); + ok(ret == TRUE, "got error %lu\n", GetLastError()); + ok(count == 1, "got count %lu\n", count); + if (ir.EventType == MOUSE_EVENT && ir.Event.MouseEvent.dwEventFlags == DOUBLE_CLICK) + got_mouse_event = TRUE; + } + ok(got_mouse_event, "didn't find mouse event\n"); +} + +static void test_unbound_handles_child(DWORD parent_pid, UINT_PTR parent_input_int, UINT_PTR parent_output_int) +{ + HANDLE parent_process, parent_input, parent_output, unbound_input, input, parent_event, child_event; + OBJECT_ATTRIBUTES attr = {sizeof(attr)}; + IO_STATUS_BLOCK iosb; + UNICODE_STRING name; + NTSTATUS status; + INPUT_RECORD ir; + DWORD count; + BOOL ret; + + parent_event = OpenEventA(EVENT_ALL_ACCESS, FALSE, "winetest_unbound_handles_parent"); + child_event = OpenEventA(EVENT_ALL_ACCESS, FALSE, "winetest_unbound_handles_child"); + + parent_process = OpenProcess(PROCESS_DUP_HANDLE, FALSE, parent_pid); + ok(!!parent_process, "got error %lu\n", GetLastError()); + + ret = DuplicateHandle(parent_process, (HANDLE)parent_input_int, + GetCurrentProcess(), &parent_input, 0, FALSE, DUPLICATE_SAME_ACCESS); + ok(ret, "got error %lu\n", GetLastError()); + ret = DuplicateHandle(parent_process, (HANDLE)parent_output_int, + GetCurrentProcess(), &parent_output, 0, FALSE, DUPLICATE_SAME_ACCESS); + ok(ret, "got error %lu\n", GetLastError()); + + ret = WaitForSingleObject(parent_input, 0); + todo_wine ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(parent_output, 0); + todo_wine ok(!ret, "got %d\n", ret); + + ret = PeekConsoleInputW(parent_input, &ir, 1, &count); + ok(!ret, "got %d\n", ret); + ok(GetLastError() == ERROR_INVALID_HANDLE, "got error %lu\n", GetLastError()); + + /* We cannot create an unbound handle, because we have no console. */ + + attr.ObjectName = &name; + attr.Attributes = OBJ_INHERIT; + RtlInitUnicodeString( &name, L"\\Device\\ConDrv\\Input" ); + status = NtCreateFile( &unbound_input, FILE_READ_DATA | FILE_WRITE_DATA | SYNCHRONIZE | FILE_READ_ATTRIBUTES | + FILE_WRITE_ATTRIBUTES, &attr, &iosb, NULL, FILE_ATTRIBUTE_NORMAL, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, FILE_CREATE, + FILE_NON_DIRECTORY_FILE | FILE_SYNCHRONOUS_IO_NONALERT, NULL, 0 ); + todo_wine ok(status == STATUS_INVALID_HANDLE, "got %#lx\n", status); + if (!status) NtClose( unbound_input ); + + /* Allocate a console. */ + + ret = AllocConsole(); + ok(ret == TRUE, "got error %lu\n", GetLastError()); + + unbound_input = create_unbound_handle(FALSE, TRUE); + + input = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING, 0, 0); + ok(input != INVALID_HANDLE_VALUE, "got error %lu\n", GetLastError()); + + memset(&ir, 0, sizeof(ir)); + ir.EventType = MOUSE_EVENT; + ir.Event.MouseEvent.dwEventFlags = DOUBLE_CLICK; + ret = WriteConsoleInputW(input, &ir, 1, &count); + ok(ret, "got error %lu\n", GetLastError()); + + ret = WaitForSingleObject(unbound_input, 0); + ok(!ret, "got %d\n", ret); + + /* We can read from our input using the parent's handle, but its signaled + * state will not reflect our input. */ + + check_mouse_event(parent_input); + + ret = WaitForSingleObject(unbound_input, 0); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ret = WaitForSingleObject(parent_input, 0); + todo_wine ok(!ret, "got %d\n", ret); + + CloseHandle(input); + ret = FreeConsole(); + ok(ret == TRUE, "got error %lu\n", GetLastError()); + + ret = WaitForSingleObject(unbound_input, 0); + todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + CloseHandle(unbound_input); + + ret = WaitForSingleObject(parent_input, 0); + todo_wine ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(parent_output, 0); + todo_wine ok(!ret, "got %d\n", ret); + + /* Parent will consume its input, designalling those handles. */ + SetEvent(child_event); + ret = WaitForSingleObject(parent_event, 1000); + ok(!ret, "got %d\n", ret); + + ret = WaitForSingleObject(parent_input, 0); + todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ret = WaitForSingleObject(parent_output, 0); + todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); +} + +static void test_FreeConsole(HANDLE input, HANDLE orig_output) +{ + static const INPUT_RECORD ir = {.EventType = MOUSE_EVENT, .Event.MouseEvent.dwEventFlags = DOUBLE_CLICK}; + HANDLE handle, output, parent_event, child_event, unbound_output = NULL, unbound_input = NULL, unbound_input2; + STARTUPINFOA si = {sizeof(si)}; + char buf[MAX_PATH], **argv; + PROCESS_INFORMATION info; DWORD size, mode, type; WCHAR title[16]; - char buf[32]; HWND hwnd; UINT cp; BOOL ret; @@ -4144,6 +4264,53 @@ static void test_FreeConsole(void) unbound_output = create_unbound_handle(TRUE, TRUE); } + output = CreateConsoleScreenBuffer(GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CONSOLE_TEXTMODE_BUFFER, NULL); + ok(output != INVALID_HANDLE_VALUE, "got error %lu\n", GetLastError()); + + ret = WriteConsoleInputW(input, &ir, 1, &size); + ok(ret, "got error %lu\n", GetLastError()); + + ret = WaitForSingleObject(output, 0); + ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(unbound_output, 0); + ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(unbound_input, 0); + ok(!ret, "got %d\n", ret); + + parent_event = CreateEventA(NULL, FALSE, FALSE, "winetest_unbound_handles_parent"); + child_event = CreateEventA(NULL, FALSE, FALSE, "winetest_unbound_handles_child"); + + winetest_get_mainargs(&argv); + sprintf(buf, "\"%s\" console unbound_handles %#lx %#Ix %#Ix", + argv[0], GetCurrentProcessId(), (UINT_PTR)unbound_input, (UINT_PTR)unbound_output); + ret = CreateProcessA(NULL, buf, NULL, NULL, FALSE, DETACHED_PROCESS, NULL, NULL, &si, &info); + ok(ret, "got error %lu\n", GetLastError()); + + /* Wait for child to test our handles while there's input available. */ + ret = WaitForSingleObject(child_event, 1000); + todo_wine ok(!ret, "got %d\n", ret); + + /* Read the input. */ + check_mouse_event(unbound_input); + + ret = WaitForSingleObject(output, 0); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ret = WaitForSingleObject(unbound_output, 0); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ret = WaitForSingleObject(unbound_input, 0); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + + /* Test that our handles are now designaled in the child. */ + SetEvent(parent_event); + + CloseHandle(info.hThread); + wait_child_process(info.hProcess); + CloseHandle(info.hProcess); + + ret = WriteConsoleInputW(input, &ir, 1, &size); + ok(ret, "got error %lu\n", GetLastError()); + ret = FreeConsole(); ok(ret, "FreeConsole failed: %lu\n", GetLastError()); @@ -4170,6 +4337,68 @@ static void test_FreeConsole(void) (GetLastError() == ERROR_INVALID_HANDLE || broken(GetLastError() == ERROR_FILE_NOT_FOUND /* winxp */)), "CreateFileA failed: %lu\n", GetLastError()); + CloseHandle(input); + CloseHandle(orig_output); + + ret = WaitForSingleObject(output, 0); + todo_wine ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(unbound_output, 0); + todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ret = WaitForSingleObject(unbound_input, 0); + todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + + /* Create a new console. + * + * The previous unbound handles remain associated with the old console. + * As with the cross-process case, they can be read from, which reads from + * the current process's active console, but the signaled state does not + * reflect the current console. + * + * For some inexplicable reason, allocating the new console also causes the + * original orphaned handles to become signaled. */ + ret = AllocConsole(); + ok(ret, "got error %lu\n", GetLastError()); + + ret = WaitForSingleObject(output, 0); + todo_wine ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(unbound_output, 0); + todo_wine ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(unbound_input, 0); + todo_wine ok(!ret, "got %d\n", ret); + + input = CreateFileA("CONIN$", GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING, 0, 0); + ok(input != INVALID_HANDLE_VALUE, "got error %lu\n", GetLastError()); + ret = WriteConsoleInputW(input, &ir, 1, &size); + ok(ret, "got error %lu\n", GetLastError()); + + unbound_input2 = create_unbound_handle(FALSE, TRUE); + + ret = WaitForSingleObject(unbound_input2, 0); + ok(!ret, "got %d\n", ret); + + check_mouse_event(unbound_input); + + ret = WaitForSingleObject(output, 0); + todo_wine ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(unbound_output, 0); + todo_wine ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(unbound_input, 0); + todo_wine ok(!ret, "got %d\n", ret); + + ret = WaitForSingleObject(unbound_input2, 0); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + + ret = FreeConsole(); + ok(ret, "got error %lu\n", GetLastError()); + CloseHandle(input); + + ret = WaitForSingleObject(output, 0); + todo_wine ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(unbound_output, 0); + todo_wine ok(!ret, "got %d\n", ret); + ret = WaitForSingleObject(unbound_input, 0); + todo_wine ok(!ret, "got %d\n", ret); + handle = CreateConsoleScreenBuffer(GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, CONSOLE_TEXTMODE_BUFFER, NULL); @@ -4252,6 +4481,7 @@ static void test_FreeConsole(void) CloseHandle(unbound_input); CloseHandle(unbound_output); + CloseHandle(output); } static void test_SetConsoleScreenBufferInfoEx(HANDLE std_output) @@ -5473,6 +5703,12 @@ START_TEST(console) return; } + if (argc == 6 && !strcmp(argv[2], "unbound_handles")) + { + test_unbound_handles_child(strtoul(argv[3], NULL, 16), strtoull(argv[4], NULL, 16), strtoull(argv[5], NULL, 16)); + return; + } + test_current = argc >= 3 && !strcmp(argv[2], "--current"); using_pseudo_console = argc >= 3 && !strcmp(argv[2], "--pseudo-console"); @@ -5665,7 +5901,7 @@ START_TEST(console) test_pseudo_console(); test_AttachConsole(hConOut); test_AllocConsole(); - test_FreeConsole(); + test_FreeConsole(hConIn, hConOut); test_condrv_server_as_root_directory(); test_CreateProcessCUI(); test_CtrlHandlerSubsystem(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7608
From: Elizabeth Figura <zfigura(a)codeweavers.com> --- dlls/kernel32/tests/console.c | 9 +++++++-- server/console.c | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c index dd879725658..b879b7dcd80 100644 --- a/dlls/kernel32/tests/console.c +++ b/dlls/kernel32/tests/console.c @@ -4185,8 +4185,13 @@ static void test_unbound_handles_child(DWORD parent_pid, UINT_PTR parent_input_i FILE_WRITE_ATTRIBUTES, &attr, &iosb, NULL, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, FILE_CREATE, FILE_NON_DIRECTORY_FILE | FILE_SYNCHRONOUS_IO_NONALERT, NULL, 0 ); - todo_wine ok(status == STATUS_INVALID_HANDLE, "got %#lx\n", status); - if (!status) NtClose( unbound_input ); + ok(status == STATUS_INVALID_HANDLE, "got %#lx\n", status); + RtlInitUnicodeString( &name, L"\\Device\\ConDrv\\Output" ); + status = NtCreateFile( &unbound_input, FILE_READ_DATA | FILE_WRITE_DATA | SYNCHRONIZE | FILE_READ_ATTRIBUTES | + FILE_WRITE_ATTRIBUTES, &attr, &iosb, NULL, FILE_ATTRIBUTE_NORMAL, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, FILE_CREATE, + FILE_NON_DIRECTORY_FILE | FILE_SYNCHRONOUS_IO_NONALERT, NULL, 0 ); + ok(status == STATUS_INVALID_HANDLE, "got %#lx\n", status); /* Allocate a console. */ diff --git a/server/console.c b/server/console.c index c3e7bf57e92..16093fed4bc 100644 --- a/server/console.c +++ b/server/console.c @@ -1314,6 +1314,13 @@ static struct object *console_device_lookup_name( struct object *obj, struct uni if (name->len == sizeof(inputW) && !memcmp( name->str, inputW, name->len )) { struct console_input *console_input; + + if (!current->process->console) + { + set_error( STATUS_INVALID_HANDLE ); + return NULL; + } + name->len = 0; if (!(console_input = alloc_object( &console_input_ops ))) return NULL; console_input->fd = alloc_pseudo_fd( &console_input_fd_ops, &console_input->obj, @@ -1329,6 +1336,13 @@ static struct object *console_device_lookup_name( struct object *obj, struct uni if (name->len == sizeof(outputW) && !memcmp( name->str, outputW, name->len )) { struct console_output *console_output; + + if (!current->process->console) + { + set_error( STATUS_INVALID_HANDLE ); + return NULL; + } + name->len = 0; if (!(console_output = alloc_object( &console_output_ops ))) return NULL; console_output->fd = alloc_pseudo_fd( &console_output_fd_ops, &console_output->obj, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7608
From: Elizabeth Figura <zfigura(a)codeweavers.com> Wait on the handle directly instead of delegating to the console's wait queue. --- dlls/kernel32/tests/console.c | 8 ++++---- server/console.c | 36 +++++++++++++++++++---------------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c index b879b7dcd80..6126a1ffb6d 100644 --- a/dlls/kernel32/tests/console.c +++ b/dlls/kernel32/tests/console.c @@ -4346,7 +4346,7 @@ static void test_FreeConsole(HANDLE input, HANDLE orig_output) CloseHandle(orig_output); ret = WaitForSingleObject(output, 0); - todo_wine ok(!ret, "got %d\n", ret); + ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(unbound_output, 0); todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); ret = WaitForSingleObject(unbound_input, 0); @@ -4365,7 +4365,7 @@ static void test_FreeConsole(HANDLE input, HANDLE orig_output) ok(ret, "got error %lu\n", GetLastError()); ret = WaitForSingleObject(output, 0); - todo_wine ok(!ret, "got %d\n", ret); + ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(unbound_output, 0); todo_wine ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(unbound_input, 0); @@ -4384,7 +4384,7 @@ static void test_FreeConsole(HANDLE input, HANDLE orig_output) check_mouse_event(unbound_input); ret = WaitForSingleObject(output, 0); - todo_wine ok(!ret, "got %d\n", ret); + ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(unbound_output, 0); todo_wine ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(unbound_input, 0); @@ -4398,7 +4398,7 @@ static void test_FreeConsole(HANDLE input, HANDLE orig_output) CloseHandle(input); ret = WaitForSingleObject(output, 0); - todo_wine ok(!ret, "got %d\n", ret); + ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(unbound_output, 0); todo_wine ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(unbound_input, 0); diff --git a/server/console.c b/server/console.c index 16093fed4bc..8d495a31a71 100644 --- a/server/console.c +++ b/server/console.c @@ -214,7 +214,7 @@ struct screen_buffer static void screen_buffer_dump( struct object *obj, int verbose ); static void screen_buffer_destroy( struct object *obj ); -static int screen_buffer_add_queue( struct object *obj, struct wait_queue_entry *entry ); +static int screen_buffer_signaled( struct object *obj, struct wait_queue_entry *entry ); static struct fd *screen_buffer_get_fd( struct object *obj ); static struct object *screen_buffer_open_file( struct object *obj, unsigned int access, unsigned int sharing, unsigned int options ); @@ -224,10 +224,10 @@ static const struct object_ops screen_buffer_ops = sizeof(struct screen_buffer), /* size */ &file_type, /* type */ screen_buffer_dump, /* dump */ - screen_buffer_add_queue, /* add_queue */ - NULL, /* remove_queue */ - NULL, /* signaled */ - NULL, /* satisfied */ + add_queue, /* add_queue */ + remove_queue, /* remove_queue */ + screen_buffer_signaled, /* signaled */ + no_satisfied, /* satisfied */ no_signal, /* signal */ screen_buffer_get_fd, /* get_fd */ default_map_access, /* map_access */ @@ -839,21 +839,18 @@ static void screen_buffer_destroy( struct object *obj ) free_async_queue( &screen_buffer->ioctl_q ); } -static struct object *screen_buffer_open_file( struct object *obj, unsigned int access, - unsigned int sharing, unsigned int options ) +static int screen_buffer_signaled( struct object *obj, struct wait_queue_entry *entry ) { - return grab_object( obj ); + struct screen_buffer *screen_buffer = (struct screen_buffer *)obj; + assert( obj->ops == &screen_buffer_ops ); + if (!screen_buffer->input) return 1; + return screen_buffer->input->signaled; } -static int screen_buffer_add_queue( struct object *obj, struct wait_queue_entry *entry ) +static struct object *screen_buffer_open_file( struct object *obj, unsigned int access, + unsigned int sharing, unsigned int options ) { - struct screen_buffer *screen_buffer = (struct screen_buffer*)obj; - if (!screen_buffer->input) - { - set_error( STATUS_ACCESS_DENIED ); - return 0; - } - return console_add_queue( &screen_buffer->input->obj, entry ); + return grab_object( obj ); } static struct fd *screen_buffer_get_fd( struct object *obj ) @@ -1560,8 +1557,15 @@ DECL_HANDLER(get_next_console_request) if (!req->signal) server->console->signaled = 0; else if (!server->console->signaled) { + struct screen_buffer *screen_buffer; + server->console->signaled = 1; wake_up( &server->console->obj, 0 ); + LIST_FOR_EACH_ENTRY( screen_buffer, &screen_buffer_list, struct screen_buffer, entry ) + { + if (screen_buffer->input == server->console) + wake_up( &screen_buffer->obj, 0 ); + } } if (req->read) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7608
From: Elizabeth Figura <zfigura(a)codeweavers.com> Do not delegate to the current process's queue, either, but use the queue for this object. I/O acts on the current process's console, but the handle signaled state is the same across all handles to this object, and reflects the signaled state of the console of the process that created the object. --- dlls/kernel32/tests/console.c | 8 +++---- server/console.c | 39 +++++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c index 6126a1ffb6d..211a1990541 100644 --- a/dlls/kernel32/tests/console.c +++ b/dlls/kernel32/tests/console.c @@ -4168,7 +4168,7 @@ static void test_unbound_handles_child(DWORD parent_pid, UINT_PTR parent_input_i ok(ret, "got error %lu\n", GetLastError()); ret = WaitForSingleObject(parent_input, 0); - todo_wine ok(!ret, "got %d\n", ret); + ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(parent_output, 0); todo_wine ok(!ret, "got %d\n", ret); @@ -4227,7 +4227,7 @@ static void test_unbound_handles_child(DWORD parent_pid, UINT_PTR parent_input_i ok(ret == TRUE, "got error %lu\n", GetLastError()); ret = WaitForSingleObject(unbound_input, 0); - todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); CloseHandle(unbound_input); ret = WaitForSingleObject(parent_input, 0); @@ -4241,7 +4241,7 @@ static void test_unbound_handles_child(DWORD parent_pid, UINT_PTR parent_input_i ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(parent_input, 0); - todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); ret = WaitForSingleObject(parent_output, 0); todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); } @@ -4350,7 +4350,7 @@ static void test_FreeConsole(HANDLE input, HANDLE orig_output) ret = WaitForSingleObject(unbound_output, 0); todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); ret = WaitForSingleObject(unbound_input, 0); - todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); /* Create a new console. * diff --git a/server/console.c b/server/console.c index 8d495a31a71..1e01024a57e 100644 --- a/server/console.c +++ b/server/console.c @@ -296,12 +296,14 @@ struct console_input { struct object obj; /* object header */ struct fd *fd; /* pseudo-fd */ + struct list entry; /* entry in console_input_list */ + struct console *console; /* associated console at creation time */ }; static void console_input_dump( struct object *obj, int verbose ); +static int console_input_signaled( struct object *obj, struct wait_queue_entry *entry ); static struct object *console_input_open_file( struct object *obj, unsigned int access, unsigned int sharing, unsigned int options ); -static int console_input_add_queue( struct object *obj, struct wait_queue_entry *entry ); static struct fd *console_input_get_fd( struct object *obj ); static void console_input_destroy( struct object *obj ); @@ -310,9 +312,9 @@ static const struct object_ops console_input_ops = sizeof(struct console_input), /* size */ &device_type, /* type */ console_input_dump, /* dump */ - console_input_add_queue, /* add_queue */ - NULL, /* remove_queue */ - NULL, /* signaled */ + add_queue, /* add_queue */ + remove_queue, /* remove_queue */ + console_input_signaled, /* signaled */ no_satisfied, /* satisfied */ no_signal, /* signal */ console_input_get_fd, /* get_fd */ @@ -463,6 +465,7 @@ static const struct fd_ops console_connection_fd_ops = }; static struct list screen_buffer_list = LIST_INIT(screen_buffer_list); +static struct list console_input_list = LIST_INIT(console_input_list); static int queue_host_ioctl( struct console_server *server, unsigned int code, unsigned int output, struct async *async, struct async_queue *queue ); @@ -742,6 +745,7 @@ static void console_dump( struct object *obj, int verbose ) static void console_destroy( struct object *obj ) { struct console *console = (struct console *)obj; + struct console_input *input; struct screen_buffer *curr; assert( obj->ops == &console_ops ); @@ -760,6 +764,12 @@ static void console_destroy( struct object *obj ) if (curr->input == console) curr->input = NULL; } + LIST_FOR_EACH_ENTRY( input, &console_input_list, struct console_input, entry ) + { + if (input->console == console) + input->console = NULL; + } + free_async_queue( &console->ioctl_q ); free_async_queue( &console->read_q ); if (console->fd) @@ -1327,6 +1337,8 @@ static struct object *console_device_lookup_name( struct object *obj, struct uni release_object( console_input ); return NULL; } + console_input->console = current->process->console; + list_add_head( &console_input_list, &console_input->entry ); return &console_input->obj; } @@ -1402,14 +1414,12 @@ static void console_input_dump( struct object *obj, int verbose ) fputs( "console Input device\n", stderr ); } -static int console_input_add_queue( struct object *obj, struct wait_queue_entry *entry ) +static int console_input_signaled( struct object *obj, struct wait_queue_entry *entry ) { - if (!current->process->console) - { - set_error( STATUS_ACCESS_DENIED ); - return 0; - } - return console_add_queue( ¤t->process->console->obj, entry ); + struct console_input *console_input = (struct console_input *)obj; + assert( obj->ops == &console_input_ops ); + if (!console_input->console) return 0; + return console_input->console->signaled; } static struct fd *console_input_get_fd( struct object *obj ) @@ -1431,6 +1441,7 @@ static void console_input_destroy( struct object *obj ) assert( obj->ops == &console_input_ops ); if (console_input->fd) release_object( console_input->fd ); + list_remove( &console_input->entry ); } static void console_input_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) @@ -1558,6 +1569,7 @@ DECL_HANDLER(get_next_console_request) else if (!server->console->signaled) { struct screen_buffer *screen_buffer; + struct console_input *input; server->console->signaled = 1; wake_up( &server->console->obj, 0 ); @@ -1566,6 +1578,11 @@ DECL_HANDLER(get_next_console_request) if (screen_buffer->input == server->console) wake_up( &screen_buffer->obj, 0 ); } + LIST_FOR_EACH_ENTRY( input, &console_input_list, struct console_input, entry ) + { + if (input->console == server->console) + wake_up( &input->obj, 0 ); + } } if (req->read) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7608
From: Elizabeth Figura <zfigura(a)codeweavers.com> Do not delegate to the current process's queue, either, but use the queue for this object. I/O acts on the current process's console, but the handle signaled state is the same across all handles to this object, and reflects the signaled state of the console of the process that created the object. --- dlls/kernel32/tests/console.c | 6 +++--- server/console.c | 39 +++++++++++++++++++++++++---------- 2 files changed, 31 insertions(+), 14 deletions(-) diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c index 211a1990541..ea5339c936f 100644 --- a/dlls/kernel32/tests/console.c +++ b/dlls/kernel32/tests/console.c @@ -4170,7 +4170,7 @@ static void test_unbound_handles_child(DWORD parent_pid, UINT_PTR parent_input_i ret = WaitForSingleObject(parent_input, 0); ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(parent_output, 0); - todo_wine ok(!ret, "got %d\n", ret); + ok(!ret, "got %d\n", ret); ret = PeekConsoleInputW(parent_input, &ir, 1, &count); ok(!ret, "got %d\n", ret); @@ -4243,7 +4243,7 @@ static void test_unbound_handles_child(DWORD parent_pid, UINT_PTR parent_input_i ret = WaitForSingleObject(parent_input, 0); ok(ret == WAIT_TIMEOUT, "got %d\n", ret); ret = WaitForSingleObject(parent_output, 0); - todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); } static void test_FreeConsole(HANDLE input, HANDLE orig_output) @@ -4348,7 +4348,7 @@ static void test_FreeConsole(HANDLE input, HANDLE orig_output) ret = WaitForSingleObject(output, 0); ok(!ret, "got %d\n", ret); ret = WaitForSingleObject(unbound_output, 0); - todo_wine ok(ret == WAIT_TIMEOUT, "got %d\n", ret); + ok(ret == WAIT_TIMEOUT, "got %d\n", ret); ret = WaitForSingleObject(unbound_input, 0); ok(ret == WAIT_TIMEOUT, "got %d\n", ret); diff --git a/server/console.c b/server/console.c index 1e01024a57e..1d64a2195a9 100644 --- a/server/console.c +++ b/server/console.c @@ -355,10 +355,12 @@ struct console_output { struct object obj; /* object header */ struct fd *fd; /* pseudo-fd */ + struct list entry; /* entry in console_output_list */ + struct console *console; /* associated console at creation time */ }; static void console_output_dump( struct object *obj, int verbose ); -static int console_output_add_queue( struct object *obj, struct wait_queue_entry *entry ); +static int console_output_signaled( struct object *obj, struct wait_queue_entry *entry ); static struct fd *console_output_get_fd( struct object *obj ); static struct object *console_output_open_file( struct object *obj, unsigned int access, unsigned int sharing, unsigned int options ); @@ -369,9 +371,9 @@ static const struct object_ops console_output_ops = sizeof(struct console_output), /* size */ &device_type, /* type */ console_output_dump, /* dump */ - console_output_add_queue, /* add_queue */ - NULL, /* remove_queue */ - NULL, /* signaled */ + add_queue, /* add_queue */ + remove_queue, /* remove_queue */ + console_output_signaled, /* signaled */ no_satisfied, /* satisfied */ no_signal, /* signal */ console_output_get_fd, /* get_fd */ @@ -466,6 +468,7 @@ static const struct fd_ops console_connection_fd_ops = static struct list screen_buffer_list = LIST_INIT(screen_buffer_list); static struct list console_input_list = LIST_INIT(console_input_list); +static struct list console_output_list = LIST_INIT(console_output_list); static int queue_host_ioctl( struct console_server *server, unsigned int code, unsigned int output, struct async *async, struct async_queue *queue ); @@ -745,6 +748,7 @@ static void console_dump( struct object *obj, int verbose ) static void console_destroy( struct object *obj ) { struct console *console = (struct console *)obj; + struct console_output *output; struct console_input *input; struct screen_buffer *curr; @@ -770,6 +774,12 @@ static void console_destroy( struct object *obj ) input->console = NULL; } + LIST_FOR_EACH_ENTRY( output, &console_output_list, struct console_output, entry ) + { + if (output->console == console) + output->console = NULL; + } + free_async_queue( &console->ioctl_q ); free_async_queue( &console->read_q ); if (console->fd) @@ -1361,6 +1371,8 @@ static struct object *console_device_lookup_name( struct object *obj, struct uni release_object( console_output ); return NULL; } + console_output->console = current->process->console; + list_add_head( &console_output_list, &console_output->entry ); return &console_output->obj; } @@ -1485,14 +1497,12 @@ static void console_output_dump( struct object *obj, int verbose ) fputs( "console Output device\n", stderr ); } -static int console_output_add_queue( struct object *obj, struct wait_queue_entry *entry ) +static int console_output_signaled( struct object *obj, struct wait_queue_entry *entry ) { - if (!current->process->console || !current->process->console->active) - { - set_error( STATUS_ACCESS_DENIED ); - return 0; - } - return console_add_queue( ¤t->process->console->obj, entry ); + struct console_output *console_output = (struct console_output *)obj; + assert( obj->ops == &console_output_ops ); + if (!console_output->console) return 0; + return console_output->console->signaled; } static struct fd *console_output_get_fd( struct object *obj ) @@ -1514,6 +1524,7 @@ static void console_output_destroy( struct object *obj ) assert( obj->ops == &console_output_ops ); if (console_output->fd) release_object( console_output->fd ); + list_remove( &console_output->entry ); } static void console_output_ioctl( struct fd *fd, ioctl_code_t code, struct async *async ) @@ -1569,6 +1580,7 @@ DECL_HANDLER(get_next_console_request) else if (!server->console->signaled) { struct screen_buffer *screen_buffer; + struct console_output *output; struct console_input *input; server->console->signaled = 1; @@ -1583,6 +1595,11 @@ DECL_HANDLER(get_next_console_request) if (input->console == server->console) wake_up( &input->obj, 0 ); } + LIST_FOR_EACH_ENTRY( output, &console_output_list, struct console_output, entry ) + { + if (output->console == server->console) + wake_up( &output->obj, 0 ); + } } if (req->read) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7608
Pushed a v2 to address test failures. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7608#note_98464
Jacek Caban (@jacek) commented about server/console.c:
};
static struct list screen_buffer_list = LIST_INIT(screen_buffer_list); +static struct list console_input_list = LIST_INIT(console_input_list); I think it would be cleaner to store the list inside the console struct instead. On a related note, the same likely applies to the screen buffer list, its global scope is probably a leftover from the old console architecture.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7608#note_98526
Jacek Caban (@jacek) commented about server/console.c:
{ struct object obj; /* object header */ struct fd *fd; /* pseudo-fd */ + struct list entry; /* entry in console_output_list */ + struct console *console; /* associated console at creation time */ This is a bit surprising, given that these are meant to be unbound. I guess it's fine since tests back it up, but another explanation could be that it's actually bound to the parent process it was created in and, when needed, uses that process's current console. That would feel more consistent with my understanding of unbound console handles. Would you mind checking that theory?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7608#note_98527
On Thu Mar 20 15:05:31 2025 +0000, Jacek Caban wrote:
I think it would be cleaner to store the list inside the console struct instead. On a related note, the same likely applies to the screen buffer list, its global scope is probably a leftover from the old console architecture. Sure. The only reason I hesitated to do this is we need a separate field to mark whether it's attached to a console anyway, but I can see it being more idiomatic that way.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7608#note_98548
On Thu Mar 20 15:05:31 2025 +0000, Jacek Caban wrote:
This is a bit surprising, given that these are meant to be unbound. I guess it's fine since tests back it up, but another explanation could be that it's actually bound to the parent process it was created in and, when needed, uses that process's current console. That would feel more consistent with my understanding of unbound console handles. Would you mind checking that theory? The tests should in fact already show this, specifically the hunk starting with "Create a new console". It certainly would have made more sense to attach to the creating process, but nope.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7608#note_98549
On Thu Mar 20 20:57:19 2025 +0000, Elizabeth Figura wrote:
The tests should in fact already show this, specifically the hunk starting with "Create a new console". It certainly would have made more sense to attach to the creating process, but nope. Oh, right, I missed that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7608#note_98555
participants (3)
-
Elizabeth Figura -
Elizabeth Figura (＠zfigura)
-
Jacek Caban (＠jacek)