Use str* API to check for valid numeric value when parsing configs

server/conf.c

@@ -151,6 +151,8 @@ static void ups_update(const char *fn, const char *name, const char *desc)
  */
 static int parse_boolean(char *arg, int *result)
 {
+	assert(arg);
+	assert(result);
 	if ( (!strcasecmp(arg, "true")) || (!strcasecmp(arg, "on")) || (!strcasecmp(arg, "yes")) || (!strcasecmp(arg, "1"))) {
 		*result = 1;
 		return 1;
@@ -165,8 +167,9 @@ static int parse_boolean(char *arg, int *result)
 /* Return 1 if usable, 0 if not, -1 for an invalid LISTEN address. */
 static int parse_upsd_conf_args(size_t numargs, char **arg)
 {
-	int n, starts_with_digit;
-	long l;
+	int	starts_with_digit;
+	unsigned int	u;
+	long	l;
 
 	/* everything below here uses up through arg[1] */
 	if (numargs < 2)
@@ -185,33 +188,37 @@ static int parse_upsd_conf_args(size_t numargs, char **arg)
 		if ( str_to_int (arg[1], &lvl, 10) && lvl >= 0 ) {
 			nut_debug_level_global = lvl;
 		} else {
-			upslogx(LOG_INFO, "DEBUG_MIN has non numeric or negative value in upsd.conf");
+			upslogx(LOG_INFO, "DEBUG_MIN has non-numeric or negative value in upsd.conf");
 		}
 		return 1;
 	}
 
 	/* MAXAGE <seconds> */
 	if (!strcmp(arg[0], "MAXAGE")) {
-		if (starts_with_digit && str_to_int(arg[1], &n, 10)) {
-			maxage = n;
+		/* In practice, sstate_dead() compares "elapsed" since "ups->last_heard":
+		 * to "maxage/3" for ping to driver,
+		 * to "maxage + 1" to proclaim dead, and
+		 * to "maxage" to log as probably dead.
+		 * Values under 0 are outright wrong, and under cca 5 not really good
+		 * (self-inflicted DoS on pinger and log writer) although not illegal.
+		 * Might be useful for stress-testing etc.
+		 */
+		if (starts_with_digit && str_to_uint(arg[1], &u, 10)) {
+			maxage = u;
 			return 1;
 		}
-		else {
-			upslogx(LOG_ERR, "MAXAGE has invalid or out of range numeric value (%s)!", arg[1]);
-			return 0;
-		}
+		goto invalid_number_out_of_range;
 	}
 
 	/* TRACKINGDELAY <seconds> */
 	if (!strcmp(arg[0], "TRACKINGDELAY")) {
-		if (starts_with_digit && str_to_int(arg[1], &n, 10)) {
-			tracking_delay = n;
+    /* Perhaps a negative value is not toxic, but 0 suffices to forget 
+     * the status info right away, should someone want to (for tests?) */
+		if (starts_with_digit && str_to_uint(arg[1], &u, 10)) {
+			tracking_delay = u;
 			return 1;
 		}
-		else {
-			upslogx(LOG_ERR, "TRACKINGDELAY has invalid or out of range numeric value (%s)!", arg[1]);
-			return 0;
-		}
+		goto invalid_number_out_of_range;
 	}
 
 	/* ALLOW_NO_DEVICE <bool> */
@@ -223,8 +230,7 @@ static int parse_upsd_conf_args(size_t numargs, char **arg)
 		if (parse_boolean(arg[1], &allow_no_device))
 			return 1;
 
-		upslogx(LOG_ERR, "ALLOW_NO_DEVICE has non numeric and non boolean value (%s)!", arg[1]);
-		return 0;
+		goto invalid_notnumber_notboolean;
 	}
 
 	/* ALLOW_NOT_ALL_LISTENERS <bool> */
@@ -236,8 +242,7 @@ static int parse_upsd_conf_args(size_t numargs, char **arg)
 		if (parse_boolean(arg[1], &allow_not_all_listeners))
 			return 1;
 
-		upslogx(LOG_ERR, "ALLOW_NOT_ALL_LISTENERS has non numeric and non boolean value (%s)!", arg[1]);
-		return 0;
+		goto invalid_notnumber_notboolean;
 	}
 
 	/* MAXCONN <connections> */
@@ -248,10 +253,7 @@ static int parse_upsd_conf_args(size_t numargs, char **arg)
 			maxconn = (nfds_t)l;
 			return 1;
 		}
-		else {
-			upslogx(LOG_ERR, "MAXCONN has invalid or out of range numeric value (%s)!", arg[1]);
-			return 0;
-		}
+		goto invalid_number_out_of_range;
 	}
 
 	/* STATEPATH <dir> */
