diff mbox series

[FFmpeg-devel,v3] avformat: enable UDP IPv6 multicast interface selection using localaddr

Message ID AS8P193MB1997CB70F372EDEA4F5F732C8D352@AS8P193MB1997.EURP193.PROD.OUTLOOK.COM
State New
Headers show
Series [FFmpeg-devel,v3] avformat: enable UDP IPv6 multicast interface selection using localaddr | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Ignjatović, Lazar (RS) March 26, 2024, 12:34 p.m. UTC 
avformat: enable UDP IPv6 multicast interface selection using localaddr

localaddr option now properly works with IPv6 addresses. Properly
resolved interface index in places where default 0 interface index is
used (marked with TODO: within udp.c). Adjusted binding for multicast
sockets that are used for reading from the network. Need for this
arises from the fact that link-local multicast addresses need to have a
defined interface for binding to avoid ambiguity between multiple
link-local networks on the same host. Failing to set this option causes
errors on Linux systems for interface and link-local scopes.

For mcast addresses, bind to mcast address is attempted as before.
In case that this fails, which will happen on Windows, socket is bound
to INADDR_ANY/IN6ADDR_ANY_INIT depending on address family. Actual
interface selection is performed using udp_set_multicast_interface to
point to the desired interface for sending.

Utilization of sin6_scope_id field enables usage and adequate resolving
of IPv6 addresses that utilize zone index (e.g. fe80::1%eth2)

Closes: #368

Signed-off-by: Lazar Ignjatovic <Lazar.Ignjatovic@cubic.com>
---
V1 -> V2: SO_BINDTODEVICE -> sin6_scope_id, addressed comments
V2 -> V3: mcast check sin6_scope_id for iface specification
NOTE: Due to comments, this patch is proposed as one of two alternatives
The other alternative uses exclusively %scope format for defining
interfaces. This patch can handle scoped IPv6 link-local addresses


 configure             |  3 ++
 libavformat/ip.c      | 48 ++++++++++++++++++++++++
 libavformat/ip.h      |  6 +++
 libavformat/network.h |  6 +++
 libavformat/udp.c     | 85 +++++++++++++++++++++++++++++++++++++++----
 5 files changed, 140 insertions(+), 8 deletions(-)

--
2.41.0.windows.2


This message has been marked as Public on 03/26/2024 12:34Z.

Comments

Rémi Denis-Courmont March 26, 2024, 6:25 p.m. UTC | #1 
Both patches -1 for same reasons as before and that's unnecessary, 
functionally incorrect and potentially racy, enumeration of local interfaces.
Ignjatović, Lazar (RS) March 27, 2024, 8:53 a.m. UTC | #2 
This message has been marked as Public on 03/27/2024 08:53Z.
On Tuesday, March 26, 2024 7:26 PM Rémi Denis-Courmont wrote:

> Both patches -1 for same reasons as before and that's unnecessary, functionally incorrect and potentially racy, enumeration of local interfaces.

