June 2023 - Wine-gitlab - list.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/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

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: > break; > } > > + if (unix_addr.addr.sa_family == AF_UNIX) > + { > + status = STATUS_NOT_SUPPORTED; We should print a FIXME here as well. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35916

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: > .dwMessageSize = UINT_MAX, > .szProtocol = L"SPX II", > }, > + { > + .dwServiceFlags1 = XP1_GUARANTEED_DELIVERY | XP1_GUARANTEED_ORDER | XP1_IFS_HANDLES, > + .dwProviderFlags = PFL_MATCHES_PROTOCOL_ZERO, > + .ProviderId = {0xa00943d9, 0x9c2e, 0x4633, {0x9b, 0x59, 0x00, 0x57, 0xa3, 0x16, 0x09, 0x94}}, > + .dwCatalogEntryId = 1007, > + .ProtocolChain.ChainLen = 1, > + .iVersion = 2, > + .iAddressFamily = AF_UNIX, > + .iMaxSockAddr = sizeof(struct sockaddr_un), > + .iMinSockAddr = sizeof(struct sockaddr_un) - sizeof((struct sockaddr_un){}.sun_path), I don't think the {} construction is portable. We have offsetof() for this anyway. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35917

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: > } > break; > > + case AF_UNIX: > + if (len < sizeof(struct sockaddr_un)) > + { > + SetLastError( WSAEFAULT ); > + return -1; > + } > + break; Can the length be less than the whole size of sun_path? As always we should test for this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35918

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/ntdll/unix/socket.c: > return sizeof(uaddr->in6); > } > > + case WS_AF_UNIX: > + { > + struct WS_sockaddr_un win = {0}; > + > + if (wsaddrlen < sizeof(win)) return 0; > + memcpy( &win, wsaddr, sizeof(win) ); > + uaddr->un.sun_family = AF_UNIX; > + memcpy( uaddr->un.sun_path, win.sun_path, sizeof(win.sun_path) ); > + return sizeof(uaddr->un); This can't work, though; we need to convert it to Unix first. (Or print a FIXME and fail for DGRAM sockets.) Linux man-pages call out returning sizeof(sockaddr_un) as okay on Linux, but is it portable? We may want to return strlen()+1 just to be safe. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2786#note_35915

1 0