@@ -317,10 +319,11 @@ static int parse_upsd_conf_args(size_t numargs, char **arg)
 # ifdef WITH_CLIENT_CERTIFICATE_VALIDATION
 	/* CERTREQUEST (0 | 1 | 2) */
 	if (!strcmp(arg[0], "CERTREQUEST")) {
-		if (starts_with_digit && str_to_int(arg[1], &n, 10)
-		&&  n >= NETSSL_CERTREQ_MIN && n <= NETSSL_CERTREQ_MAX
-		) {
-			certrequest = n;
+		/* NOTE: Not checking so far for u >= NETSSL_CERTREQ_MIN (0)
+		 *  to avoid reasonable compiler warnings about "always true"
+		 *  conditional checks with an unsigned int */
+		if (starts_with_digit && str_to_uint(arg[1], &u, 10) && u <= NETSSL_CERTREQ_MAX) {
+			certrequest = u;
 			return 1;
 		}
 		else {
@@ -336,8 +339,7 @@ static int parse_upsd_conf_args(size_t numargs, char **arg)
 				certrequest = NETSSL_CERTREQ_REQUIRE;	/* 2 */
 				return 1;
 			}
-			upslogx(LOG_ERR, "CERTREQUEST has invalid or out of range value (%s)!", arg[1]);
-			return 0;
+			goto invalid_number_out_of_range;
 		}
 	}
 # else	/* !WITH_CLIENT_CERTIFICATE_VALIDATION */
@@ -416,6 +418,17 @@ static int parse_upsd_conf_args(size_t numargs, char **arg)
 
 	/* not recognized */
 	return 0;
+
+invalid_number_out_of_range:
+	upslogx(LOG_ERR, "%s has invalid or out of range numeric value (%s)! %s",
+		arg[0], arg[1], errno ? strerror(errno) : "");
+	return 0;
+
+invalid_notnumber_notboolean:
+	upslogx(LOG_ERR, "%s has non numeric and non boolean value (%s)!",
+		arg[0], arg[1]);
+	return 0;
+
 }
 
 /* called for fatal errors in parseconf like malloc failures */

server/netssl.c

@@ -74,7 +74,7 @@ char	*certpath = NULL;
 int	disable_weak_ssl = 0;
 
 #ifdef WITH_CLIENT_CERTIFICATE_VALIDATION
-int	certrequest = 0;
+unsigned int	certrequest = 0;
 #endif /* WITH_CLIENT_CERTIFICATE_VALIDATION */
 
 static int	ssl_initialized = 0;

server/netssl.h

@@ -34,7 +34,7 @@ extern char	*certpasswd;
 extern char	*certpath;
 extern int	disable_weak_ssl;
 #ifdef WITH_CLIENT_CERTIFICATE_VALIDATION
-extern int	certrequest;
+extern unsigned int	certrequest;
 #endif /* WITH_CLIENT_CERTIFICATE_VALIDATION */
 
 /* List possible values for certrequested */

server/sstate.c

@@ -460,10 +460,10 @@ const cmdlist_t *sstate_getcmdlist(const upstype_t *ups)
 	return ups->cmdlist;
 }
 
