Merge pull request networkupstools#2963 from sebastiankuttnig/issue-2948

common.c: VA copies with buffer upsizing, upsdrvquery.c: str handling fixes

Author: jimklimov

NEWS.adoc

@@ -59,6 +59,13 @@ https://github.com/networkupstools/nut/milestone/9
      `DRIVERLIST` variables inspected by `configure` script, rather than
      having all their names hard-coded like before, led to inability to
      `configure --with-drivers=dummy-ups`. [#2825, #2927, fixed by PR #2929]
+   * A problem noted with `upsdrvquery` (since NUT v2.8.1) message logging
+     at high debug verbosity levels (5+) with very large blocks of content
+     has exposed a deficiency in variable-argument handling, and specifically
+     adaptive resizing of the output buffer or truncation of logged inputs
+     (which is something NUT code tried to do since the beginning of time),
+     and could lead to "segmentation fault" crashes on some platforms.
+     [issue #2948, PR #2963]
 
  - common code:
    * Revised common `writepid()` to use `altpidpath()` as location for the

common/common.c

@@ -3206,9 +3206,25 @@ static void vupslog(int priority, const char *fmt, va_list va, int use_strerror)
 	 * or the calling methods should check it against their
 	 * "fmt_dynamic" expectations). */
 	do {
+#ifdef HAVE_VA_COPY_VARIANT
+		va_list	va_snprintf;
+
+		/* va_copy() to avoid mangling on re-use (see issue #2948),
+		 * this lets us retry safely vsnprintf() with the VA copies */
+		va_copy(va_snprintf, va);
+		ret = vsnprintf(buf, bufsize, fmt, va_snprintf);
+		va_end(va_snprintf);
+#else
+		/* Without va_copy(), we cannot safely retry vsnprintf()
+		 * and will accept truncation if the buffer is too small */
 		ret = vsnprintf(buf, bufsize, fmt, va);
+#endif
 
 		if ((ret < 0) || ((uintmax_t)ret >= (uintmax_t)bufsize)) {
+			/* Not building the below block on systems without va_copy(),
+			 * otherwise consumed VAs would get re-used & mangled on retries.
+			 * Instead, we want fall-through directly to the truncation message. */
+#ifdef HAVE_VA_COPY_VARIANT
 			/* Try to adjust bufsize until we can print the
 			 * whole message. Note that standards only require
 			 * up to 4095 bytes to be manageable in printf-like
@@ -3261,6 +3277,7 @@ static void vupslog(int priority, const char *fmt, va_list va, int use_strerror)
 		/* Arbitrary limit, gotta stop somewhere */
 		if (bufsize > LARGEBUF * 64) {
 vupslog_too_long:
+#endif
 			if (syslog_is_disabled()) {
 				fprintf(stderr, "vupslog: vsnprintf needed "
 					"more than %" PRIuSIZE " bytes; logged "

configure.ac

@@ -1433,6 +1433,54 @@ AS_IF([test x"${ac_cv_func_strdup}" = xyes],
     [AC_MSG_WARN([Required C library routine strdup not found; try adding -D_POSIX_C_SOURCE=200112L])]
     )
 
+AC_CACHE_CHECK([for va_copy(dest,src)],
+  [ac_cv_have_va_copy],
+  [
+    AX_RUN_OR_LINK_IFELSE(
+      [AC_LANG_PROGRAM(
+        [[#include <stdarg.h>
+          void check_va_copy(const char *fmt, ...) {
+            va_list ap1, ap2;
+            va_start(ap1, fmt);
+            va_copy(ap2, ap1);
+            va_end(ap1);
+            va_end(ap2);
+          }]],
+        [[check_va_copy("%s", "test");]])],
+      [ac_cv_have_va_copy=yes],
+      [ac_cv_have_va_copy=no])
+  ])
+
+AS_IF([test x"$ac_cv_have_va_copy" = xyes],
+  [AC_DEFINE([HAVE_VA_COPY], [1], [defined if va_copy(dest,src) is available])])
+
+AC_CACHE_CHECK([for __va_copy(dest,src)],
+  [ac_cv_have___va_copy],
+  [
+    AX_RUN_OR_LINK_IFELSE(
+      [AC_LANG_PROGRAM(
+        [[#include <stdarg.h>
+          void check___va_copy(const char *fmt, ...) {
+            va_list ap1, ap2;
+            va_start(ap1, fmt);
+            __va_copy(ap2, ap1);
+            va_end(ap1);
+            va_end(ap2);
+          }]],
+        [[check___va_copy("%s", "test");]])],
+      [ac_cv_have___va_copy=yes],
+      [ac_cv_have___va_copy=no])
+  ])
+
+AS_IF([test x"$ac_cv_have___va_copy" = xyes],
+  [AC_DEFINE([HAVE___VA_COPY], [1], [defined if __va_copy(dest,src) is available])])
+
+AS_IF([test x"$ac_cv_have_va_copy" = xyes -o x"$ac_cv_have___va_copy" = xyes],
+  [AC_DEFINE([HAVE_VA_COPY_VARIANT], [1], [defined if va_copy or __va_copy is available])])
+
+AS_IF([test x"$ac_cv_have_va_copy" != xyes -a x"$ac_cv_have___va_copy" != xyes],
+  [AC_MSG_WARN([No va_copy(dest,src) or __va_copy(dest,src) available - may truncate oversized logs])])
+
 AC_CHECK_HEADER([sys/select.h],
     [AC_DEFINE([HAVE_SYS_SELECT_H], [1],
         [Define to 1 if you have <sys/select.h>.])])

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3502 utf-8
+personal_ws-1.1 en 3503 utf-8
 AAC
 AAS
 ABI
@@ -2902,6 +2902,7 @@ reposurgeon
 repotec
 req
 resetter
+resizing
 resolv
 restoreconf
 resync

drivers/upsdrvquery.c

@@ -363,12 +363,21 @@ ssize_t upsdrvquery_read_timeout(udq_pipe_conn_t *conn, struct timeval tv) {
 		ret = -1;
 #endif  /* WIN32 */
 
-	upsdebugx(ret > 0 ? 5 : 6,
-		"%s: received %" PRIiMAX " bytes from driver socket: %s",
-		__func__, (intmax_t)ret, (ret > 0 ? conn->buf : "<null>"));
-	if (ret > 0 && conn->buf[0] == '\0')
-		upsdebug_hex(5, "payload starts with zero byte: ",
-			conn->buf, ((size_t)ret > sizeof(conn->buf) ? sizeof(conn->buf) : (size_t)ret));
+	if (ret > 0) {
+		size_t len = (size_t)ret > sizeof(conn->buf) ? sizeof(conn->buf) : (size_t)ret;
+
+		upsdebugx(5, "%s: received %" PRIiMAX " bytes from driver socket: %.*s",
+			__func__, (intmax_t)ret, (int)len, conn->buf);
+
+		if (conn->buf[0] == '\0') {
+			upsdebug_hex(5, "payload starts with zero byte: ",
+				conn->buf, len);
+		}
+	} else {
+		upsdebugx(6, "%s: received %" PRIiMAX " bytes from driver socket: <null>",
+			__func__, (intmax_t)ret);
+	}
+
 	return ret;
 }
 
@@ -483,7 +492,11 @@ ssize_t upsdrvquery_prepare(udq_pipe_conn_t *conn, struct timeval tv) {
 	}
 
 	/* Check that we can have a civilized dialog --
-	 * nope, this one is for network protocol */
+	 * nope, this one is for network protocol
+	 *
+	 * FIXME: strcmp(conn->buf, "ON") -> buf is NOT null terminated!
+	 * The above FIXME is not the reason this block is/was commented out.
+	 */
 /*
 	if (upsdrvquery_write(conn, "GET TRACKING\n") < 0)
 		goto socket_error;
@@ -745,7 +758,8 @@ ssize_t upsdrvquery_oneshot_conn(
 	}
 
 	if (buf) {
-		snprintf(buf, bufsz, "%s", conn->buf);
+		size_t len = strnlen(conn->buf, sizeof(conn->buf));
+		snprintf(buf, bufsz, "%.*s", (int)len, conn->buf);
 	}
 
 finish:

include/common.h

@@ -546,6 +546,13 @@ int vsnprintfcat(char *dst, size_t size, const char *fmt, va_list ap);
 int snprintfcat(char *dst, size_t size, const char *fmt, ...)
 	__attribute__ ((__format__ (__printf__, 3, 4)));
 
+/* Define a missing va_copy using __va_copy, if available: */
+#ifndef HAVE_VA_COPY
+# ifdef HAVE___VA_COPY
+#  define va_copy(dest, src) __va_copy(dest, src)
+# endif
+#endif
+
 /* Mitigate the inherent insecurity of dynamically constructed formatting
  * strings vs. a fixed vararg list with its amounts and types of variables
  * printed by this or that method and pre-compiled in the program.