diff --git a/server/conf.c b/server/conf.c index 5cc0ff5986..555421c549 100644 --- a/server/conf.c +++ b/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 */ 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 */ 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 */ @@ -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 */ @@ -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 */ @@ -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 */ @@ -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 */ diff --git a/server/netssl.c b/server/netssl.c index 4ff10e791f..4025941a5f 100644 --- a/server/netssl.c +++ b/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; diff --git a/server/netssl.h b/server/netssl.h index 7b2d2dcabd..104dc0f091 100644 --- a/server/netssl.h +++ b/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 */ diff --git a/server/sstate.c b/server/sstate.c index b96b430edc..290aef8779 100644 --- a/server/sstate.c +++ b/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 */ } diff --git a/server/sstate.h b/server/sstate.h index dde724a035..a24f45ee4d 100644 --- a/server/sstate.h +++ b/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); diff --git a/server/upsd.c b/server/upsd.c index 63916e2920..b42784451f 100644 --- a/server/upsd.c +++ b/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 diff --git a/server/upsd.h b/server/upsd.h index 35007ac0fa..2c0e31c4d0 100644 --- a/server/upsd.h +++ b/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; diff --git a/tests/.gitignore b/tests/.gitignore index 56dc9deefe..2dccfa367b 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -19,6 +19,7 @@ /test_authconf.log /test_authconf.trs /test_upsdconf +/test_upsdconf-*/ /test_upsdconf.log /test_upsdconf.trs /selftest-rw/* diff --git a/tests/Makefile.am b/tests/Makefile.am index 3577f83028..43c4ca67ac 100644 --- a/tests/Makefile.am +++ b/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) diff --git a/tests/test_upsdconf.c b/tests/test_upsdconf.c index 3ff298f669..8e996954b5 100644 --- a/tests/test_upsdconf.c +++ b/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