[PATCH 0/1] MR1015: wpcap: use UNIX 98/BSD 4.4 sockaddr header
Interpreting the fist two bytes of the `sockaddr` struct as `sa_family` produces incorrect behaviour on macOS and (presumbly) BSD. This change is however still backwards compatible with Linux since `sa_family` is of effective type `uint8_t` and `sa_len` is currently disregarded. 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) */ }; ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015
From: Marc-Aurel Zent <marc_aurel(a)me.com> Interpreting the fist two bytes of the sockaddr struct as sa_family produces incorrect behaviour on macOS and (presumbly) BSD. This change is however still backwards compatible with Linux since sa_family_t is uint8_t and sa_len is currently disregarded. --- dlls/wpcap/unixlib.h | 3 ++- dlls/wpcap/wpcap.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/dlls/wpcap/unixlib.h b/dlls/wpcap/unixlib.h index 8cd64ef0210..41a7fa7568f 100644 --- a/dlls/wpcap/unixlib.h +++ b/dlls/wpcap/unixlib.h @@ -19,7 +19,8 @@ struct sockaddr_hdr { - unsigned short sa_family; + unsigned char sa_len; + unsigned char sa_family; }; struct pcap_address diff --git a/dlls/wpcap/wpcap.c b/dlls/wpcap/wpcap.c index df35954bfa7..6a2c8e18f82 100644 --- a/dlls/wpcap/wpcap.c +++ b/dlls/wpcap/wpcap.c @@ -248,7 +248,7 @@ static struct sockaddr_hdr *dup_sockaddr( const struct sockaddr_hdr *addr ) { struct sockaddr_in *dst, *src = (struct sockaddr_in *)addr; if (!(dst = calloc( 1, sizeof(*dst) ))) return NULL; - dst->sin_family = src->sin_family; + dst->sin_family = AF_INET; dst->sin_port = src->sin_port; dst->sin_addr = src->sin_addr; ret = (struct sockaddr_hdr *)dst; @@ -258,7 +258,7 @@ static struct sockaddr_hdr *dup_sockaddr( const struct sockaddr_hdr *addr ) { struct sockaddr_in6 *dst, *src = (struct sockaddr_in6 *)addr; if (!(dst = malloc( sizeof(*dst) ))) return NULL; - dst->sin6_family = src->sin6_family; + dst->sin6_family = AF_INET6; dst->sin6_port = src->sin6_port; dst->sin6_flowinfo = src->sin6_flowinfo; dst->sin6_addr = src->sin6_addr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1015
Adding @hans as reviewer. Hans, you added that struct and the helper function afaict. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10074
This change is however still backwards compatible with Linux since `sa_family` is of effective type `uint8_t` and `sa_len` is currently disregarded.
This is only true on big-endian systems. Note that `sa_family` is in host endian, unlike other sockaddr fields. On little-endian systems, your change means that `sa_len` will take the place of the *lower* 8 bits of former `sa_family`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10114
Jinoh Kang (@iamahuman) commented about dlls/wpcap/wpcap.c:
{ struct sockaddr_in *dst, *src = (struct sockaddr_in *)addr; if (!(dst = calloc( 1, sizeof(*dst) ))) return NULL; - dst->sin_family = src->sin_family; + dst->sin_family = AF_INET;
We should declare separate structs for Unix pcap and Win32 (w)pcap instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10119
Jinoh Kang (@iamahuman) commented about dlls/wpcap/unixlib.h:
struct sockaddr_hdr { - unsigned short sa_family; + unsigned char sa_len; + unsigned char sa_family;
We should check BSD variants instead, like this: ``` #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) || defined(__APPLE__) ``` Another option would be to check the existence of `sa_len` in `struct sockaddr` via autoconf, but glibc doesn't seem to do that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10118
Thanks for your contribution! I've left a few comments on your PR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10120
On Mon Oct 10 13:45:33 2022 +0000, Jinoh Kang wrote:
This change is however still backwards compatible with Linux since `sa_family` is of effective type `uint8_t` and `sa_len` is currently disregarded. This is only true on big-endian systems. Note that `sa_family` is in host endian, unlike other sockaddr fields. On little-endian systems, your change means that `sa_len` will take the place of the *lower* 8 bits of former `sa_family`. Endian issues aside, I think it's better not to be too clever on bit representations on general.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10121
On Mon Oct 10 13:45:33 2022 +0000, Jinoh Kang wrote:
Endian issues aside, I think it's better not to be too clever on bit representations on general. Ah yeah are right indeed, at that point in time it just made sense in my head and I believe I tested it once on Linux but some human error must have happened there, I was trying to go for a minimal change…
This indeed is a major breaking change in the sockaddr struct that needs to be treated differently in a clear way. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10126
On Mon Oct 10 13:42:53 2022 +0000, Jinoh Kang wrote:
We should check BSD variants instead, like this: ``` #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) || defined(__APPLE__) ``` Another option would be to check the existence of `sa_len` in `struct sockaddr` via autoconf, but glibc doesn't seem to do that. This does seem like a good option going forward but implies that there are only 2 types of sockaddr structs host libpcap might return (which might very well be true though for all operating systems wine is supposed to work on)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10132
On Mon Oct 10 13:42:54 2022 +0000, Jinoh Kang wrote:
We should declare separate structs for Unix pcap and Win32 (w)pcap instead. I had an original version to fix this that moved all the build_win32_device logic and helper functions to its own unixlib file, where you have access to the host pcap structs and the Win32 ones. This has the advantage of not indiscriminately casting everything from host libpcap to Win32 structs and resulted in functions like
`static SOCKADDR *dup_sockaddr( const struct sockaddr *addr )` or `static struct pcap_interface *build_win32_device( const pcap_if_t *unix_dev, const char *source, const char *adapter_name )` Was a bit ugly though since SOCKET (and something else which I can’t remember right now) had to be undefined (even with the WS prefix) and it was a rather large change in the end, which seemed unnecessary when Unix and Win32 structs seemed to happily align except for the sockaddr, so I tried to come up with something smarter, which was not the best idea… -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10134
On Mon Oct 10 15:50:45 2022 +0000, Jinoh Kang wrote:
Thanks for your contribution! I've left a few comments on your PR. Thanks looking over it, in general which of the two options above would you think would be best to resolve the underlying issue? (Might be a bit slow to update this PR though since I am currently on vacation)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10135
We should move conversion of the sockaddr structures to the Unix side. That way we can get the definitions from the Unix headers, though we still need to allocate memory for the structures on the PE side. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1015#note_10373
participants (5)
-
André Zwing -
Hans Leidekker (＠hans)
-
Jinoh Kang (＠iamahuman)
-
Marc-Aurel Zent
-
Marc-Aurel Zent (＠marzent)