Re: [Openvpn-devel] [PATCH] get_default_gateway() HWADDR overhaul

2024-01-01 Thread Arne Schwabe

Am 01.01.24 um 10:27 schrieb Gert Doering:

commit f13331005d5a7 (gerrit/454) most painfully works around the limitations
of the SIOCGIFCONF API, with struct member access on an unaligned buffer,
possibly overrunning sockaddr structures, etc. - and the result still did
not work on OpenSolaris and OpenBSD (no AF_LINK in the returned elements).

Reading through OpenBSD "ifconfig" source, I found getifaddrs(3), which
is exactly what we want here - it works on FreeBSD, NetBSD, OpenBSD and
MacOS, and all returned pointers are properly aligned, so the code gets
shorter, easier to read, and UBSAN is still happy.

OpenSolaris does have getifaddrs(3), but (surprise) it does not work, as
in "it does not return AF_LINK addresses".  It does have SIOCGIFHWADDR,
instead, and "man if_tcp" claims "should behave in a manner compatible
with Linux" - so TARGET_SOLARIS gets a copy of the Linux code now (works).




-if (!strncmp(ifr.ifr_name, rgi->iface, IFNAMSIZ))


Mini nitpick from clang-tidy:

Clang-Tidy: No header providing "strncmp" is directly included.

Adding string.h would could be done if we want a v2 of the patch

Tested also on macOS.

Acked-By: Arne Schwabe 



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] get_default_gateway() HWADDR overhaul

2024-01-01 Thread Gert Doering
commit f13331005d5a7 (gerrit/454) most painfully works around the limitations
of the SIOCGIFCONF API, with struct member access on an unaligned buffer,
possibly overrunning sockaddr structures, etc. - and the result still did
not work on OpenSolaris and OpenBSD (no AF_LINK in the returned elements).

Reading through OpenBSD "ifconfig" source, I found getifaddrs(3), which
is exactly what we want here - it works on FreeBSD, NetBSD, OpenBSD and
MacOS, and all returned pointers are properly aligned, so the code gets
shorter, easier to read, and UBSAN is still happy.

OpenSolaris does have getifaddrs(3), but (surprise) it does not work, as
in "it does not return AF_LINK addresses".  It does have SIOCGIFHWADDR,
instead, and "man if_tcp" claims "should behave in a manner compatible
with Linux" - so TARGET_SOLARIS gets a copy of the Linux code now (works).

Signed-off-by: Gert Doering 
---
 src/openvpn/route.c | 97 +++--
 1 file changed, 32 insertions(+), 65 deletions(-)

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 0e6667f7..f75199c6 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -3445,6 +3445,9 @@ get_default_gateway_ipv6(struct route_ipv6_gateway_info 
*rgi6,
 #include 
 #include 
 #include 
+#if !defined(TARGET_SOLARIS)
+#include 
+#endif
 
 struct rtmsg {
 struct rt_msghdr m_rtm;
@@ -3641,19 +3644,11 @@ get_default_gateway(struct route_gateway_info *rgi, 
openvpn_net_ctx_t *ctx)
 rgi->flags |= RGI_NETMASK_DEFINED;
 }
 
-#if !defined(TARGET_SOLARIS)
-/* Illumos/Solaris does not provide AF_LINK entries when calling the
- * SIOCGIFCONF API, so there is little sense to trying to figure out a
- * MAC address from an API that does not provide that information */
-
 /* try to read MAC addr associated with interface that owns default 
gateway */
 if (rgi->flags & RGI_IFACE_DEFINED)
 {
-struct ifconf ifc;
-const int bufsize = 4096;
-char *buffer;
-
-buffer = (char *) gc_malloc(bufsize, true, );
+#if defined(TARGET_SOLARIS)
+/* OpenSolaris has a getifaddr() call, but it does not return AF_LINK 
*/
 sockfd = socket(AF_INET, SOCK_DGRAM, 0);
 if (sockfd < 0)
 {
@@ -3661,71 +3656,43 @@ get_default_gateway(struct route_gateway_info *rgi, 
openvpn_net_ctx_t *ctx)
 goto done;
 }
 
-ifc.ifc_len = bufsize;
-ifc.ifc_buf = buffer;
+struct ifreq ifreq = { 0 };
+
+/* now get the hardware address. */
+strncpynt(ifreq.ifr_name, rgi->iface, sizeof(ifreq.ifr_name));
+if (ioctl(sockfd, SIOCGIFHWADDR, ) < 0)
+{
+msg(M_WARN, "GDG: SIOCGIFHWADDR(%s) failed", ifreq.ifr_name);
+}
+else
+{
+memcpy(rgi->hwaddr, _addr.sa_data, 6);
+rgi->flags |= RGI_HWADDR_DEFINED;
+}
+#else  /* if defined(TARGET_SOLARIS) */
+struct ifaddrs *ifap, *ifa;
 
-if (ioctl(sockfd, SIOCGIFCONF, (char *)) < 0)
+if (getifaddrs() != 0)
 {
-msg(M_WARN, "GDG: ioctl #2 failed");
+msg(M_WARN|M_ERRNO, "GDG: getifaddrs() failed");
 goto done;
 }
-close(sockfd);
-sockfd = -1;
 
-for (cp = buffer; cp <= buffer + ifc.ifc_len - sizeof(struct ifreq); )
+for (ifa = ifap; ifa; ifa = ifa->ifa_next)
 {
-struct ifreq ifr = { 0 };
-/* this is not always using an 8 byte alignment that struct ifr
- * requires. Need to memcpy() to a strict ifr to force 8-byte
- * alignment required for member access */
-memcpy(, cp, sizeof(struct ifreq));
-const size_t len = sizeof(ifr.ifr_name) + 
max(sizeof(ifr.ifr_addr), ifr.ifr_addr.sa_len);
-
-if (!ifr.ifr_addr.sa_family)
-{
-break;
-}
-if (!strncmp(ifr.ifr_name, rgi->iface, IFNAMSIZ))
+if (ifa->ifa_addr != NULL
+&& ifa->ifa_addr->sa_family == AF_LINK
+&& !strncmp(ifa->ifa_name, rgi->iface, IFNAMSIZ) )
 {
-if (ifr.ifr_addr.sa_family == AF_LINK)
-{
-/* This is a confusing member access on multiple levels.
- *
- * struct sockaddr_dl is 20 bytes (on macOS and NetBSD,
- * larger on other BSDs) in size and has
- * 12 bytes space for the Ethernet interface name
- * (max 16 bytes) and  hw address (6 bytes)
- *
- * So if the interface name is more than 6 byte, the
- * location of hwaddr extends beyond the struct.
- *
- * This struct is embedded into ifreq that has
- * 16 bytes for a sockaddr and also expects this
- * struct to potentially extend beyond the