[PATCH v6 0/2] MR1015: wpcap: Convert Unix sockaddr to win32 before duplication
Interpreting the fist two bytes of the `sockaddr` struct as `sa_family` produces incorrect behaviour on macOS and (presumbly) BSD. This change uses both Unix and Windows socket headers to convert the libpcap provided sockaddr before duplicating it. This fixes sockaddr being always NULL on macOS/BSD and ipv6 addresses not working in general since `AF_INET6` differs on Windows(23), macOS(30) and Linux(10) For reference https://www.ibm.com/docs/en/i/7.3?topic=family-af-inet-address and this is from the <sys/socket.h> on macOS ``` /* * [XSI] Structure used by kernel to store most addresses. */ struct sockaddr { __uint8_t sa_len; /* total length */ sa_family_t sa_family; /* [XSI] address family */ char sa_data[14]; /* [XSI] addr value (actually larger) */ }; ``` -- v6: wpcap: Build devices in unixlib for find_all_devices. wpcap: Split device allocation and build in find_all_devices. https://gitlab.winehq.org/wine/wine/-/merge_requests/1015
From: Marc-Aurel Zent <marc_aurel(a)me.com> Additionally stores socket addresses in sockaddr_storage, since the socket address size is not known during allocation. --- dlls/wpcap/unixlib.c | 6 ++ dlls/wpcap/unixlib.h | 12 ++-- dlls/wpcap/wpcap.c | 129 ++++++++++++++++++++++++++++--------------- 3 files changed, 96 insertions(+), 51 deletions(-) diff --git a/dlls/wpcap/unixlib.c b/dlls/wpcap/unixlib.c index a44627a7754..ebe37ca5909 100644 --- a/dlls/wpcap/unixlib.c +++ b/dlls/wpcap/unixlib.c @@ -34,6 +34,12 @@ #include "windef.h" #include "winbase.h" #include "winternl.h" +#define USE_WS_PREFIX +/* the following 2 libpcap defines interfere with winsock2.h */ +#undef SOCKET +#undef INVALID_SOCKET +#include "winsock2.h" +#include "ws2ipdef.h" #include "wine/unixlib.h" #include "wine/debug.h" diff --git a/dlls/wpcap/unixlib.h b/dlls/wpcap/unixlib.h index 8cd64ef0210..6b815379e7f 100644 --- a/dlls/wpcap/unixlib.h +++ b/dlls/wpcap/unixlib.h @@ -17,18 +17,14 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ -struct sockaddr_hdr -{ - unsigned short sa_family; -}; struct pcap_address { struct pcap_address *next; - struct sockaddr_hdr *addr; - struct sockaddr_hdr *netmask; - struct sockaddr_hdr *broadaddr; - struct sockaddr_hdr *dstaddr; + SOCKADDR_STORAGE *addr; + SOCKADDR_STORAGE *netmask; + SOCKADDR_STORAGE *broadaddr; + SOCKADDR_STORAGE *dstaddr; }; struct pcap_interface diff --git a/dlls/wpcap/wpcap.c b/dlls/wpcap/wpcap.c index df35954bfa7..0863849f7ae 100644 --- a/dlls/wpcap/wpcap.c +++ b/dlls/wpcap/wpcap.c @@ -238,51 +238,45 @@ static char *build_win32_description( const struct pcap_interface *unix_dev ) return ret; } -static struct sockaddr_hdr *dup_sockaddr( const struct sockaddr_hdr *addr ) +static int dup_sockaddr( const struct sockaddr_storage *unix_addr, struct sockaddr_storage *win32_addr ) { - struct sockaddr_hdr *ret; - - switch (addr->sa_family) + switch (unix_addr->ss_family) { case AF_INET: { - struct sockaddr_in *dst, *src = (struct sockaddr_in *)addr; - if (!(dst = calloc( 1, sizeof(*dst) ))) return NULL; + struct sockaddr_in *src = (struct sockaddr_in *)unix_addr; + struct sockaddr_in *dst = (struct sockaddr_in *)win32_addr; dst->sin_family = src->sin_family; dst->sin_port = src->sin_port; dst->sin_addr = src->sin_addr; - ret = (struct sockaddr_hdr *)dst; - break; + return 0; } case AF_INET6: { - struct sockaddr_in6 *dst, *src = (struct sockaddr_in6 *)addr; - if (!(dst = malloc( sizeof(*dst) ))) return NULL; + struct sockaddr_in6 *src = (struct sockaddr_in6 *)unix_addr; + struct sockaddr_in6 *dst = (struct sockaddr_in6 *)win32_addr; dst->sin6_family = src->sin6_family; dst->sin6_port = src->sin6_port; dst->sin6_flowinfo = src->sin6_flowinfo; dst->sin6_addr = src->sin6_addr; dst->sin6_scope_id = src->sin6_scope_id; - ret = (struct sockaddr_hdr *)dst; - break; + return 0; } default: - FIXME( "address family %u not supported\n", addr->sa_family ); - return NULL; + FIXME( "address family %u not supported\n", unix_addr->ss_family ); + return -1; } - - return ret; } -static struct pcap_address *build_win32_address( struct pcap_address *src ) +static struct pcap_address *alloc_win32_address( const struct pcap_address *src ) { struct pcap_address *dst; if (!(dst = calloc( 1, sizeof(*dst) ))) return NULL; - if (src->addr && !(dst->addr = dup_sockaddr( src->addr ))) goto err; - if (src->netmask && !(dst->netmask = dup_sockaddr( src->netmask ))) goto err; - if (src->broadaddr && !(dst->broadaddr = dup_sockaddr( src->broadaddr ))) goto err; - if (src->dstaddr && !(dst->dstaddr = dup_sockaddr( src->dstaddr ))) goto err; + if (src->addr && !(dst->addr = calloc( 1, sizeof( *dst->addr )))) goto err; + if (src->netmask && !(dst->netmask = calloc( 1, sizeof( *dst->netmask )))) goto err; + if (src->broadaddr && !(dst->broadaddr = calloc( 1, sizeof( *dst->broadaddr )))) goto err; + if (src->dstaddr && !(dst->dstaddr = calloc( 1, sizeof( *dst->dstaddr )))) goto err; return dst; err: @@ -294,6 +288,15 @@ err: return NULL; } +static int build_win32_address( const struct pcap_address *unix_addr, struct pcap_address *win32_addr ) +{ + if (unix_addr->addr && dup_sockaddr( unix_addr->addr, win32_addr->addr )) return -1; + if (unix_addr->netmask && dup_sockaddr( unix_addr->netmask, win32_addr->netmask )) return -1; + if (unix_addr->broadaddr && dup_sockaddr( unix_addr->broadaddr, win32_addr->broadaddr )) return -1; + if (unix_addr->dstaddr && dup_sockaddr( unix_addr->dstaddr, win32_addr->dstaddr )) return -1; + return 0; +} + static void add_win32_address( struct pcap_address **list, struct pcap_address *addr ) { struct pcap_address *cur = *list; @@ -305,19 +308,31 @@ static void add_win32_address( struct pcap_address **list, struct pcap_address * } } -static struct pcap_address *build_win32_addresses( struct pcap_address *addrs ) +static struct pcap_address *alloc_win32_addresses( struct pcap_address *addrs ) { struct pcap_address *src, *dst, *ret = NULL; src = addrs; while (src) { - if ((dst = build_win32_address( src ))) add_win32_address( &ret, dst ); + if ((dst = alloc_win32_address( src ))) add_win32_address( &ret, dst ); src = src->next; } return ret; } -static struct pcap_interface *build_win32_device( const struct pcap_interface *unix_dev, const char *source, +static void build_win32_addresses( struct pcap_address *unix_addrs, struct pcap_address *win32_addrs ) +{ + while (unix_addrs && win32_addrs) + { + if (!build_win32_address( unix_addrs, win32_addrs )) + win32_addrs = win32_addrs->next; + unix_addrs = unix_addrs->next; + } + + free_addresses( win32_addrs ); /* addresses whose socket family could not be mapped */ +} + +static struct pcap_interface *alloc_win32_device( const struct pcap_interface *unix_dev, const char *source, const char *adapter_name ) { struct pcap_interface *ret; @@ -325,8 +340,7 @@ static struct pcap_interface *build_win32_device( const struct pcap_interface *u if (!(ret = calloc( 1, sizeof(*ret) ))) return NULL; if (!(ret->name = build_win32_name( source, adapter_name ))) goto err; if (!(ret->description = build_win32_description( unix_dev ))) goto err; - ret->addresses = build_win32_addresses( unix_dev->addresses ); - ret->flags = unix_dev->flags; + if (!(ret->addresses = alloc_win32_addresses( unix_dev->addresses ))) goto err; return ret; err: @@ -337,6 +351,12 @@ err: return NULL; } +static void build_win32_device( const struct pcap_interface *unix_dev, struct pcap_interface *win32_dev) +{ + build_win32_addresses( unix_dev->addresses, win32_dev->addresses ); + win32_dev->flags = unix_dev->flags; +} + static void add_win32_device( struct pcap_interface **list, struct pcap_interface *dev ) { struct pcap_interface *cur = *list; @@ -348,35 +368,58 @@ static void add_win32_device( struct pcap_interface **list, struct pcap_interfac } } -static int find_all_devices( const char *source, struct pcap_interface **devs, char *errbuf ) +static struct pcap_interface *alloc_win32_devs( const char *source, struct pcap_interface *unix_devs ) { - struct pcap_interface *unix_devs, *win32_devs = NULL, *cur, *dev; + struct pcap_interface *ret = NULL, *cur, *dev; IP_ADAPTER_ADDRESSES *ptr, *adapters = get_adapters(); - struct findalldevs_params params = { &unix_devs, errbuf }; - int ret; if (!adapters) + return NULL; + + cur = unix_devs; + while (cur) + { + if ((ptr = find_adapter( adapters, cur->name )) && (dev = alloc_win32_device( cur, source, ptr->AdapterName ))) + { + add_win32_device( &ret, dev ); + } + cur = cur->next; + } + + free( adapters ); + + return ret; +} + +static int find_all_devices( const char *source, struct pcap_interface **devs, char *errbuf ) +{ + struct pcap_interface *unix_devs, *win32_devs = NULL, *unix_cur, *win32_cur; + struct findalldevs_params params = { &unix_devs, errbuf }; + int ret = PCAP_CALL( findalldevs, ¶ms ); + + if (ret) { + PCAP_CALL( freealldevs, unix_devs ); + return ret; + } + + if (!(win32_devs = alloc_win32_devs( source, unix_devs ))) { if (errbuf) sprintf( errbuf, "Out of memory." ); + PCAP_CALL( freealldevs, unix_devs ); return -1; } - if (!(ret = PCAP_CALL( findalldevs, ¶ms ))) + unix_cur = unix_devs; + win32_cur = win32_devs; + while (unix_cur && win32_cur) { - cur = unix_devs; - while (cur) - { - if ((ptr = find_adapter( adapters, cur->name )) && (dev = build_win32_device( cur, source, ptr->AdapterName ))) - { - add_win32_device( &win32_devs, dev ); - } - cur = cur->next; - } - *devs = win32_devs; - PCAP_CALL( freealldevs, unix_devs ); + build_win32_device( unix_cur, win32_cur ); + unix_cur = unix_cur->next; + win32_cur = win32_cur->next; } + *devs = win32_devs; + PCAP_CALL( freealldevs, unix_devs ); - free( adapters ); return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1015
From: Marc-Aurel Zent <marc_aurel(a)me.com> --- dlls/wpcap/unixlib.c | 83 +++++++++++++++++++++++++++++++++++++++--- dlls/wpcap/unixlib.h | 11 +++--- dlls/wpcap/wpcap.c | 85 ++++++++++++-------------------------------- 3 files changed, 107 insertions(+), 72 deletions(-) diff --git a/dlls/wpcap/unixlib.c b/dlls/wpcap/unixlib.c index ebe37ca5909..abae21994bc 100644 --- a/dlls/wpcap/unixlib.c +++ b/dlls/wpcap/unixlib.c @@ -141,14 +141,89 @@ static NTSTATUS wrap_dump_open( void *args ) return STATUS_SUCCESS; } +static int dup_sockaddr( const struct sockaddr *unix_addr, struct sockaddr_storage *win32_addr ) +{ + switch (unix_addr->sa_family) + { + case AF_INET: + { + struct sockaddr_in *src = (struct sockaddr_in *)unix_addr; + SOCKADDR_IN *dst = (SOCKADDR_IN *)win32_addr; + dst->sin_family = WS_AF_INET; + dst->sin_port = (USHORT)src->sin_port; + memcpy(&dst->sin_addr, &src->sin_addr, sizeof(IN_ADDR)); + return 0; + } + case AF_INET6: + { + struct sockaddr_in6 *src = (struct sockaddr_in6 *)unix_addr; + SOCKADDR_IN6 *dst = (SOCKADDR_IN6 *)win32_addr; + dst->sin6_family = WS_AF_INET6; + dst->sin6_port = (USHORT)src->sin6_port; + dst->sin6_flowinfo = (ULONG)src->sin6_flowinfo; + memcpy(&dst->sin6_addr, &src->sin6_addr, sizeof(IN6_ADDR)); + dst->sin6_scope_id = (ULONG)src->sin6_scope_id; + return 0; + } + default: + FIXME( "unix address family %u not supported\n", unix_addr->sa_family ); + return -1; + } +} + +static int build_win32_address( const struct pcap_addr *unix_addr, struct pcap_address *win32_addr ) +{ + if (unix_addr->addr && win32_addr->addr && dup_sockaddr( unix_addr->addr, win32_addr->addr )) return -1; + if (unix_addr->netmask && win32_addr->netmask && dup_sockaddr( unix_addr->netmask, win32_addr->netmask )) return -1; + if (unix_addr->broadaddr && win32_addr->broadaddr && dup_sockaddr( unix_addr->broadaddr, win32_addr->broadaddr )) return -1; + if (unix_addr->dstaddr && win32_addr->dstaddr && dup_sockaddr( unix_addr->dstaddr, win32_addr->dstaddr )) return -1; + return 0; +} + +static void build_win32_addresses( struct pcap_addr *unix_addrs, struct pcap_address *win32_addrs ) +{ + while (unix_addrs && win32_addrs) + { + if (!build_win32_address( unix_addrs, win32_addrs )) + win32_addrs = win32_addrs->next; + unix_addrs = unix_addrs->next; + } +} + +static void build_win32_device( const pcap_if_t *unix_dev, struct pcap_interface *win32_dev) +{ + build_win32_addresses( unix_dev->addresses, win32_dev->addresses ); + win32_dev->flags = unix_dev->flags; +} + static NTSTATUS wrap_findalldevs( void *args ) { const struct findalldevs_params *params = args; + pcap_if_t **unix_devs = (pcap_if_t **)params->unix_devs; + pcap_if_t *unix_cur; + struct pcap_interface **win32_devs = params->win32_devs; + struct pcap_interface *win32_cur; int ret; - ret = pcap_findalldevs( (pcap_if_t **)params->devs, params->errbuf ); - if (params->devs && !*params->devs) - ERR_(winediag)( "Failed to access raw network (pcap), this requires special permissions.\n" ); - return ret; + + if (!*win32_devs) + { + ret = pcap_findalldevs( unix_devs, params->errbuf ); + if ((unix_devs && !*unix_devs) || ret == PCAP_ERROR_PERM_DENIED) + ERR_(winediag)( "Failed to access raw network (pcap), this requires special permissions.\n" ); + return ret; + } + + unix_cur = *unix_devs; + win32_cur = *win32_devs; + while (unix_cur && win32_cur) + { + build_win32_device( unix_cur, win32_cur ); + unix_cur = unix_cur->next; + win32_cur = win32_cur->next; + } + pcap_freealldevs( *unix_devs ); + + return STATUS_SUCCESS; } static NTSTATUS wrap_free_datalinks( void *args ) diff --git a/dlls/wpcap/unixlib.h b/dlls/wpcap/unixlib.h index 6b815379e7f..2bf1cda3a55 100644 --- a/dlls/wpcap/unixlib.h +++ b/dlls/wpcap/unixlib.h @@ -21,10 +21,10 @@ struct pcap_address { struct pcap_address *next; - SOCKADDR_STORAGE *addr; - SOCKADDR_STORAGE *netmask; - SOCKADDR_STORAGE *broadaddr; - SOCKADDR_STORAGE *dstaddr; + struct sockaddr_storage *addr; + struct sockaddr_storage *netmask; + struct sockaddr_storage *broadaddr; + struct sockaddr_storage *dstaddr; }; struct pcap_interface @@ -103,7 +103,8 @@ struct dump_open_params struct findalldevs_params { - struct pcap_interface **devs; + struct pcap_interface **unix_devs; + struct pcap_interface **win32_devs; char *errbuf; }; diff --git a/dlls/wpcap/wpcap.c b/dlls/wpcap/wpcap.c index 0863849f7ae..3598c129ffb 100644 --- a/dlls/wpcap/wpcap.c +++ b/dlls/wpcap/wpcap.c @@ -238,36 +238,6 @@ static char *build_win32_description( const struct pcap_interface *unix_dev ) return ret; } -static int dup_sockaddr( const struct sockaddr_storage *unix_addr, struct sockaddr_storage *win32_addr ) -{ - switch (unix_addr->ss_family) - { - case AF_INET: - { - struct sockaddr_in *src = (struct sockaddr_in *)unix_addr; - struct sockaddr_in *dst = (struct sockaddr_in *)win32_addr; - dst->sin_family = src->sin_family; - dst->sin_port = src->sin_port; - dst->sin_addr = src->sin_addr; - return 0; - } - case AF_INET6: - { - struct sockaddr_in6 *src = (struct sockaddr_in6 *)unix_addr; - struct sockaddr_in6 *dst = (struct sockaddr_in6 *)win32_addr; - dst->sin6_family = src->sin6_family; - dst->sin6_port = src->sin6_port; - dst->sin6_flowinfo = src->sin6_flowinfo; - dst->sin6_addr = src->sin6_addr; - dst->sin6_scope_id = src->sin6_scope_id; - return 0; - } - default: - FIXME( "address family %u not supported\n", unix_addr->ss_family ); - return -1; - } -} - static struct pcap_address *alloc_win32_address( const struct pcap_address *src ) { struct pcap_address *dst; @@ -288,15 +258,6 @@ err: return NULL; } -static int build_win32_address( const struct pcap_address *unix_addr, struct pcap_address *win32_addr ) -{ - if (unix_addr->addr && dup_sockaddr( unix_addr->addr, win32_addr->addr )) return -1; - if (unix_addr->netmask && dup_sockaddr( unix_addr->netmask, win32_addr->netmask )) return -1; - if (unix_addr->broadaddr && dup_sockaddr( unix_addr->broadaddr, win32_addr->broadaddr )) return -1; - if (unix_addr->dstaddr && dup_sockaddr( unix_addr->dstaddr, win32_addr->dstaddr )) return -1; - return 0; -} - static void add_win32_address( struct pcap_address **list, struct pcap_address *addr ) { struct pcap_address *cur = *list; @@ -320,16 +281,19 @@ static struct pcap_address *alloc_win32_addresses( struct pcap_address *addrs ) return ret; } -static void build_win32_addresses( struct pcap_address *unix_addrs, struct pcap_address *win32_addrs ) +static int trim_win32_addresses( struct pcap_address *addrs ) { - while (unix_addrs && win32_addrs) + struct pcap_address *cur = addrs, *last = NULL; + while (cur) { - if (!build_win32_address( unix_addrs, win32_addrs )) - win32_addrs = win32_addrs->next; - unix_addrs = unix_addrs->next; + if (!cur->addr->ss_family) break; + last = cur; + cur = cur->next; } - - free_addresses( win32_addrs ); /* addresses whose socket family could not be mapped */ + free_addresses(cur); + if (last) last->next = NULL; + else return -1; + return 0; } static struct pcap_interface *alloc_win32_device( const struct pcap_interface *unix_dev, const char *source, @@ -351,12 +315,6 @@ err: return NULL; } -static void build_win32_device( const struct pcap_interface *unix_dev, struct pcap_interface *win32_dev) -{ - build_win32_addresses( unix_dev->addresses, win32_dev->addresses ); - win32_dev->flags = unix_dev->flags; -} - static void add_win32_device( struct pcap_interface **list, struct pcap_interface *dev ) { struct pcap_interface *cur = *list; @@ -393,8 +351,8 @@ static struct pcap_interface *alloc_win32_devs( const char *source, struct pcap static int find_all_devices( const char *source, struct pcap_interface **devs, char *errbuf ) { - struct pcap_interface *unix_devs, *win32_devs = NULL, *unix_cur, *win32_cur; - struct findalldevs_params params = { &unix_devs, errbuf }; + struct pcap_interface *unix_devs, *win32_devs = NULL, *cur; + struct findalldevs_params params = { &unix_devs, &win32_devs, errbuf }; int ret = PCAP_CALL( findalldevs, ¶ms ); if (ret) { @@ -406,20 +364,21 @@ static int find_all_devices( const char *source, struct pcap_interface **devs, c { if (errbuf) sprintf( errbuf, "Out of memory." ); PCAP_CALL( freealldevs, unix_devs ); - return -1; + return -1; /* PCAP_ERROR, generic error code */ } - unix_cur = unix_devs; - win32_cur = win32_devs; - while (unix_cur && win32_cur) + ret = PCAP_CALL( findalldevs, ¶ms ); + + /* free the pre-allocated socket addresses which could not be converted from their + * Unix to win32 representation */ + cur = win32_devs; + while (cur) { - build_win32_device( unix_cur, win32_cur ); - unix_cur = unix_cur->next; - win32_cur = win32_cur->next; + if (trim_win32_addresses(cur->addresses)) cur->addresses = NULL; + cur = cur->next; } - *devs = win32_devs; - PCAP_CALL( freealldevs, unix_devs ); + *devs = win32_devs; return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1015
On Wed Oct 26 20:59:47 2022 +0000, Hans Leidekker wrote:
You shouldn't need another Unix call. The idea would be to pass a win32 adapter -> unix device mapping to findalldevs(). You'd call it first to get the needed size for the pcap_interface array, then allocate memory and then call it again to retrieve the array. Thanks for looking into it!
I removed the additional Unix call and am calling findalldevs twice as you suggested, but am storing the name mapping indirectly in the device allocation on the PE side, which gets passed in again. Ended up being a larger change in the end, but I believe all cases should now be handled gracefully. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_12278
participants (2)
-
Marc-Aurel Zent -
Marc-Aurel Zent (＠marzent)