- wine-gitlab - winehq.org

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about dlls/ntdll/unix/socket.c: > } > #endif > > + case AF_UNIX: > + { > + struct WS_sockaddr_un win = {0}; > + > + if (wsaddrlen < sizeof(win)) return -1; > + win.sun_family = WS_AF_UNIX; > + memcpy( win.sun_path, uaddr->un.sun_path, sizeof(win.sun_path) ); > + memcpy( wsaddr, &win, sizeof(win) ); > + return sizeof(win); Similarly this isn't going to work without converting from Unix to win32 paths. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35928

1 0

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c: > + ok(!memcmp((char[108]){0}, outAddr.sun_path, sizeof(addr.sun_path)), > + "getsockname returned incorrect path '%s' for provided path '%s'\n", > + outAddr.sun_path, > + addr.sun_path); > + > + memset(&outAddr, 0, sizeof(outAddr)); > + ret = getsockname(server, (SOCKADDR *)&outAddr, &(int){sizeof(outAddr)}); > + ok(!ret, "Could not get info on Unix socket: %lu\n", GetLastError()); > + ok(!memcmp(addr.sun_path, outAddr.sun_path, sizeof(addr.sun_path)), > + "getsockname returned incorrect path '%s' for provided path '%s'\n", > + outAddr.sun_path, > + addr.sun_path); > + > + memset(&outAddr, 0, sizeof(outAddr)); > + ret = getpeername(listener, (SOCKADDR *)&outAddr, &(int){sizeof(outAddr)}); > + ok(ret == -1, "Got info on Unix socket: %lu\n", GetLastError()); If we're testing that something returns an error we should test the value of GetLastError() as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35927

1 0

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c: > + > + client = socket(AF_UNIX, SOCK_STREAM, 0); > + ok(client != INVALID_SOCKET, "Failed to create second Unix socket: %lu\n", > + GetLastError()); > + > + clientThread = CreateThread(NULL, 0, test_afunix_client_connect_thread, > + &(struct afunix_thread_param){ client, addr }, 0, NULL); > + ok(clientThread != NULL, "CreateThread failed unexpectedly: %ld\n", GetLastError()); > + server = accept(listener, NULL, NULL); > + ok(server != INVALID_SOCKET, "Could not accept Unix socket connection: %lu\n", > + GetLastError()); > + > + WaitForSingleObject(clientThread, 1000); > + > + memset(&outAddr, 0, sizeof(outAddr)); > + ret = getsockname(listener, (SOCKADDR *)&outAddr, &(int){sizeof(outAddr)}); Here and elsewhere, we should actually test the returned length. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35926

1 0

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c: > + ok(listener != INVALID_SOCKET, "Could not create Unix socket: %lu\n", > + GetLastError()); > + > + ret = bind(listener, (SOCKADDR *)&addr, sizeof(SOCKADDR_UN)); > + ok(!ret, "Could not bind Unix socket: %lu\n", GetLastError()); > + > + ret = listen(listener, 1); > + ok(!ret, "Could not listen on Unix socket: %lu\n", GetLastError()); > + > + client = socket(AF_UNIX, SOCK_STREAM, 0); > + ok(client != INVALID_SOCKET, "Failed to create second Unix socket: %lu\n", > + GetLastError()); > + > + clientThread = CreateThread(NULL, 0, test_afunix_client_connect_thread, > + &(struct afunix_thread_param){ client, addr }, 0, NULL); > + ok(clientThread != NULL, "CreateThread failed unexpectedly: %ld\n", GetLastError()); Do we need to bother with the client thread here? (Could we potentially make the client socket nonblocking instead?) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35924

1 0

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about dlls/ws2_32/tests/sock.c: > + > + ret = listen(listener, 1); > + ok(!ret, "Could not listen on Unix socket: %lu\n", GetLastError()); > + > + client = socket(AF_UNIX, SOCK_STREAM, 0); > + ok(client != INVALID_SOCKET, "Failed to create second Unix socket: %lu\n", > + GetLastError()); > + > + clientThread = CreateThread(NULL, 0, test_afunix_client_connect_thread, > + &(struct afunix_thread_param){ client, addr }, 0, NULL); > + ok(clientThread != NULL, "CreateThread failed unexpectedly: %ld\n", GetLastError()); > + server = accept(listener, NULL, NULL); > + ok(server != INVALID_SOCKET, "Could not accept Unix socket connection: %lu\n", > + GetLastError()); > + > + WaitForSingleObject(clientThread, 1000); If you're going to use a thread you should check this return value; if it fails then that means we have a bug in the implementation. We're also missing a CloseHandle(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35925

1 0

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about server/sock.c: > { > const struct afd_connect_params *params = get_req_data(); > const struct WS_sockaddr *addr; > - union unix_sockaddr unix_addr; > + union unix_sockaddr unix_addr, win_addr; This win_addr variable looks unused? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35922

1 0

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about server/sock.c: > } > unix_len = sizeof(unix_addr); > if (!getsockname( acceptfd, &unix_addr.addr, &unix_len )) > - acceptsock->addr_len = sockaddr_from_unix( &unix_addr, &acceptsock->addr.addr, sizeof(acceptsock->addr) ); > + { > + unix_len = sizeof(unix_addr); This second assignment to "unix_len" looks unused? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35923

1 0

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about server/sock.c: > if (unix_addr.addr.sa_family == AF_INET && !memcmp( &unix_addr.in.sin_addr, magic_loopback_addr, 4 )) > unix_addr.in.sin_addr.s_addr = htonl( INADDR_LOOPBACK ); > > + if (sock->family == WS_AF_UNIX && *((char *)(params + 1) + params->addr_len)) > + { > + char *unix_path = (char *)(params + 1) + params->addr_len; > + char unix_path_copy[PATH_MAX]; > + strcpy(unix_path_copy, unix_path); > + if (chdir( dirname( unix_path_copy ) ) == -1) > + { > + set_error( sock_get_ntstatus( errno ) ); > + return; > + } > + memcpy( unix_addr.un.sun_path, basename( unix_path ), sizeof(unix_addr.un.sun_path) ); We shouldn't need this anymore now that we're pssing the real unix path, right? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35921

1 0

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about dlls/ws2_32/socket.c: > > if (!(sync_event = get_sync_event())) return -1; > > - params = malloc( sizeof(int) + len ); > - ret_addr = malloc( len ); > + if (addr->sa_family == AF_UNIX && *addr->sa_data) Accessing sa_data in this way is convenient, but looks wrong. I'd rather just see the cast to sockaddr_un written out anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35919

1 0

Re: [PATCH v25 0/7] MR2786: Add support for AF_UNIX sockets
by Zebediah Figura (＠zfigura) 16 Jun '23

16 Jun '23

Zebediah Figura (@zfigura) commented about dlls/ws2_32/socket.c: > > free( params ); > free( ret_addr ); > + if (unix_path) > + free( unix_path ); You can skip the if here; free() does it for you. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35920

1 0

2025

2024

2023

2022

wine-gitlab