Enumeration of interfaces is done only for IPv4 MCAST_JOIN_SOURCE_GROUP/MCAST_BLOCK_SOURCE within zone index patch.
Adding a separate parameter just for this seems pointless to me. Would removal of this, and reverting back to how it was
( = 0; // default interface in every case), address your concerns?

Lazar Ignjatović
Associate Software Engineer

Cubic Defense
cubic.com
Rémi Denis-Courmont March 27, 2024, 3:31 p.m. UTC | #3 
Le keskiviikkona 27. maaliskuuta 2024, 10.53.25 EET Ignjatović, Lazar (RS) a 
écrit :
> This message has been marked as Public on 03/27/2024 08:53Z.
> 
> On Tuesday, March 26, 2024 7:26 PM Rémi Denis-Courmont wrote:
> > Both patches -1 for same reasons as before and that's unnecessary,
> > functionally incorrect and potentially racy, enumeration of local
> > interfaces.
> Enumeration of interfaces is done only for IPv4
> MCAST_JOIN_SOURCE_GROUP/MCAST_BLOCK_SOURCE within zone index patch. Adding
> a separate parameter just for this seems pointless to me. Would removal of
> this, and reverting back to how it was ( = 0; // default interface in every
> case), address your concerns?

I have mixed feelings about changing how IPv4 works. Sure, it provides better 
consistency, and it is easier to remember an interface name than an IP address 
(especially if it is dynamically assigned). But it will also break backward 
compatibility. Not that I care about this specific hypothetical breakage, but 
somebody else very well might.

Either way, if you change how IPv4 works, you need to do it as a separate 
patch to adding IPv6 support. Avoiding reviewer confusion is just one of 
several reasons.
diff mbox series

Patch

diff --git a/configure b/configure
index e019d1b996..08f35bbd25 100755
--- a/configure
+++ b/configure
@@ -2258,6 +2258,7 @@  HEADERS_LIST="
     valgrind_valgrind_h
     windows_h
     winsock2_h
+    iphlpapi_h
 "

 INTRINSICS_LIST="
@@ -6406,6 +6407,8 @@  if ! disabled network; then
         check_struct winsock2.h "struct sockaddr" sa_len
         check_type ws2tcpip.h "struct sockaddr_in6"
         check_type ws2tcpip.h "struct sockaddr_storage"
+        check_headers iphlpapi.h -liphlpapi && network_extralibs="$network_extralibs -liphlpapi" || disable iphlpapi_h
+        check_func_headers iphlpapi.h GetBestInterfaceEx $network_extralibs
     else
         disable network
     fi
diff --git a/libavformat/ip.c b/libavformat/ip.c
index b2c7ef07e5..821595edc5 100644
--- a/libavformat/ip.c
+++ b/libavformat/ip.c
@@ -18,6 +18,9 @@ 
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */

+#define _DEFAULT_SOURCE
+#define _SVID_SOURCE
+
 #include <string.h>
 #include "ip.h"
 #include "libavutil/avstring.h"
@@ -159,3 +162,48 @@  void ff_ip_reset_filters(IPSourceFilters *filters)
     filters->nb_include_addrs = 0;
     filters->nb_exclude_addrs = 0;
 }
+
+unsigned int ff_ip_resolve_interface_index(struct sockaddr_storage *local_addr)
+{
+#if HAVE_WINSOCK2_H
+#if HAVE_IPHLPAPI_H
+    DWORD retval;
+    unsigned long iface;
+
+    if (local_addr == NULL)
+        return 0;
+
+    retval = GetBestInterfaceEx((struct sockaddr*)local_addr, &iface);
+    if (retval == NO_ERROR)
+        return iface;
+#endif /* HAVE_IPHLPAPI_H */
+    return 0;
+#else /* HAVE_WINSOCK2_H */
+    struct ifaddrs *ifaddr, *ifa;
+    unsigned int iface;
+
+    if (local_addr == NULL)
+        return 0;
+
+#if HAVE_STRUCT_SOCKADDR_IN6
+    if (local_addr->ss_family == AF_INET6) {
+        iface = ((struct sockaddr_in6*)local_addr)->sin6_scope_id;
+        if (iface)
+            return iface;
+    }
+#endif /* HAVE_STRUCT_SOCKADDR_IN6 */
+    if (getifaddrs(&ifaddr) == -1)
+        return 0;
+
+    iface = 0;
+    for (ifa = ifaddr; ifa != NULL; ifa = ifa->ifa_next) {
+        if (ifa->ifa_addr != NULL && compare_addr((struct sockaddr_storage*)ifa->ifa_addr, local_addr) == 0) {
+            iface = if_nametoindex(ifa->ifa_name);
+            break;
+        }
+    }
+
+    freeifaddrs(ifaddr);
+    return iface;
+#endif /* HAVE_WINSOCK2_H */
+}
diff --git a/libavformat/ip.h b/libavformat/ip.h
index b76cdab91c..4085e96f08 100644
--- a/libavformat/ip.h
+++ b/libavformat/ip.h
@@ -69,4 +69,10 @@  int ff_ip_parse_blocks(void *log_ctx, const char *buf, IPSourceFilters *filters)
  */
 void ff_ip_reset_filters(IPSourceFilters *filters);

