Merge pull request networkupstools#2916 from jimklimov/common-inet_ntopX

Move `inet_ntopXX()` methods to `common.{c,h}`

Author: jimklimov

NEWS.adoc

@@ -52,6 +52,15 @@ https://github.com/networkupstools/nut/milestone/9
    * Common code hardening: added sanity-checking for dynamically constructed
      or selected formatting strings with variable-argument list methods
      (typically used with log printing, `dstate` setting, etc.) [#2450]
+   * Refactored repetitive implementations of `inet_ntopSS()` (nee
+     `inet_ntopW()` in `upsd.c`) and `inet_ntopAI()` methods into `common.c`,
+     so now they can be re-used or expanded more easily. [#2916]
+
+ - `upsd` updates:
+   * Fixed two bugs about printing the "further (ignored) addresses resolved
+     for this name": the way to extract IP address string was not portable
+     and misfired on some platforms, and the way to print had a theoretical
+     potential for buffer overflow. [#2915]
 
 
 Release notes for NUT 2.8.3 - what's new since 2.8.2

clients/upsclient.c

@@ -20,6 +20,8 @@
    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 */
 
+#define NUT_WANT_INET_NTOP_XX	1
+
 #include "config.h"	/* safe because it doesn't contain prototypes */
 #include "nut_platform.h"
 
@@ -1162,34 +1164,9 @@ int upscli_tryconnect(UPSCONN_t *ups, const char *host, uint16_t port, int flags
 			    ups->upserror == UPSCLI_ERR_CONNFAILURE &&
 			    ups->syserrno == ETIMEDOUT
 			) {
-				/* https://stackoverflow.com/a/29147085/4715872
-				 * obviously INET6_ADDRSTRLEN is expected to be larger
-				 * than INET_ADDRSTRLEN, but this may be required in case
-				 * if for some unexpected reason IPv6 is not supported, and
-				 * INET6_ADDRSTRLEN is defined as 0
-				 * but this is not very likely and I am aware of no cases of
-				 * this in practice (editor)
-				 */
-				char	addrstr[(INET6_ADDRSTRLEN > INET_ADDRSTRLEN ? INET6_ADDRSTRLEN : INET_ADDRSTRLEN) + 1];
-				addrstr[0] = '\0';
-				switch (ai->ai_family) {
-					case AF_INET: {
-						struct sockaddr_in addr_in;
-						memcpy(&addr_in, ai->ai_addr, sizeof(addr_in));
-						inet_ntop(AF_INET, &(addr_in.sin_addr), addrstr, INET_ADDRSTRLEN);
-						break;
-					}
-					case AF_INET6: {
-						struct sockaddr_in6 addr_in6;
-						memcpy(&addr_in6, ai->ai_addr, sizeof(addr_in6));
-						inet_ntop(AF_INET6, &(addr_in6.sin6_addr), addrstr, INET6_ADDRSTRLEN);
-						break;
-					}
-					default:
-						break;
-				}
+				const char	*addrstr = inet_ntopAI(ai);
 				upslogx(LOG_WARNING, "%s: Connection to host timed out: '%s'",
-					__func__, *addrstr ? addrstr : NUT_STRARG(host));
+					__func__, (addrstr && *addrstr) ? addrstr : NUT_STRARG(host));
 				break;
 			}
 			continue;

common/common.c

@@ -18,6 +18,8 @@
    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 */
 
+#define NUT_WANT_INET_NTOP_XX	1
+
 #include "common.h"
 #include "timehead.h"
 
@@ -4713,3 +4715,67 @@ int match_regex_hex(const regex_t *preg, const int n)
 	return match_regex(preg, buf);
 }
 #endif	/* HAVE_LIBREGEX */
+
+/* NOT THREAD SAFE!
+ * Helpers to convert one IP address to string from different structure types
+ * Return pointer to internal buffer, or NULL and errno upon errors */
+const char *inet_ntopSS(struct sockaddr_storage *s)
+{
+	static char str[40];
+
+	if (!s) {
+		errno = EINVAL;
+		return NULL;
+	}
+
+	switch (s->ss_family)
+	{
+	case AF_INET:
+		return inet_ntop (AF_INET, &(((struct sockaddr_in *)s)->sin_addr), str, 16);
+	case AF_INET6:
+		return inet_ntop (AF_INET6, &(((struct sockaddr_in6 *)s)->sin6_addr), str, 40);
+	default:
+		errno = EAFNOSUPPORT;
+		return NULL;
+	}
+}
+
+const char *inet_ntopAI(struct addrinfo *ai)
+{
+	/* Note: below we manipulate copies of ai - cannot cast into
+	 * specific structure type pointers right away because:
+	 *   error: cast from 'struct sockaddr *' to 'struct sockaddr_storage *'
+	 *          increases required alignment from 2 to 8
+	 */
+	/* https://stackoverflow.com/a/29147085/4715872
+	 * obviously INET6_ADDRSTRLEN is expected to be larger
+	 * than INET_ADDRSTRLEN, but this may be required in case
+	 * if for some unexpected reason IPv6 is not supported, and
+	 * INET6_ADDRSTRLEN is defined as 0
+	 * but this is not very likely and I am aware of no cases of
+	 * this in practice (editor)
+	 */
+	static char	addrstr[(INET6_ADDRSTRLEN > INET_ADDRSTRLEN ? INET6_ADDRSTRLEN : INET_ADDRSTRLEN) + 1];
+
+	if (!ai || !ai->ai_addr) {
+		errno = EINVAL;
+		return NULL;
+	}
+
+	addrstr[0] = '\0';
+	switch (ai->ai_family) {
+		case AF_INET: {
+			struct sockaddr_in	addr_in;
+			memcpy(&addr_in, ai->ai_addr, sizeof(addr_in));
+			return inet_ntop(AF_INET, &(addr_in.sin_addr), addrstr, INET_ADDRSTRLEN);
+		}
+		case AF_INET6: {
+			struct sockaddr_in6	addr_in6;
+			memcpy(&addr_in6, ai->ai_addr, sizeof(addr_in6));
+			return inet_ntop(AF_INET6, &(addr_in6.sin6_addr), addrstr, INET6_ADDRSTRLEN);
+		}
+		default:
+			errno = EAFNOSUPPORT;
+			return NULL;
+	}
+}

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3463 utf-8
+personal_ws-1.1 en 3467 utf-8
 AAC
 AAS
 ABI
@@ -2206,6 +2206,7 @@ ina
 includePath
 includedir
 inductor
+inet
 infos
 infoval
 inh
@@ -2624,6 +2625,9 @@ nowarn
 np
 nspr
 nss
+ntopAI
+ntopSS
+ntopW
 nuget
 num
 numOfBytesFromUPS

include/common.h

@@ -66,13 +66,26 @@
 #endif
 
 #ifndef WIN32
-#include <syslog.h>
+# include <syslog.h>
 #else	/* WIN32 */
-#include <winsock2.h>
-#include <windows.h>
-#include <ws2tcpip.h>
+# include <winsock2.h>
+# include <windows.h>
+# include <ws2tcpip.h>
 #endif	/* WIN32 */
 
+#ifdef NUT_WANT_INET_NTOP_XX
+/* We currently have a few consumers who should define this macro before
+ * including common.h, while we do not want to encumber preprocessing the
+ * majority of codebase with these headers and our (thread-unsafe) methods.
+ */
+# ifndef WIN32
+#  include <netdb.h>
+#  include <sys/socket.h>
+#  include <arpa/inet.h>
+#  include <netinet/in.h>
+# endif	/* WIN32 */
+#endif	/* NUT_WANT_INET_NTOP_XX */
+
 #include <unistd.h>	/* useconds_t */
 #ifndef HAVE_USECONDS_T
 # define useconds_t	unsigned long int
@@ -403,6 +416,14 @@ const char * rootpidpath(void);
 /* Die with a standard message if socket filename is too long */
 void check_unix_socket_filename(const char *fn);
 
+#ifdef NUT_WANT_INET_NTOP_XX
+/* NOT THREAD SAFE!
+ * Helpers to convert one IP address to string from different structure types
+ * Return pointer to internal buffer, or NULL and errno upon errors */
+const char *inet_ntopSS(struct sockaddr_storage *s);
+const char *inet_ntopAI(struct addrinfo *ai);
+#endif	/* NUT_WANT_INET_NTOP_XX */
+
 /* Provide integration for systemd inhibitor interface (where available,
  * dummy code otherwise) implementing the pseudo-code example from
  * https://systemd.io/INHIBITOR_LOCKS/

server/upsd.c

@@ -5,7 +5,7 @@
 	2008		Arjen de Korte <[email protected]>
 	2011 - 2012	Arnaud Quette <arnaud.quette.free.fr>
 	2019 		Eaton (author: Arnaud Quette <[email protected]>)
-	2020 - 2024	Jim Klimov <[email protected]>
+	2020 - 2025	Jim Klimov <[email protected]>
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -22,6 +22,8 @@
    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
 */
 
+#define NUT_WANT_INET_NTOP_XX	1
+
 #include "config.h"	/* must be the first header */
 
 #include "upsd.h"
@@ -170,22 +172,6 @@ static int	reload_flag = 0, exit_flag = 0;
 # define SERVICE_UNIT_NAME "nut-server.service"
 #endif
 
-static const char *inet_ntopW (struct sockaddr_storage *s)
-{
-	static char str[40];
-
-	switch (s->ss_family)
-	{
-	case AF_INET:
-		return inet_ntop (AF_INET, &(((struct sockaddr_in *)s)->sin_addr), str, 16);
-	case AF_INET6:
-		return inet_ntop (AF_INET6, &(((struct sockaddr_in6 *)s)->sin6_addr), str, 40);
-	default:
-		errno = EAFNOSUPPORT;
-		return NULL;
-	}
-}
-
 /* return a pointer to the named ups if possible */
 upstype_t *get_ups_ptr(const char *name)
 {
@@ -507,17 +493,13 @@ static void setuptcp(stype_t *server)
 		}
 
 		if (ai->ai_next) {
-			char ipaddrbuf[SMALLBUF];
-			const char *ipaddr;
-			snprintf(ipaddrbuf, sizeof(ipaddrbuf), " as ");
-			ipaddr = inet_ntop(ai->ai_family, ai->ai_addr,
-				ipaddrbuf + strlen(ipaddrbuf),
-				sizeof(ipaddrbuf));
+			const char *ipaddr = inet_ntopAI(ai);
 			upslogx(LOG_WARNING,
-				"setuptcp: bound to %s%s but there seem to be "
+				"setuptcp: bound to %s%s%s but there seem to be "
 				"further (ignored) addresses resolved for this name",
 				server->addr,
-				ipaddr == NULL ? "" : ipaddrbuf);
+				ipaddr == NULL ? "" : " as ",
+				ipaddr == NULL ? "" : ipaddr);
 		}
 
 		server->sock_fd = sock_fd;
@@ -812,7 +794,7 @@ static void client_connect(stype_t *server)
 
 	time(&client->last_heard);
 
-	client->addr = xstrdup(inet_ntopW(&csock));
+	client->addr = xstrdup(inet_ntopSS(&csock));
 
 	client->tracking = 0;