[PATCH 0/3] MR1988: ntoskrnl.exe/tests: Don't bind to specific listen port from userspace.
Today, the driver_netio test client binds a new socket to a specific network port. This triggers a firewall alert on Windows 7, and the alert UI window may interfere with user32:msg tests. Fix this by binding to a dynamically-allocated port instead. Related: - https://bugs.winehq.org/show_bug.cgi?id=53891 - https://bugs.winehq.org/show_bug.cgi?id=54202 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1988
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/ntoskrnl.exe/tests/driver.h | 1 + dlls/ntoskrnl.exe/tests/ntoskrnl.c | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/driver.h b/dlls/ntoskrnl.exe/tests/driver.h index 34dfdca476c..774908a5c4a 100644 --- a/dlls/ntoskrnl.exe/tests/driver.h +++ b/dlls/ntoskrnl.exe/tests/driver.h @@ -64,6 +64,7 @@ struct main_test_input DWORD process_id; SIZE_T teststr_offset; ULONG64 *modified_value; + ULONG_PTR parameter; }; struct return_status_params diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index ff93cfc98ec..e80b2368352 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -392,7 +392,7 @@ static void cat_okfile(void) static ULONG64 modified_value; -static void main_test(void) +static void main_test(ULONG_PTR parameter) { struct main_test_input *test_input; DWORD size; @@ -402,6 +402,7 @@ static void main_test(void) test_input->process_id = GetCurrentProcessId(); test_input->teststr_offset = (SIZE_T)((BYTE *)&teststr - (BYTE *)NtCurrentTeb()->Peb->ImageBaseAddress); test_input->modified_value = &modified_value; + test_input->parameter = parameter; modified_value = 0; res = DeviceIoControl(device, IOCTL_WINETEST_MAIN_TEST, test_input, sizeof(*test_input), NULL, 0, &size, NULL); @@ -1248,7 +1249,7 @@ static void test_driver_netio(struct testsign_context *ctx) ok(device != INVALID_HANDLE_VALUE, "failed to open device: %lu\n", GetLastError()); hthread = CreateThread(NULL, 0, wsk_test_thread, NULL, 0, NULL); - main_test(); + main_test(0); WaitForSingleObject(hthread, INFINITE); CloseHandle(device); @@ -1933,7 +1934,7 @@ START_TEST(ntoskrnl) test_basic_ioctl(); - main_test(); + main_test(0); todo_wine ok(modified_value == 0xdeadbeeffeedcafe, "Got unexpected value %#I64x.\n", modified_value); test_overlapped(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1988
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Today, the driver_netio test client uses SO_REUSEADDR to reuse previously used network address and port for its socket. This may trigger a firewall alert on Windows 7, and the alert UI window may interfere with user32:msg tests. Fix this by removing call to setsockopt([...], SO_REUSEADDR, [...]). Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53891 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54202 --- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index ee1516fc887..ee94bf45186 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1186,7 +1186,6 @@ static DWORD WINAPI wsk_test_thread(void *parameter) int ret, err, socklen; char buffer[256]; WSADATA data; - int opt_val; ret = WSAStartup(version, &data); ok(!ret, "WSAStartup() failed, ret %u.\n", ret); @@ -1197,9 +1196,6 @@ static DWORD WINAPI wsk_test_thread(void *parameter) s_listen = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); ok(s_listen != INVALID_SOCKET, "Error creating socket, WSAGetLastError() %u.\n", WSAGetLastError()); - opt_val = 1; - setsockopt(s_listen, SOL_SOCKET, SO_REUSEADDR, (const char *)&opt_val, sizeof(opt_val)); - memset(&addr, 0, sizeof(addr)); addr.sin_family = AF_INET; addr.sin_addr.s_addr = htonl(0x7f000001); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1988
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=128544 Your paranoid android. === w1064_tsign (64 bit report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout === debian11 (32 bit report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout === debian11 (32 bit ar:MA report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout === debian11 (32 bit de report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout === debian11 (32 bit fr report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout === debian11 (32 bit he:IL report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout === debian11 (32 bit hi:IN report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout === debian11 (32 bit ja:JP report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout === debian11 (32 bit zh:CN report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout === debian11b (64 bit WoW report) === ntoskrnl.exe: ntoskrnl.c:1209: Test failed: Got unexpected ret -1, WSAGetLastError() 10014. ntoskrnl.c:1210: Test failed: expected 16, got 0. ntoskrnl.c:1268: Test failed: Could not bind socket. ntoskrnl: Timeout
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Today, the driver_netio test client binds a new socket to a specific network port. This triggers a firewall alert on Windows 7, and the alert UI window may interfere with user32:msg tests. Fix this by binding to a dynamically-allocated port instead. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53891 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54202 --- dlls/ntoskrnl.exe/tests/driver.h | 1 - dlls/ntoskrnl.exe/tests/driver_netio.c | 10 ++++---- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 32 ++++++++++++++++++++++---- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/driver.h b/dlls/ntoskrnl.exe/tests/driver.h index 774908a5c4a..6d3f5643aec 100644 --- a/dlls/ntoskrnl.exe/tests/driver.h +++ b/dlls/ntoskrnl.exe/tests/driver.h @@ -77,4 +77,3 @@ struct return_status_params static const GUID control_class = {0xdeadbeef, 0x29ef, 0x4538, {0xa5, 0xfd, 0xb6, 0x95, 0x73, 0xa3, 0x62, 0xc0}}; #define SERVER_LISTEN_PORT 9374 -#define CLIENT_LISTEN_PORT 9375 diff --git a/dlls/ntoskrnl.exe/tests/driver_netio.c b/dlls/ntoskrnl.exe/tests/driver_netio.c index e406f49eacb..528356b923c 100644 --- a/dlls/ntoskrnl.exe/tests/driver_netio.c +++ b/dlls/ntoskrnl.exe/tests/driver_netio.c @@ -381,9 +381,10 @@ static void test_wsk_listen_socket(void) ExFreePool(buffer2); } -static void test_wsk_connect_socket(void) +static void test_wsk_connect_socket(ULONG_PTR parameter) { const WSK_PROVIDER_CONNECTION_DISPATCH *connect_dispatch; + USHORT client_listen_port = (USHORT)parameter; struct socket_context context; struct sockaddr_in addr; LARGE_INTEGER timeout; @@ -409,7 +410,7 @@ static void test_wsk_connect_socket(void) memset(&addr, 0, sizeof(addr)); addr.sin_family = AF_INET; - addr.sin_port = htons(CLIENT_LISTEN_PORT); + addr.sin_port = htons(client_listen_port); addr.sin_addr.s_addr = htonl(0x7f000001); IoReuseIrp(wsk_irp, STATUS_UNSUCCESSFUL); @@ -437,7 +438,7 @@ static void test_wsk_connect_socket(void) ok(!wsk_irp->IoStatus.Information, "Got unexpected Information %#Ix.\n", wsk_irp->IoStatus.Information); - addr.sin_port = htons(CLIENT_LISTEN_PORT); + addr.sin_port = htons(client_listen_port); addr.sin_addr.s_addr = htonl(0x7f000001); IoReuseIrp(wsk_irp, STATUS_UNSUCCESSFUL); @@ -466,6 +467,7 @@ static void test_wsk_connect_socket(void) static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *stack) { void *buffer = irp->AssociatedIrp.SystemBuffer; + struct main_test_input *test_input = buffer; if (!buffer) return STATUS_ACCESS_VIOLATION; @@ -473,7 +475,7 @@ static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *st netio_init(); test_wsk_get_address_info(); test_wsk_listen_socket(); - test_wsk_connect_socket(); + test_wsk_connect_socket(test_input->parameter); irp->IoStatus.Information = 0; return STATUS_SUCCESS; diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index e80b2368352..ee1516fc887 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1170,14 +1170,21 @@ static void test_driver3(struct testsign_context *ctx) DeleteFileW(filename); } +struct wsk_test_cb +{ + HANDLE client_started_event; + USHORT client_listen_port; +}; + static DWORD WINAPI wsk_test_thread(void *parameter) { static const char test_send_string[] = "Client test string 1."; static const WORD version = MAKEWORD(2, 2); SOCKET s_listen, s_accept, s_connect; + struct wsk_test_cb *cb = parameter; struct sockaddr_in addr; + int ret, err, socklen; char buffer[256]; - int ret, err; WSADATA data; int opt_val; @@ -1195,7 +1202,6 @@ static DWORD WINAPI wsk_test_thread(void *parameter) memset(&addr, 0, sizeof(addr)); addr.sin_family = AF_INET; - addr.sin_port = htons(CLIENT_LISTEN_PORT); addr.sin_addr.s_addr = htonl(0x7f000001); ret = bind(s_listen, (struct sockaddr *)&addr, sizeof(addr)); ok(!ret, "Got unexpected ret %d, WSAGetLastError() %u.\n", ret, WSAGetLastError()); @@ -1203,6 +1209,13 @@ static DWORD WINAPI wsk_test_thread(void *parameter) ret = listen(s_listen, SOMAXCONN); ok(!ret, "Got unexpected ret %d, WSAGetLastError() %u.\n", ret, WSAGetLastError()); + ret = getsockname(s_listen, (struct sockaddr *)&addr, &socklen); + ok(!ret, "Got unexpected ret %d, WSAGetLastError() %u.\n", ret, WSAGetLastError()); + ok(socklen == sizeof(addr), "expected %Iu, got %d.\n", sizeof(addr), socklen); + + cb->client_listen_port = ntohs(addr.sin_port); + SetEvent(cb->client_started_event); + addr.sin_port = htons(SERVER_LISTEN_PORT); ret = connect(s_connect, (struct sockaddr *)&addr, sizeof(addr)); @@ -1231,6 +1244,7 @@ static DWORD WINAPI wsk_test_thread(void *parameter) static void test_driver_netio(struct testsign_context *ctx) { + struct wsk_test_cb cb = { NULL }; WCHAR filename[MAX_PATH]; SC_HANDLE service; HANDLE hthread; @@ -1248,10 +1262,20 @@ static void test_driver_netio(struct testsign_context *ctx) device = CreateFileA("\\\\.\\winetest_netio", 0, 0, NULL, OPEN_EXISTING, 0, NULL); ok(device != INVALID_HANDLE_VALUE, "failed to open device: %lu\n", GetLastError()); - hthread = CreateThread(NULL, 0, wsk_test_thread, NULL, 0, NULL); - main_test(0); + cb.client_started_event = CreateEventW(NULL, FALSE, FALSE, NULL); + ok(cb.client_started_event != NULL, "CreateEventW returned error %lu\n", GetLastError()); + + hthread = CreateThread(NULL, 0, wsk_test_thread, &cb, 0, NULL); + ok(hthread != NULL, "CreateThread returned error %lu\n", GetLastError()); + + WaitForSingleObject(cb.client_started_event, INFINITE); + ok(cb.client_listen_port != 0, "Could not bind socket.\n"); + + main_test(cb.client_listen_port); WaitForSingleObject(hthread, INFINITE); + CloseHandle(hthread); + CloseHandle(cb.client_started_event); CloseHandle(device); unload_driver(service); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1988
Do I understand correctly that the permissions dialog popup is caused solely by SO_REUSEADDR? If that is the case, did you try to just remove setting this option without doing anything else, does anything break? I am not fully sure now without more testing, but it is possible this SO_REUSEADDR was only needed to avoid test failures on Wine when closing the socket do not immediately make the port available for listen (unlike Windows). SO_REUSEADDR has completely different semantics between winsock and BSD sockets. Since not so long ago the compatibility on this part has been improved in Wine, we now always set native SO_REUSEADDR on TCP sockets. And test expectedly doesn't break for me in Wine if I just remove SO_REUSEADDR. I didn't test it on Windows (only run on testbot) but I'd hope it will be fine there as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1988#note_21269
On Sat Jan 21 00:42:27 2023 +0000, Paul Gofman wrote:
Do I understand correctly that the permissions dialog popup is caused solely by SO_REUSEADDR? If that is the case, did you try to just remove setting this option without doing anything else, does anything break? I am not fully sure now without more testing, but it is possible this SO_REUSEADDR was only needed to avoid test failures on Wine when closing the socket do not immediately make the port available for listen (unlike Windows). SO_REUSEADDR has completely different semantics between winsock and BSD sockets. Since not so long ago the compatibility on this part has been improved in Wine, we now always set native SO_REUSEADDR on TCP sockets. And test expectedly doesn't break for me in Wine if I just remove SO_REUSEADDR. I didn't test it on Windows (only run on testbot) but I'd hope it will be fine there as well. No, the actual culprit turned out to be binding to `INADDR_ANY` after more testing. It is not clear that `SO_REUSEADDR` is triggering the firewall. This MR does not prevent the firewall alert dialog. I'm submitting a separate PR for that.
(I actually realized this much earlier, but I have only so much time...) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1988#note_21294
This merge request was closed by Jinoh Kang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1988
On Sat Jan 21 00:43:00 2023 +0000, Jinoh Kang wrote:
No, the actual culprit turned out to be binding to `INADDR_ANY` after more testing. It is not clear that `SO_REUSEADDR` is triggering the firewall. This MR does not prevent the firewall alert dialog. I'm submitting a separate PR for that. (I actually realized this much earlier, but I have only so much time...) My original theory was that the firewall dialog was caused by _both_ `SO_REUSEADDR` _and_ binding to a fixed port (perhaps only in well-known or registered range?) I thought not hard-coding ports in tests was a good idea in general, but now that the actual cause of the alert turned out to be different, it's true that the extra complexity introduced by dynamic ports is much harder to justify.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1988#note_21295
participants (4)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin -
Paul Gofman (＠gofman)