+/**
+ * Resolves IP address to an associated interface index
+ * @return interface index, 0 as default interface value on error
+ */
+unsigned int ff_ip_resolve_interface_index(struct sockaddr_storage *local_addr);
+
 #endif /* AVFORMAT_IP_H */
diff --git a/libavformat/network.h b/libavformat/network.h
index ca214087fc..2461b651d4 100644
--- a/libavformat/network.h
+++ b/libavformat/network.h
@@ -38,6 +38,10 @@ 
 #include <winsock2.h>
 #include <ws2tcpip.h>

+#if HAVE_IPHLPAPI_H
+#include <iphlpapi.h>
+#endif
+
 #ifndef EPROTONOSUPPORT
 #define EPROTONOSUPPORT WSAEPROTONOSUPPORT
 #endif
@@ -64,6 +68,8 @@  int ff_neterrno(void);
 #include <netinet/in.h>
 #include <netinet/tcp.h>
 #include <netdb.h>
+#include <net/if.h>
+#include <ifaddrs.h>

 #define ff_neterrno() AVERROR(errno)
 #endif /* HAVE_WINSOCK2_H */
diff --git a/libavformat/udp.c b/libavformat/udp.c
index d9514f5026..69ef93bfed 100644
--- a/libavformat/udp.c
+++ b/libavformat/udp.c
@@ -220,8 +220,10 @@  static int udp_join_multicast_group(int sockfd, struct sockaddr *addr,
         struct ipv6_mreq mreq6;

         memcpy(&mreq6.ipv6mr_multiaddr, &(((struct sockaddr_in6 *)addr)->sin6_addr), sizeof(struct in6_addr));
-        //TODO: Interface index should be looked up from local_addr
-        mreq6.ipv6mr_interface = 0;
+        mreq6.ipv6mr_interface = ff_ip_resolve_interface_index((struct sockaddr_storage *)local_addr);
+        if (!mreq6.ipv6mr_interface)
+            mreq6.ipv6mr_interface = ((struct sockaddr_in6 *)addr)->sin6_scope_id;
+
         if (setsockopt(sockfd, IPPROTO_IPV6, IPV6_ADD_MEMBERSHIP, &mreq6, sizeof(mreq6)) < 0) {
             ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IPV6_ADD_MEMBERSHIP)");
             return ff_neterrno();
@@ -231,6 +233,41 @@  static int udp_join_multicast_group(int sockfd, struct sockaddr *addr,
     return 0;
 }

+static int udp_set_multicast_interface(int sockfd, struct sockaddr *addr,
+                                    struct sockaddr *local_addr, void *logctx)
+{
+#ifdef IP_MULTICAST_IF
+    if (addr->sa_family == AF_INET) {
+        struct ip_mreq mreq;
+
+        mreq.imr_multiaddr.s_addr = ((struct sockaddr_in *)addr)->sin_addr.s_addr;
+        if (local_addr)
+            mreq.imr_interface = ((struct sockaddr_in *)local_addr)->sin_addr;
+        else
+            mreq.imr_interface.s_addr = INADDR_ANY;
+
+        if (setsockopt(sockfd, IPPROTO_IP, IP_MULTICAST_IF, (const void *)&mreq, sizeof(mreq)) < 0) {
+            ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IP_MULTICAST_IF)");
+            return ff_neterrno();
+        }
+    }
+#endif
+#if defined(IPV6_MULTICAST_IF) && defined(IPPROTO_IPV6)
+    if (addr->sa_family == AF_INET6) {
+        unsigned int iface;
+        iface = ff_ip_resolve_interface_index((struct sockaddr_storage *)local_addr);
+        if (!iface)
+            iface= ((struct sockaddr_in6 *)addr)->sin6_scope_id;
+
+        if (setsockopt(sockfd, IPPROTO_IPV6, IPV6_MULTICAST_IF, &iface, sizeof(unsigned int)) < 0) {
+            ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IPV6_MULTICAST_IF)");
+            return ff_neterrno();
+        }
+    }
+#endif
+    return 0;
+}
+
 static int udp_leave_multicast_group(int sockfd, struct sockaddr *addr,
                                      struct sockaddr *local_addr, void *logctx)
 {
@@ -254,8 +291,10 @@  static int udp_leave_multicast_group(int sockfd, struct sockaddr *addr,
         struct ipv6_mreq mreq6;

         memcpy(&mreq6.ipv6mr_multiaddr, &(((struct sockaddr_in6 *)addr)->sin6_addr), sizeof(struct in6_addr));
-        //TODO: Interface index should be looked up from local_addr
-        mreq6.ipv6mr_interface = 0;
+        mreq6.ipv6mr_interface = ff_ip_resolve_interface_index((struct sockaddr_storage *)local_addr);
+        if (!mreq6.ipv6mr_interface)
+            mreq6.ipv6mr_interface = ((struct sockaddr_in6 *)addr)->sin6_scope_id;
+
         if (setsockopt(sockfd, IPPROTO_IPV6, IPV6_DROP_MEMBERSHIP, &mreq6, sizeof(mreq6)) < 0) {
             ff_log_net_error(logctx, AV_LOG_ERROR, "setsockopt(IPV6_DROP_MEMBERSHIP)");
             return -1;
@@ -282,8 +321,7 @@  static int udp_set_multicast_sources(URLContext *h,
             struct group_source_req mreqs;
             int level = addr->sa_family == AF_INET ? IPPROTO_IP : IPPROTO_IPV6;

-            //TODO: Interface index should be looked up from local_addr
-            mreqs.gsr_interface = 0;
+            mreqs.gsr_interface = ff_ip_resolve_interface_index(local_addr);
             memcpy(&mreqs.gsr_group, addr, addr_len);
             memcpy(&mreqs.gsr_source, &sources[i], sizeof(*sources));

@@ -839,10 +877,35 @@  static int udp_open(URLContext *h, const char *uri, int flags)
      * port. This fails on windows. This makes sending to the same address
      * using sendto() fail, so only do it if we're opened in read-only mode. */
     if (s->is_multicast && (h->flags & AVIO_FLAG_READ)) {
+#if HAVE_STRUCT_SOCKADDR_IN6 && !HAVE_WINSOCK2_H
+        if (s->dest_addr.ss_family == AF_INET6) {
+            unsigned int iface;
+            struct sockaddr_in6 *addr;
+            addr = (struct sockaddr_in6 *)&s->dest_addr;
+            iface = ff_ip_resolve_interface_index(&s->local_addr_storage);
+            if (iface)
+                addr->sin6_scope_id = iface;
+        }
+#endif
         bind_ret = bind(udp_fd,(struct sockaddr *)&s->dest_addr, len);
     }
-    /* bind to the local address if not multicast or if the multicast
-     * bind failed */
+
+    /* bind to ADDR_ANY if the multicast bind failed */
+    if (s->is_multicast && bind_ret < 0) {
+        struct addrinfo *res;
+
+        if (s->dest_addr.ss_family == AF_INET)
+            res = ff_ip_resolve_host(h, "0.0.0.0", 0, SOCK_DGRAM, AF_UNSPEC, 0);
+        else if (s->dest_addr.ss_family == AF_INET6)
+            res = ff_ip_resolve_host(h, "::", 0, SOCK_DGRAM, AF_UNSPEC, 0);
+
+        if (res && res->ai_addr) {
+            bind_ret = bind(udp_fd, res->ai_addr, res->ai_addrlen);
+        }
+
+        freeaddrinfo(res);
+    }
+
     /* the bind is needed to give a port to the socket now */
     if (bind_ret < 0 && bind(udp_fd,(struct sockaddr *)&my_addr, len) < 0) {
         ff_log_net_error(h, AV_LOG_ERROR, "bind failed");
@@ -855,6 +918,12 @@  static int udp_open(URLContext *h, const char *uri, int flags)
     s->local_port = udp_port(&my_addr, len);

     if (s->is_multicast) {
+        if ((ret = udp_set_multicast_interface(udp_fd,
+                                        (struct sockaddr *)&s->dest_addr,
+                                        (struct sockaddr *)&s->local_addr_storage,
+                                        h)) < 0)
+            goto fail;
+
         if (h->flags & AVIO_FLAG_WRITE) {
             /* output */
             if ((ret = udp_set_multicast_ttl(udp_fd, s->ttl, (struct sockaddr *)&s->dest_addr, h)) < 0)