[PATCH v4 0/2] MR6021: nsiproxy.sys: Use net.inet.tcp.pcblist64 to enumerate connections on macOS.
The handles returned by libproc (namely struct socket_info's soi_pcb) use all 64 bits, but the ones from the pcblist sysctl are truncated to 32. That makes find_owning_pid fail. The pcblist64 sysctl was added in macOS 10.6 and returns handles that match those from libproc. -- There does not seem to be a MIB constant for pcblist64, so I had to fetch it with sysctlbyname. -- v4: nsiproxy.sys: Use the pcblist64 sysctl to enumerate UDP connections on macOS. https://gitlab.winehq.org/wine/wine/-/merge_requests/6021
From: Tim Clem <tclem(a)codeweavers.com> The handles returned by libproc (namely struct socket_info's soi_pcb) use all 64 bits, but the ones from the pcblist sysctl are truncated to 32. That makes find_owning_pid fail. The pcblist64 sysctl was added in macOS 10.6 and returns handles that match those from libproc. --- dlls/nsiproxy.sys/tcp.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/dlls/nsiproxy.sys/tcp.c b/dlls/nsiproxy.sys/tcp.c index 5734c4d9ee0..f9aa516e9f9 100644 --- a/dlls/nsiproxy.sys/tcp.c +++ b/dlls/nsiproxy.sys/tcp.c @@ -625,12 +625,24 @@ static NTSTATUS tcp_conns_enumerate_all( UINT filter, struct nsi_tcp_conn_key *k } #elif defined(HAVE_SYS_SYSCTL_H) && defined(TCPCTL_PCBLIST) && defined(HAVE_STRUCT_XINPGEN) { - int mib[] = { CTL_NET, PF_INET, IPPROTO_TCP, TCPCTL_PCBLIST }; size_t len = 0; char *buf = NULL; struct xinpgen *xig, *orig_xig; - if (sysctl( mib, ARRAY_SIZE(mib), NULL, &len, NULL, 0 ) < 0) +#ifdef __APPLE__ + static int mib[CTL_MAXNAME]; + static size_t mib_len = 0; + if (mib_len == 0) + { + mib_len = CTL_MAXNAME; + sysctlnametomib( "net.inet.tcp.pcblist64", mib, &mib_len ); + } +#else + int mib[] = { CTL_NET, PF_INET, IPPROTO_TCP, TCPCTL_PCBLIST }; + size_t mib_len = ARRAY_SIZE(mib); +#endif + + if (sysctl( mib, mib_len, NULL, &len, NULL, 0 ) < 0) { ERR( "Failure to read net.inet.tcp.pcblist via sysctl\n" ); status = STATUS_NOT_SUPPORTED; @@ -644,7 +656,7 @@ static NTSTATUS tcp_conns_enumerate_all( UINT filter, struct nsi_tcp_conn_key *k goto err; } - if (sysctl( mib, ARRAY_SIZE(mib), buf, &len, NULL, 0 ) < 0) + if (sysctl( mib, mib_len, buf, &len, NULL, 0 ) < 0) { ERR( "Failure to read net.inet.tcp.pcblist via sysctl\n" ); status = STATUS_NOT_SUPPORTED; @@ -664,7 +676,11 @@ static NTSTATUS tcp_conns_enumerate_all( UINT filter, struct nsi_tcp_conn_key *k xig->xig_len > sizeof(struct xinpgen); xig = (struct xinpgen *)((char *)xig + xig->xig_len)) { -#if __FreeBSD_version >= 1200026 +#ifdef __APPLE__ + struct xtcpcb64 *tcp = (struct xtcpcb64 *)xig; + struct xinpcb64 *in = &tcp->xt_inpcb; + struct xsocket64 *sock = &in->xi_socket; +#elif __FreeBSD_version >= 1200026 struct xtcpcb *tcp = (struct xtcpcb *)xig; struct xinpcb *in = &tcp->xt_inp; struct xsocket *sock = &in->xi_socket; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6021
From: Tim Clem <tclem(a)codeweavers.com> --- dlls/nsiproxy.sys/udp.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/dlls/nsiproxy.sys/udp.c b/dlls/nsiproxy.sys/udp.c index 2248d74234b..9c4b467173d 100644 --- a/dlls/nsiproxy.sys/udp.c +++ b/dlls/nsiproxy.sys/udp.c @@ -296,14 +296,26 @@ static NTSTATUS udp_endpoint_enumerate_all( void *key_data, UINT key_size, void } #elif defined(HAVE_SYS_SYSCTL_H) && defined(UDPCTL_PCBLIST) && defined(HAVE_STRUCT_XINPGEN) { - int mib[] = { CTL_NET, PF_INET, IPPROTO_UDP, UDPCTL_PCBLIST }; size_t len = 0; char *buf = NULL; struct xinpgen *xig, *orig_xig; - if (sysctl( mib, ARRAY_SIZE(mib), NULL, &len, NULL, 0 ) < 0) +#ifdef __APPLE__ + static int mib[CTL_MAXNAME]; + static size_t mib_len = 0; + if (mib_len == 0) + { + mib_len = CTL_MAXNAME; + sysctlnametomib( "net.inet.udp.pcblist64", mib, &mib_len ); + } +#else + int mib[] = { CTL_NET, PF_INET, IPPROTO_UDP, UDPCTL_PCBLIST }; + size_t mib_len = ARRAY_SIZE(mib); +#endif + + if (sysctl( mib, mib_len, NULL, &len, NULL, 0 ) < 0) { - ERR( "Failure to read net.inet.udp.pcblist via sysctlbyname!\n" ); + ERR( "Failure to read net.inet.udp.pcblist via sysctl!\n" ); status = STATUS_NOT_SUPPORTED; goto err; } @@ -315,9 +327,9 @@ static NTSTATUS udp_endpoint_enumerate_all( void *key_data, UINT key_size, void goto err; } - if (sysctl( mib, ARRAY_SIZE(mib), buf, &len, NULL, 0 ) < 0) + if (sysctl( mib, mib_len, buf, &len, NULL, 0 ) < 0) { - ERR( "Failure to read net.inet.udp.pcblist via sysctlbyname!\n" ); + ERR( "Failure to read net.inet.udp.pcblist via sysctl!\n" ); status = STATUS_NOT_SUPPORTED; goto err; } @@ -335,7 +347,10 @@ static NTSTATUS udp_endpoint_enumerate_all( void *key_data, UINT key_size, void xig->xig_len > sizeof (struct xinpgen); xig = (struct xinpgen *)((char *)xig + xig->xig_len)) { -#if __FreeBSD_version >= 1200026 +#ifdef __APPLE__ + struct xinpcb64 *in = (struct xinpcb64 *)xig; + struct xsocket64 *sock = &in->xi_socket; +#elif __FreeBSD_version >= 1200026 struct xinpcb *in = (struct xinpcb *)xig; struct xsocket *sock = &in->xi_socket; #else -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6021
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 tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=146942 Your paranoid android. === debian11b (64 bit WoW report) === ddraw: ddraw2.c:3728: Test failed: Expected screen size 1024x768, got 0x0. ddraw2.c:3735: Test failed: Expected (0,0)-(1024,768), got (-32000,-32000)-(-31840,-31969). ddraw2.c:3805: Test failed: Expected (0,0)-(640,480), got (-32000,-32000)-(-31840,-31969). ddraw2.c:3830: Test failed: Expected (0,0)-(640,480), got (-32000,-32000)-(-31840,-31969). ddraw7.c:3734: Test failed: Expected message 0x5, but didn't receive it. ddraw7.c:3736: Test failed: Expected screen size 1024x768, got 0x0. ddraw7.c:3743: Test failed: Expected (0,0)-(1024,768), got (-32000,-32000)-(-31840,-31969). ddraw7.c:3813: Test failed: Expected (0,0)-(640,480), got (-32000,-32000)-(-31840,-31969). ddraw7.c:3838: Test failed: Expected (0,0)-(640,480), got (-32000,-32000)-(-31840,-31969). user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000010700CC, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
Brendan Shanks (@bshanks) commented about dlls/nsiproxy.sys/udp.c:
} #elif defined(HAVE_SYS_SYSCTL_H) && defined(UDPCTL_PCBLIST) && defined(HAVE_STRUCT_XINPGEN) { - int mib[] = { CTL_NET, PF_INET, IPPROTO_UDP, UDPCTL_PCBLIST }; size_t len = 0; char *buf = NULL; struct xinpgen *xig, *orig_xig;
- if (sysctl( mib, ARRAY_SIZE(mib), NULL, &len, NULL, 0 ) < 0) +#ifdef __APPLE__ + static int mib[CTL_MAXNAME]; + static size_t mib_len = 0; + if (mib_len == 0) + { + mib_len = CTL_MAXNAME;
I'm not sure how likely it is, but if the first thread has set mib_len but sysctlnametomib() hasn't returned yet, another thread would call sysctl() with an invalid/empty mib. Maybe use dispatch_once() or pthread_once() to fill the cache? Or don't cache at all? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6021#note_75609
On Tue Jul 9 19:03:25 2024 +0000, Brendan Shanks wrote:
I'm not sure how likely it is, but if the first thread has set mib_len but sysctlnametomib() hasn't returned yet, another thread would call sysctl() with an invalid/empty mib. Maybe use dispatch_once() or pthread_once() to fill the cache? Or don't cache at all? Ah, good call. Breaking things out into a separate function for pthread_once would be verbose. What if I just stop having `mib_len` do double-duty and do something like this?
``` static int mib[CTL_MAXNAME], inited = 0; static size_t mib_len = CTL_MAXNAME; if (!inited) { sysctlnametomib( "net.inet.tcp.pcblist64", mib, &mib_len ); inited = 1; } ``` Then the worst case is an extra call to `sysctlnametomib`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6021#note_75623
On Tue Jul 9 19:37:37 2024 +0000, Tim Clem wrote:
Ah, good call. Breaking things out into a separate function for pthread_once would be verbose. What if I just stop having `mib_len` do double-duty and do something like this? ``` static int mib[CTL_MAXNAME], inited = 0; static size_t mib_len = CTL_MAXNAME; if (!inited) { sysctlnametomib( "net.inet.tcp.pcblist64", mib, &mib_len ); inited = 1; } ``` Then the worst case is an extra call to `sysctlnametomib`. Yeah that's better. I guess it's still possible that one thread could be inside sysctlnametomib() and writing to mib while another one is trying to call sysctl() for it, but that seems pretty remote.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6021#note_75809
On Wed Jul 10 19:23:10 2024 +0000, Brendan Shanks wrote:
Yeah that's better. I guess it's still possible that one thread could be inside sysctlnametomib() and writing to mib while another one is trying to call sysctl() for it, but that seems pretty remote. Yeah, it's still not thread-safe. I don't think adding a `get_pcblist_mib()` (or whatever) helper that used `pthread_once` would be too horrible.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6021#note_75883
participants (5)
-
Brendan Shanks (＠bshanks) -
Huw Davies (＠huw)
-
Marvin -
Tim Clem
-
Tim Clem (＠tclem)