Merge remote-tracking branch 'origin/issue-2450'

Closes: PR networkupstools#2460

Signed-off-by: Jim Klimov <[email protected]>

Author: jimklimov

NEWS.adoc

@@ -49,6 +49,9 @@ https://github.com/networkupstools/nut/milestone/9
    * Refactored NUT "common" sources to reference `nut_version.h` macros from
      a smaller C source file, to minimize the compilation unit size impacted
      by development iterations. [issue #2097]
+   * 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]
 
 
 Release notes for NUT 2.8.3 - what's new since 2.8.2

clients/upsclient.c

@@ -567,16 +567,6 @@ const char *upscli_strerror(UPSCONN_t *ups)
 	char	sslbuf[UPSCLI_ERRBUF_LEN];
 #endif
 
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic push
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
-#pragma GCC diagnostic ignored "-Wformat-security"
-#endif
-
 	if (!ups) {
 		return upscli_errlist[UPSCLI_ERR_INVALIDARG].str;
 	}
@@ -595,23 +585,26 @@ const char *upscli_strerror(UPSCONN_t *ups)
 		return upscli_errlist[ups->upserror].str;
 
 	case 1:		/* add message from system's strerror */
-		snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN,
+		snprintf_dynamic(
+			ups->errbuf, UPSCLI_ERRBUF_LEN,
 			upscli_errlist[ups->upserror].str,
-			strerror(ups->syserrno));
+			"%s", strerror(ups->syserrno));
 		return ups->errbuf;
 
 	case 2:		/* SSL error */
 #ifdef WITH_OPENSSL
 		err = ERR_get_error();
 		if (err) {
 			ERR_error_string(err, sslbuf);
-			snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN,
+			snprintf_dynamic(
+				ups->errbuf, UPSCLI_ERRBUF_LEN,
 				upscli_errlist[ups->upserror].str,
-				sslbuf);
+				"%s", sslbuf);
 		} else {
-			snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN,
+			snprintf_dynamic(
+				ups->errbuf, UPSCLI_ERRBUF_LEN,
 				upscli_errlist[ups->upserror].str,
-				"peer disconnected");
+				"%s", "peer disconnected");
 		}
 #elif defined(WITH_NSS) /* WITH_OPENSSL */
 		if (PR_GetErrorTextLength() < UPSCLI_ERRBUF_LEN) {
@@ -628,19 +621,16 @@ const char *upscli_strerror(UPSCONN_t *ups)
 		return ups->errbuf;
 
 	case 3:		/* parsing (parseconf) error */
-		snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN,
+		snprintf_dynamic(
+			ups->errbuf, UPSCLI_ERRBUF_LEN,
 			upscli_errlist[ups->upserror].str,
-			ups->pc_ctx.errmsg);
+			"%s", ups->pc_ctx.errmsg);
 		return ups->errbuf;
 
 	default:
 		break;
 	}
 
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic pop
-#endif
-
 	/* fallthrough */
 
 	snprintf(ups->errbuf, UPSCLI_ERRBUF_LEN, "Unknown error flag %d",
@@ -1392,23 +1382,9 @@ static void build_cmd(char *buf, size_t bufsize, const char *cmdname,
 			format = " %s";
 		}
 
-		/* snprintfcat would tie us to common */
-
-		len = strlen(buf);
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic push
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
-#pragma GCC diagnostic ignored "-Wformat-security"
-#endif
-		snprintf(buf + len, bufsize - len, format,
-			pconf_encode(arg[i], enc, sizeof(enc)));
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic pop
-#endif
+		snprintfcat_dynamic(
+			buf, bufsize, format,
+			"%s", pconf_encode(arg[i], enc, sizeof(enc)));
 	}
 
 	len = strlen(buf);

clients/upsimage.c

@@ -278,20 +278,10 @@ static void drawbar(
 	gdImageFilledRectangle(im, 25, bar_y, width - 25, scale_height,
 		bar_color);
 
-	/* stick the text version of the value at the bottom center */
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic push
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
-#endif
-#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
-#pragma GCC diagnostic ignored "-Wformat-security"
-#endif
-	snprintf(text, sizeof(text), format, value);
-#ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
-#pragma GCC diagnostic pop
-#endif
+	/* stick the text version of the value at the bottom center
+	 * expected format is one of imgvar[] entries for "double value"
+	 */
+	snprintf_dynamic(text, sizeof(text), format, "%f", value);
 	gdImageString(im, gdFontMediumBold,
 		(width - (int)(strlen(text))*gdFontMediumBold->w)/2,
 		height - gdFontMediumBold->h,
@@ -324,6 +314,12 @@ static void noimage(const char *fmt, ...)
 #ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
 #pragma GCC diagnostic ignored "-Wformat-security"
 #endif
+	/* Note: Not converting to hardened NUT methods with dynamic
+	 * format string checking, this one is used locally with
+	 * fixed strings (and args) */
+	/* FIXME: Actually, almost only fixed strings, no formatting
+	 * needed here: one use-case of having a format, and another
+	 * with externally prepared snprintf(). */
 	vsnprintf(msg, sizeof(msg), fmt, ap);
 #ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
 #pragma GCC diagnostic pop

clients/upsmon.c

@@ -405,6 +405,7 @@ static void do_notify(const utype_t *ups, unsigned int ntype, const char *extra)
 			upsdebugx(2, "%s: ntype 0x%04x (%s)",
 				__func__, ntype,
 				notifylist[i].name);
+
 #ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
 #pragma GCC diagnostic push
 #endif
@@ -427,13 +428,15 @@ static void do_notify(const utype_t *ups, unsigned int ntype, const char *extra)
 					 *  only have one "%s", plaster another
 					 *  to "msgfmt" and follow this path?
 					 */
-					snprintf(msg, sizeof(msg),
+					snprintf_dynamic(msg, sizeof(msg),
 						msgfmt,
+						"%s%s",
 						upsname ? upsname : "",
 						NUT_STRARG(extra));
 				} else {
-					snprintf(msg, sizeof(msg),
+					snprintf_dynamic(msg, sizeof(msg),
 						msgfmt,
+						"%s",
 						upsname ? upsname : "");
 				}
 			} else {

clients/upssched.c

@@ -413,6 +413,10 @@ static int send_to_one(conn_t *conn, const char *fmt, ...)
 #ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
 #pragma GCC diagnostic ignored "-Wformat-security"
 #endif
+	/* Note: Not converting to hardened NUT methods with dynamic
+	 * format string checking, this one is used locally with
+	 * fixed strings (and args) */
+	/* FIXME: Actually, only fixed strings, no formatting here. */
 	vsnprintf(buf, sizeof(buf), fmt, ap);
 #ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
 #pragma GCC diagnostic pop

clients/upsset.c

@@ -281,6 +281,9 @@ static void error_page(const char *next, const char *title,
 #ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC_IGNORED_FORMAT_SECURITY
 #pragma GCC diagnostic ignored "-Wformat-security"
 #endif
+	/* Note: Not converting to hardened NUT methods with dynamic
+	 * format string checking, this one is used locally with
+	 * fixed strings (and args) quite extensively. */
 	vsnprintf(msg, sizeof(msg), fmt, ap);
 #ifdef HAVE_PRAGMAS_FOR_GCC_DIAGNOSTIC_IGNORED_FORMAT_NONLITERAL
 #pragma GCC diagnostic pop