-int sstate_dead(upstype_t *ups, int arg_maxage)
+int sstate_dead(upstype_t *ups, unsigned int arg_maxage)
 {
 	time_t	now;
-	double	elapsed;
+	double	elapsed, dbl_maxage = (double)arg_maxage;
 
 	/* an unconnected ups is always dead */
 	if ((!ups) || INVALID_FD(ups->sock_fd)) {
@@ -478,23 +478,24 @@ int sstate_dead(upstype_t *ups, int arg_maxage)
 	/* Somewhere beyond a third of the maximum time - prod it to make it talk
 	 * Note this helps detect drivers that died without closing the connection
 	 */
-	if ((elapsed > (arg_maxage / 3)) && (difftime(now, ups->last_ping) > (arg_maxage / 3)))
+	if ((elapsed > (dbl_maxage / 3)) && (difftime(now, ups->last_ping) > (dbl_maxage / 3)))
 		sendping(ups);
 
-	if (elapsed > arg_maxage + 1.0) {
-		upsdebugx(3, "%s: didn't hear from driver for UPS [%s] for %g seconds (max %d)",
+	if (elapsed > dbl_maxage + 1.0) {
+		upsdebugx(3, "%s: didn't hear from driver for UPS [%s] for %g seconds (max %u)",
 			__func__, ups->name, elapsed, arg_maxage);
 		return 1;	/* dead */
 	}
 
-	if (elapsed > arg_maxage) {
+	if (elapsed > dbl_maxage) {
 		/* Per https://github.com/networkupstools/nut/issues/661 we do
 		 * often see "is stale" and "is alive" messages in the same
 		 * second, especially on busy systems that can not dedicate
 		 * every scheduled time slot to NUT daemons and data pipes.
 		 * So data exchange frequency settings may almost race...
 		 */
-		upsdebugx(3, "%s: didn't hear from driver for UPS [%s] for %g seconds (max %d); will declare it dead next second (delaying just in case of whole-second rounding issues)",
+		upsdebugx(3, "%s: didn't hear from driver for UPS [%s] for %g seconds (max %u); "
+			"will declare it dead next second (delaying just in case of whole-second rounding issues)",
 			__func__, ups->name, elapsed, arg_maxage);
 		return 0;	/* probably dead */
 	}

server/sstate.h

@@ -48,7 +48,7 @@ const cmdlist_t *sstate_getcmdlist(const upstype_t *ups);
 void sstate_makeinfolist(const upstype_t *ups, char *buf, size_t bufsize);
 void sstate_makerwlist(const upstype_t *ups, char *buf, size_t bufsize);
 void sstate_makeinstcmdlist_t(const upstype_t *ups, char *buf, size_t bufsize);
-int sstate_dead(upstype_t *ups, int maxage);
+int sstate_dead(upstype_t *ups, unsigned int maxage);
 void sstate_infofree(upstype_t *ups);
 void sstate_cmdfree(upstype_t *ups);
 int sstate_sendline(upstype_t *ups, const char *buf);

server/upsd.c

@@ -80,10 +80,10 @@ int	deny_severity = LOG_WARNING;
 upstype_t	*firstups = NULL;
 
 /* default 15 seconds before data is marked stale */
-int	maxage = 15;
+unsigned int	maxage = 15;
 
 /* default to 1h before cleaning up status tracking entries */
-int	tracking_delay = 3600;
+unsigned int	tracking_delay = 3600;
 
 /*
  * Preloaded to ALLOW_NO_DEVICE from upsd.conf or environment variable

server/upsd.h

@@ -122,7 +122,8 @@ int tracking_disable(void);
 int tracking_is_enabled(void);
 
 /* declarations from upsd.c */
-extern int		maxage, tracking_delay, allow_no_device, allow_not_all_listeners;
+extern unsigned int	maxage, tracking_delay;
+extern int		allow_no_device, allow_not_all_listeners;
 extern nfds_t		maxconn;
 extern char		*statepath, *datapath;
 extern upstype_t	*firstups;

tests/.gitignore

@@ -19,6 +19,7 @@
 /test_authconf.log
 /test_authconf.trs
 /test_upsdconf
+/test_upsdconf-*/
 /test_upsdconf.log
 /test_upsdconf.trs
 /selftest-rw/*

tests/Makefile.am

@@ -476,8 +476,8 @@ MAINTAINERCLEANFILES = Makefile.in .dirstamp
 
 # NOTE: Do not clean ".deps" in SUBDIRS of the main project,
 # the root Makefile.am takes care of that!
-#clean-local:
-#	$(AM_V_at)rm -rf $(builddir)/.deps
+clean-local:
+	$(AM_V_at)rm -rf $(builddir)/test_upsdconf-*/
 
 # Helper for only the enabled libs to get built:
 all-libs-local: $(lib_LTLIBRARIES) $(noinst_LTLIBRARIES) $(EXTRA_LTLIBRARIES)

tests/test_upsdconf.c

@@ -17,7 +17,7 @@ static const char *listen_host = NULL, *listen_port = NULL;
 static int listen_calls = 0;
 
 /* Storage normally owned by upsd.c and netssl.c. Keep the public types. */
-int maxage = 15, tracking_delay = 3600;
+unsigned int maxage = 15, tracking_delay = 3600;
 int allow_no_device = 0, allow_not_all_listeners = 0;
 nfds_t maxconn = 0;
 char *statepath = NULL, *datapath = NULL;
@@ -32,8 +32,8 @@ int certrequest = 0;
 /* These daemon operations must not be reached by the configuration tests. */
 static void unexpected_call(const char *name)
 {
-	upslogx(LOG_ERR, "Unexpected %s", name);
 	failures++;
+	upslogx(LOG_ERR, "FAILURE [#%d]\tUnexpected %s", failures, name);
 }
 
 upstype_t *get_ups_ptr(const char *name)
@@ -160,9 +160,10 @@ static void check_value(const char *option, const char *value, int accepted, uin
 	actual = stored_value(option);
 	checks++;
 	if (result != accepted || actual != expected) {
-		upslogx(LOG_ERR, "FAIL %s '%s': accepted=%d, value=%" PRIuMAX
-			"; expected %d, %" PRIuMAX, option, value, result, actual, accepted, expected);
 		failures++;
+		upslogx(LOG_ERR, "FAILURE [#%d]\t%s '%s': accepted=%d, value=%" PRIuMAX
+			"; expected %d, %" PRIuMAX,
+			failures, option, value, result, actual, accepted, expected);
 	}
 	free(args[0]);
 	free(args[1]);
@@ -187,9 +188,10 @@ static void check_value(const char *option, const char *value, int accepted, uin
 		actual = stored_value(option);
 		checks++;
 		if (actual != expected) {
-			upslogx(LOG_ERR, "FAIL load_upsdconf(%d), %s '%s': value=%" PRIuMAX
-				"; expected %" PRIuMAX, reloading, option, value, actual, expected);
 			failures++;
+			upslogx(LOG_ERR, "FAILURE [#%d]\tload_upsdconf(%d), %s '%s': value=%" PRIuMAX
+				"; expected %" PRIuMAX,
+				failures, reloading, option, value, actual, expected);
 		}
 	}
 }
@@ -286,9 +288,14 @@ int main(void)
 			check_value(options[i], invalid[j], 0, previous);
 
 		limit = (uintmax_t)INT_MAX;
+		if (!strcmp(options[i], "MAXAGE") || !strcmp(options[i], "TRACKINGDELAY")) {
+			limit = (uintmax_t)UINT_MAX;
+		} else
 		if (!strcmp(options[i], "MAXCONN")) {
 			limit = (uintmax_t)((nfds_t)-1);
-			if (limit > (uintmax_t)LONG_MAX) limit = (uintmax_t)LONG_MAX;
+			if (limit > (uintmax_t)LONG_MAX) {
+				limit = (uintmax_t)LONG_MAX;
+			}
 		}
 		else if (!strcmp(options[i], "CERTREQUEST")) {
 			limit = (uintmax_t)(NETSSL_CERTREQ_MAX);
@@ -299,8 +306,14 @@ int main(void)
 		check_value(options[i], number, 1, limit);
 		snprintf(number, sizeof(number), "%" PRIuMAX, limit + 1);
 		check_value(options[i], number, 0, previous);
+
+		/* Platform-dependent (32/64-bit ints) - value may overflow the fixed max or not */
 		snprintf(number, sizeof(number), "%" PRIuMAX, (uintmax_t)LONG_MAX + 1);
-		check_value(options[i], number, 0, previous);
+		if ((uintmax_t)LONG_MAX < limit) {
+			check_value(options[i], number, 1, (uintmax_t)LONG_MAX + 1);
+		} else {
+			check_value(options[i], number, 0, previous);
+		}
 	}
 #if defined(WITH_SSL) && defined(WITH_CLIENT_CERTIFICATE_VALIDATION)
 	check_value("CERTREQUEST", "NO", 1, 0);
@@ -329,8 +342,8 @@ int main(void)
 	ups.last_ping = ups.last_heard;
 	checks++;
 	if (sstate_dead(&ups, INT_MAX)) {
-		upslogx(LOG_ERR, "FAIL MAXAGE INT_MAX: fresh driver data is stale");
 		failures++;
+		upslogx(LOG_ERR, "FAILURE [#%d]\tMAXAGE INT_MAX: fresh driver data is stale", failures);
 	}
 	free(ups.name);
 #endif