From: David Kalnischkies Date: Wed, 23 Nov 2022 11:11:26 +0000 (+0100) Subject: Avoid breaking strict aliasing in IP_AS_V{4,6} X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7b95ba4cf9113ae8808b3e4a9425010b607dbaca;p=thirdparty%2Ftvheadend.git Avoid breaking strict aliasing in IP_AS_V{4,6} GCC complains (one example, more in tcp.h): In file included from src/main.c:41: src/tcp.h: In function ‘ip_check_equal_v4’: src/tcp.h:29:31: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing] 29 | #define IP_AS_V4(storage, f) ((struct sockaddr_in *)&(storage))->sin_##f | ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/tcp.h:67:14: note: in expansion of macro ‘IP_AS_V4’ 67 | { return IP_AS_V4(a, addr).s_addr == IP_AS_V4(b, addr).s_addr; } | ^~~~~~~~ storage (a) here is a pointer to sockaddr_storage which is either backed by a sockaddr_in or sockaddr_in6 struct (here, it would be good if it is the former, we decided based on sa_family in ip_check_equal). Referencing it means we have a pointer to a pointer to sockaddr_storage here, which we then cast to a pointer to sockaddr_in. Our so type-punned pointer is then dereferenced breaking strict-aliasing as this pointer as well as our original pointer share a memory location and both could be used to change it even through they are of different types. We can avoid this situation simply by removing the reference as then it is just casting a pointer to a different type (which in this case is legal as storage is really a sockaddr_in). Removing the reference breaks users of the macro who do not feed it a pointer to a sockaddr_storage, so while the warnings were all produced by tcp.h, we end up changing code everywhere else to resolve them – usually by just taking a reference or not dereferencing there. As this is the only instance of (detected) strict-aliasing breakage the Makefile is also changed to no longer build the entirety of tvheadend with strict-aliasing rules disabled. Similar for the disabled stringop warnings which weren't (correctly) triggered in the current code. As a small bonus, this also prevents gcc-11 from tripping with a false positive over the previous tcp change in c0f616e / #1473. --- diff --git a/Makefile b/Makefile index 1f1c2e36d..39748e32d 100644 --- a/Makefile +++ b/Makefile @@ -51,10 +51,7 @@ CFLAGS += -Werror endif CFLAGS += -Wall -Wwrite-strings -Wno-deprecated-declarations CFLAGS += -Wmissing-prototypes -CFLAGS += -fms-extensions -funsigned-char -fno-strict-aliasing -ifeq ($(COMPILER), gcc) -CFLAGS += -Wno-stringop-truncation -Wno-stringop-overflow -endif +CFLAGS += -fms-extensions -funsigned-char CFLAGS += -D_FILE_OFFSET_BITS=64 CFLAGS += -I${BUILDDIR} -I${ROOTDIR}/src -I${ROOTDIR} ifeq ($(CONFIG_ANDROID),yes) diff --git a/src/tcp.c b/src/tcp.c index 6a21bab96..8ff558730 100644 --- a/src/tcp.c +++ b/src/tcp.c @@ -94,7 +94,7 @@ ip_check_is_local_address if (!ifaddr || !ifnetmask) continue; if (ifaddr->ss_family != local->ss_family) continue; if (!any_address && !ip_check_equal(ifaddr, local)) continue; - ret = !!ip_check_in_network_v4(ifaddr, ifnetmask, peer); + ret = !!ip_check_in_network(ifaddr, ifnetmask, peer); if (ret) { if (used_local) memcpy(used_local, ifaddr, sizeof(struct sockaddr)); @@ -1051,9 +1051,9 @@ tcp_default_ip_addr ( struct sockaddr_storage *deflt, int family ) } if (ss.ss_family == AF_INET) - IP_AS_V4(ss, port) = 0; + IP_AS_V4(&ss, port) = 0; else - IP_AS_V6(ss, port) = 0; + IP_AS_V6(&ss, port) = 0; memset(deflt, 0, sizeof(*deflt)); memcpy(deflt, &ss, ss_len); @@ -1092,9 +1092,9 @@ tcp_server_bound ( void *server, struct sockaddr_storage *bound, int family ) if (tcp_default_ip_addr(bound, family) < 0) return -1; if (bound->ss_family == AF_INET) - IP_AS_V4(*bound, port) = port; + IP_AS_V4(bound, port) = port; else - IP_AS_V6(*bound, port) = port; + IP_AS_V6(bound, port) = port; return 0; } diff --git a/src/tcp.h b/src/tcp.h index 726dfaeba..076f585a5 100644 --- a/src/tcp.h +++ b/src/tcp.h @@ -26,8 +26,8 @@ #include #endif -#define IP_AS_V4(storage, f) ((struct sockaddr_in *)&(storage))->sin_##f -#define IP_AS_V6(storage, f) ((struct sockaddr_in6 *)&(storage))->sin6_##f +#define IP_AS_V4(storage, f) ((struct sockaddr_in *)(storage))->sin_##f +#define IP_AS_V6(storage, f) ((struct sockaddr_in6 *)(storage))->sin6_##f #define IP_IN_ADDR(storage) \ ((storage).ss_family == AF_INET6 ? \ &((struct sockaddr_in6 *)&(storage))->sin6_addr : \ diff --git a/src/tvhlog.c b/src/tvhlog.c index ec0eba874..82f565e2d 100644 --- a/src/tvhlog.c +++ b/src/tvhlog.c @@ -587,7 +587,7 @@ tvhlog_init ( int level, int options, const char *path ) if (rtport > 0) { tvhlog_rtfd = tvh_socket(AF_INET, SOCK_DGRAM, 0); tcp_get_ip_from_str("127.0.0.1", &tvhlog_rtss); - IP_AS_V4(tvhlog_rtss, port) = htons(rtport); + IP_AS_V4(&tvhlog_rtss, port) = htons(rtport); } } #endif diff --git a/src/udp.c b/src/udp.c index b88734011..b0c3c1e5c 100644 --- a/src/udp.c +++ b/src/udp.c @@ -76,15 +76,15 @@ udp_resolve( udp_connection_t *uc, } if (use->ai_family == AF_INET6) { ss->ss_family = AF_INET6; - IP_AS_V6(*ss, port) = htons(port); - memcpy(&IP_AS_V6(*ss, addr), &((struct sockaddr_in6 *)use->ai_addr)->sin6_addr, + IP_AS_V6(ss, port) = htons(port); + memcpy(&IP_AS_V6(ss, addr), &((struct sockaddr_in6 *)use->ai_addr)->sin6_addr, sizeof(struct in6_addr)); - *multicast = !!IN6_IS_ADDR_MULTICAST(&IP_AS_V6(*ss, addr)); + *multicast = !!IN6_IS_ADDR_MULTICAST(&IP_AS_V6(ss, addr)); } else if (use->ai_family == AF_INET) { ss->ss_family = AF_INET; - IP_AS_V4(*ss, port) = htons(port); - IP_AS_V4(*ss, addr) = ((struct sockaddr_in *)use->ai_addr)->sin_addr; - *multicast = !!IN_MULTICAST(ntohl(IP_AS_V4(*ss, addr.s_addr))); + IP_AS_V4(ss, port) = htons(port); + IP_AS_V4(ss, addr) = ((struct sockaddr_in *)use->ai_addr)->sin_addr; + *multicast = !!IN_MULTICAST(ntohl(IP_AS_V4(ss, addr.s_addr))); } freeaddrinfo(ressave); if (ss->ss_family != AF_INET && ss->ss_family != AF_INET6) { @@ -207,9 +207,9 @@ udp_bind ( int subsystem, const char *name, /* Bind useful for receiver subsystem (not for udp streamer) */ if (subsystem != LS_UDP) { if (bind(fd, (struct sockaddr *)&uc->ip, sizeof(struct sockaddr_in))) { - inet_ntop(AF_INET, &IP_AS_V4(uc->ip, addr), buf, sizeof(buf)); + inet_ntop(AF_INET, &IP_AS_V4(&uc->ip, addr), buf, sizeof(buf)); tvherror(subsystem, "%s - cannot bind %s:%hu [e=%s]", - name, buf, ntohs(IP_AS_V4(uc->ip, port)), strerror(errno)); + name, buf, ntohs(IP_AS_V4(&uc->ip, port)), strerror(errno)); goto error; } } @@ -221,7 +221,7 @@ udp_bind ( int subsystem, const char *name, struct ip_mreq_source ms; memset(&ms, 0, sizeof(ms)); - ms.imr_multiaddr = IP_AS_V4(uc->ip, addr); + ms.imr_multiaddr = IP_AS_V4(&uc->ip, addr); /* Note, ip_mreq_source does not support the ifindex parameter, so we have to resolve to the ip of the interface on all platforms. */ @@ -253,7 +253,7 @@ udp_bind ( int subsystem, const char *name, #endif memset(&m, 0, sizeof(m)); - m.imr_multiaddr = IP_AS_V4(uc->ip, addr); + m.imr_multiaddr = IP_AS_V4(&uc->ip, addr); #if !defined(PLATFORM_DARWIN) m.imr_address.s_addr = 0; m.imr_ifindex = ifindex; @@ -280,15 +280,15 @@ udp_bind ( int subsystem, const char *name, /* Bind */ if (bind(fd, (struct sockaddr *)&uc->ip, sizeof(struct sockaddr_in6))) { - inet_ntop(AF_INET6, &IP_AS_V6(uc->ip, addr), buf, sizeof(buf)); + inet_ntop(AF_INET6, &IP_AS_V6(&uc->ip, addr), buf, sizeof(buf)); tvherror(subsystem, "%s - cannot bind %s:%hu [e=%s]", - name, buf, ntohs(IP_AS_V6(uc->ip, port)), strerror(errno)); + name, buf, ntohs(IP_AS_V6(&uc->ip, port)), strerror(errno)); goto error; } if (uc->multicast) { /* Join group */ - m.ipv6mr_multiaddr = IP_AS_V6(uc->ip, addr); + m.ipv6mr_multiaddr = IP_AS_V6(&uc->ip, addr); m.ipv6mr_interface = ifindex; #ifdef SOL_IPV6 if (setsockopt(fd, SOL_IPV6, IPV6_ADD_MEMBERSHIP, &m, sizeof(m))) { diff --git a/src/upnp.c b/src/upnp.c index 8842aab32..f6a3c649f 100644 --- a/src/upnp.c +++ b/src/upnp.c @@ -227,8 +227,8 @@ upnp_server_init(const char *bindaddr) memset(&upnp_ipv4_multicast, 0, sizeof(upnp_ipv4_multicast)); upnp_ipv4_multicast.ss_family = AF_INET; - IP_AS_V4(upnp_ipv4_multicast, port) = htons(1900); - r = inet_pton(AF_INET, "239.255.255.250", &IP_AS_V4(upnp_ipv4_multicast, addr)); + IP_AS_V4(&upnp_ipv4_multicast, port) = htons(1900); + r = inet_pton(AF_INET, "239.255.255.250", &IP_AS_V4(&upnp_ipv4_multicast, addr)); assert(r); tvh_mutex_init(&upnp_lock, NULL);