[PATCH 1/3] kernel32/tests: Add test for GetConsoleProcessList.
Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> --- dlls/kernel32/tests/console.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c index e4d9d43f4c4..94a0a370462 100644 --- a/dlls/kernel32/tests/console.c +++ b/dlls/kernel32/tests/console.c @@ -1304,8 +1304,8 @@ static void test_GetConsoleProcessList(void) GetLastError()); /* We should only have 1 process but only for these specific unit tests as - * we created our own console. An AttachConsole(ATTACH_PARENT_PROCESS) would - * give us two processes for example. + * we created our own console. An AttachConsole(ATTACH_PARENT_PROCESS) + * gives us two processes - see test_AttachConsole. */ list = HeapAlloc(GetProcessHeap(), 0, sizeof(DWORD)); @@ -4354,6 +4354,20 @@ static void test_AttachConsole_child(DWORD console_pid) res = AttachConsole(ATTACH_PARENT_PROCESS); ok(res, "AttachConsole failed: %u\n", GetLastError()); + if (pGetConsoleProcessList) + { + DWORD list[2]; + DWORD pid = GetCurrentProcessId(); + SetLastError(0xdeadbeef); + len = pGetConsoleProcessList(list, 2); + todo_wine + ok(len == 2, "Expected 2, got %d\n", len); + todo_wine + ok(list[0] == console_pid || list[1] == console_pid, "Parent PID not in list\n"); + todo_wine + ok(list[0] == pid || list[1] == pid, "PID not in list\n"); + } + ok(pipe_in != GetStdHandle(STD_INPUT_HANDLE), "std handle not set to console\n"); ok(pipe_out != GetStdHandle(STD_OUTPUT_HANDLE), "std handle not set to console\n"); -- 2.30.2
Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> --- include/wine/condrv.h | 1 + server/console.c | 55 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/include/wine/condrv.h b/include/wine/condrv.h index 4d2332a1ee9..13fb2c0b2c8 100644 --- a/include/wine/condrv.h +++ b/include/wine/condrv.h @@ -43,6 +43,7 @@ #define IOCTL_CONDRV_BEEP CTL_CODE(FILE_DEVICE_CONSOLE, 20, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_CONDRV_FLUSH CTL_CODE(FILE_DEVICE_CONSOLE, 21, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_CONDRV_GET_WINDOW CTL_CODE(FILE_DEVICE_CONSOLE, 22, METHOD_BUFFERED, FILE_ANY_ACCESS) +#define IOCTL_CONDRV_GET_PROCESS_LIST CTL_CODE(FILE_DEVICE_CONSOLE, 23, METHOD_BUFFERED, FILE_ANY_ACCESS) /* console output ioctls */ #define IOCTL_CONDRV_WRITE_CONSOLE CTL_CODE(FILE_DEVICE_CONSOLE, 30, METHOD_BUFFERED, FILE_WRITE_ACCESS) diff --git a/server/console.c b/server/console.c index 5407fba1411..2df8fed5dfb 100644 --- a/server/console.c +++ b/server/console.c @@ -707,6 +707,27 @@ static void propagate_console_signal( struct console *console, enum_processes(propagate_console_signal_cb, &csi); } +struct console_process_list +{ + DWORD size; + DWORD count; + DWORD *processes; + struct console *console; +}; + +static int console_process_list_cb(struct process *process, void *user) +{ + struct console_process_list* cpl = (struct console_process_list*)user; + + if (process->console == cpl->console) + { + if (cpl->count < cpl->size) cpl->processes[cpl->count] = process->id; + cpl->count++; + } + + return 0; +} + /* dumb dump */ static void console_dump( struct object *obj, int verbose ) { @@ -963,6 +984,40 @@ static void console_ioctl( struct fd *fd, ioctl_code_t code, struct async *async return; } + case IOCTL_CONDRV_GET_PROCESS_LIST: + { + struct console_process_list cpl; + cpl.count = 0; + cpl.size = get_reply_max_size() / sizeof(DWORD); + + if (cpl.size < 1) + { + set_error( STATUS_INVALID_PARAMETER ); + return; + } + if (!console) + { + set_error( STATUS_INVALID_HANDLE ); + return; + } + + cpl.console = console; + cpl.processes = (DWORD *) set_reply_data_size( get_reply_max_size() ); + enum_processes( console_process_list_cb, &cpl ); + + if (cpl.count > cpl.size) + { + current->reply_size = sizeof(DWORD); + *cpl.processes = cpl.count; + set_error( STATUS_BUFFER_TOO_SMALL ); + return; + } + + current->reply_size = cpl.count * sizeof(DWORD); + + return; + } + default: if (!console->server || code >> 16 != FILE_DEVICE_CONSOLE) { -- 2.30.2
Hi Roman, On 2/4/22 19:03, Roman Pišl wrote:
Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> --- include/wine/condrv.h | 1 + server/console.c | 55 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)
diff --git a/include/wine/condrv.h b/include/wine/condrv.h index 4d2332a1ee9..13fb2c0b2c8 100644 --- a/include/wine/condrv.h +++ b/include/wine/condrv.h @@ -43,6 +43,7 @@ #define IOCTL_CONDRV_BEEP CTL_CODE(FILE_DEVICE_CONSOLE, 20, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_CONDRV_FLUSH CTL_CODE(FILE_DEVICE_CONSOLE, 21, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_CONDRV_GET_WINDOW CTL_CODE(FILE_DEVICE_CONSOLE, 22, METHOD_BUFFERED, FILE_ANY_ACCESS) +#define IOCTL_CONDRV_GET_PROCESS_LIST CTL_CODE(FILE_DEVICE_CONSOLE, 23, METHOD_BUFFERED, FILE_ANY_ACCESS)
/* console output ioctls */ #define IOCTL_CONDRV_WRITE_CONSOLE CTL_CODE(FILE_DEVICE_CONSOLE, 30, METHOD_BUFFERED, FILE_WRITE_ACCESS) diff --git a/server/console.c b/server/console.c index 5407fba1411..2df8fed5dfb 100644 --- a/server/console.c +++ b/server/console.c @@ -707,6 +707,27 @@ static void propagate_console_signal( struct console *console, enum_processes(propagate_console_signal_cb, &csi); }
+struct console_process_list +{ + DWORD size; + DWORD count; + DWORD *processes; + struct console *console; +};
Please avoid using DWORD for internal server fields (size and count).
+ +static int console_process_list_cb(struct process *process, void *user) +{ + struct console_process_list* cpl = (struct console_process_list*)user;
The cast is not needed.
+ + if (process->console == cpl->console) + { + if (cpl->count < cpl->size) cpl->processes[cpl->count] = process->id; + cpl->count++; + } + + return 0; +} + /* dumb dump */ static void console_dump( struct object *obj, int verbose ) { @@ -963,6 +984,40 @@ static void console_ioctl( struct fd *fd, ioctl_code_t code, struct async *async return; }
+ case IOCTL_CONDRV_GET_PROCESS_LIST: + { + struct console_process_list cpl; + cpl.count = 0; + cpl.size = get_reply_max_size() / sizeof(DWORD); + + if (cpl.size < 1) + { + set_error( STATUS_INVALID_PARAMETER ); + return; + } + if (!console) + { + set_error( STATUS_INVALID_HANDLE ); + return; + }
There is no need to test for NULL console here.
+ + cpl.console = console; + cpl.processes = (DWORD *) set_reply_data_size( get_reply_max_size() ); + enum_processes( console_process_list_cb, &cpl ); + + if (cpl.count > cpl.size) + { + current->reply_size = sizeof(DWORD); + *cpl.processes = cpl.count; + set_error( STATUS_BUFFER_TOO_SMALL ); + return; + } + + current->reply_size = cpl.count * sizeof(DWORD);
over-allocating result and direct manipulation of reply_size does not really look nice. The usual practice would be to test size first and allocate appropriate result when we know its size. In this case, it's probably easiest if you do another enum_processes() call first, just to calculate result size (another solution would be to keep track of attached processes in console object itself). Thanks, Jacek
Hi Jacek, thank you for the review. I've submitted a second version of the patches, hopefully with your suggestions addressed. Roman Dne 08. 02. 22 v 22:32 Jacek Caban napsal(a):
Hi Roman,
On 2/4/22 19:03, Roman Pišl wrote:
Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> --- include/wine/condrv.h | 1 + server/console.c | 55 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)
diff --git a/include/wine/condrv.h b/include/wine/condrv.h index 4d2332a1ee9..13fb2c0b2c8 100644 --- a/include/wine/condrv.h +++ b/include/wine/condrv.h @@ -43,6 +43,7 @@ #define IOCTL_CONDRV_BEEP CTL_CODE(FILE_DEVICE_CONSOLE, 20, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_CONDRV_FLUSH CTL_CODE(FILE_DEVICE_CONSOLE, 21, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_CONDRV_GET_WINDOW CTL_CODE(FILE_DEVICE_CONSOLE, 22, METHOD_BUFFERED, FILE_ANY_ACCESS) +#define IOCTL_CONDRV_GET_PROCESS_LIST CTL_CODE(FILE_DEVICE_CONSOLE, 23, METHOD_BUFFERED, FILE_ANY_ACCESS) /* console output ioctls */ #define IOCTL_CONDRV_WRITE_CONSOLE CTL_CODE(FILE_DEVICE_CONSOLE, 30, METHOD_BUFFERED, FILE_WRITE_ACCESS) diff --git a/server/console.c b/server/console.c index 5407fba1411..2df8fed5dfb 100644 --- a/server/console.c +++ b/server/console.c @@ -707,6 +707,27 @@ static void propagate_console_signal( struct console *console, enum_processes(propagate_console_signal_cb, &csi); } +struct console_process_list +{ + DWORD size; + DWORD count; + DWORD *processes; + struct console *console; +};
Please avoid using DWORD for internal server fields (size and count).
+ +static int console_process_list_cb(struct process *process, void *user) +{ + struct console_process_list* cpl = (struct console_process_list*)user;
The cast is not needed.
+ + if (process->console == cpl->console) + { + if (cpl->count < cpl->size) cpl->processes[cpl->count] = process->id; + cpl->count++; + } + + return 0; +} + /* dumb dump */ static void console_dump( struct object *obj, int verbose ) { @@ -963,6 +984,40 @@ static void console_ioctl( struct fd *fd, ioctl_code_t code, struct async *async return; } + case IOCTL_CONDRV_GET_PROCESS_LIST: + { + struct console_process_list cpl; + cpl.count = 0; + cpl.size = get_reply_max_size() / sizeof(DWORD); + + if (cpl.size < 1) + { + set_error( STATUS_INVALID_PARAMETER ); + return; + } + if (!console) + { + set_error( STATUS_INVALID_HANDLE ); + return; + }
There is no need to test for NULL console here.
+ + cpl.console = console; + cpl.processes = (DWORD *) set_reply_data_size( get_reply_max_size() ); + enum_processes( console_process_list_cb, &cpl ); + + if (cpl.count > cpl.size) + { + current->reply_size = sizeof(DWORD); + *cpl.processes = cpl.count; + set_error( STATUS_BUFFER_TOO_SMALL ); + return; + } + + current->reply_size = cpl.count * sizeof(DWORD);
over-allocating result and direct manipulation of reply_size does not really look nice. The usual practice would be to test size first and allocate appropriate result when we know its size. In this case, it's probably easiest if you do another enum_processes() call first, just to calculate result size (another solution would be to keep track of attached processes in console object itself).
Thanks,
Jacek
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48760 Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> --- dlls/kernel32/console.c | 18 ++++++++++++++++-- dlls/kernel32/tests/console.c | 5 ----- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/dlls/kernel32/console.c b/dlls/kernel32/console.c index 80f3419cd7a..621103978fa 100644 --- a/dlls/kernel32/console.c +++ b/dlls/kernel32/console.c @@ -252,7 +252,10 @@ DWORD WINAPI GetConsoleAliasW(LPWSTR lpSource, LPWSTR lpTargetBuffer, */ DWORD WINAPI GetConsoleProcessList(LPDWORD processlist, DWORD processcount) { - FIXME("(%p,%d): stub\n", processlist, processcount); + NTSTATUS status; + IO_STATUS_BLOCK io; + + TRACE("(%p,%d)\n", processlist, processcount); if (!processlist || processcount < 1) { @@ -260,7 +263,18 @@ DWORD WINAPI GetConsoleProcessList(LPDWORD processlist, DWORD processcount) return 0; } - return 0; + status = NtDeviceIoControlFile( RtlGetCurrentPeb()->ProcessParameters->ConsoleHandle, + NULL, NULL, NULL, &io, IOCTL_CONDRV_GET_PROCESS_LIST, + NULL, 0, processlist, processcount * sizeof(DWORD) ); + + if (status == STATUS_BUFFER_TOO_SMALL) return *processlist; + if (status) + { + SetLastError(RtlNtStatusToDosError(status)); + return 0; + } + + return io.Information / sizeof(DWORD); } /* Undocumented, called by native doskey.exe */ diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c index 94a0a370462..ef28debb6ff 100644 --- a/dlls/kernel32/tests/console.c +++ b/dlls/kernel32/tests/console.c @@ -1318,7 +1318,6 @@ static void test_GetConsoleProcessList(void) SetLastError(0xdeadbeef); ret = pGetConsoleProcessList(list, 1); - todo_wine ok(ret == 1, "Expected 1, got %d\n", ret); HeapFree(GetProcessHeap(), 0, list); @@ -1327,7 +1326,6 @@ static void test_GetConsoleProcessList(void) SetLastError(0xdeadbeef); ret = pGetConsoleProcessList(list, ret); - todo_wine ok(ret == 1, "Expected 1, got %d\n", ret); if (ret == 1) @@ -4360,11 +4358,8 @@ static void test_AttachConsole_child(DWORD console_pid) DWORD pid = GetCurrentProcessId(); SetLastError(0xdeadbeef); len = pGetConsoleProcessList(list, 2); - todo_wine ok(len == 2, "Expected 2, got %d\n", len); - todo_wine ok(list[0] == console_pid || list[1] == console_pid, "Parent PID not in list\n"); - todo_wine ok(list[0] == pid || list[1] == pid, "PID not in list\n"); } -- 2.30.2
Hi Roman, On 2/4/22 19:03, Roman Pišl wrote:
Signed-off-by: Roman Pišl <rpisl(a)seznam.cz> --- dlls/kernel32/tests/console.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c index e4d9d43f4c4..94a0a370462 100644 --- a/dlls/kernel32/tests/console.c +++ b/dlls/kernel32/tests/console.c @@ -1304,8 +1304,8 @@ static void test_GetConsoleProcessList(void) GetLastError());
/* We should only have 1 process but only for these specific unit tests as - * we created our own console. An AttachConsole(ATTACH_PARENT_PROCESS) would - * give us two processes for example. + * we created our own console. An AttachConsole(ATTACH_PARENT_PROCESS) + * gives us two processes - see test_AttachConsole. */ list = HeapAlloc(GetProcessHeap(), 0, sizeof(DWORD));
@@ -4354,6 +4354,20 @@ static void test_AttachConsole_child(DWORD console_pid) res = AttachConsole(ATTACH_PARENT_PROCESS); ok(res, "AttachConsole failed: %u\n", GetLastError());
+ if (pGetConsoleProcessList) + { + DWORD list[2]; + DWORD pid = GetCurrentProcessId(); + SetLastError(0xdeadbeef); + len = pGetConsoleProcessList(list, 2); + todo_wine + ok(len == 2, "Expected 2, got %d\n", len); + todo_wine + ok(list[0] == console_pid || list[1] == console_pid, "Parent PID not in list\n"); + todo_wine + ok(list[0] == pid || list[1] == pid, "PID not in list\n"); + } + ok(pipe_in != GetStdHandle(STD_INPUT_HANDLE), "std handle not set to console\n"); ok(pipe_out != GetStdHandle(STD_OUTPUT_HANDLE), "std handle not set to console\n");
This test looks fine, but it would be also interesting to pass length 1 here. We have an opportunity to test STATUS_BUFFER_TOO_SMALL now (that was not possible when only one process was on the list). Thanks, Jacek
participants (2)
-
Jacek Caban -
Roman Pišl