From ad253f22e73be2b5aacb3fa305f23d7762b2c4c4 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 22 Jul 2024 18:03:09 +0200 Subject: [PATCH 01/12] server/netget.c, docs/net-protocol.txt: set_var(): report IMMUTABLE flag on values in DEBUG builds [#266] Signed-off-by: Jim Klimov --- docs/net-protocol.txt | 5 +++++ server/netget.c | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/docs/net-protocol.txt b/docs/net-protocol.txt index 577e29abaa..b7795c8de6 100644 --- a/docs/net-protocol.txt +++ b/docs/net-protocol.txt @@ -156,6 +156,11 @@ exponents, and comma for thousands separator are forbidden. For example: "1200.20" is valid, while "1,200.20" and "1200,20" and "1.2e4" are invalid. +Also note that internally NUT programs can flag variables as 'IMMUTABLE': +a value of such variable can not be changed after it was initially set +(this is used e.g. for "override" settings where the device is known to +provide bogus readings). The flag is not currently exposed with `GET` +commands, but can be a reason for a `SET` to fail. This replaces the old "VARTYPE" command. diff --git a/server/netget.c b/server/netget.c index 4be093cf36..9054342550 100644 --- a/server/netget.c +++ b/server/netget.c @@ -138,6 +138,19 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) snprintf(buf, sizeof(buf), "TYPE %s %s", upsname, var); + if (node->flags & ST_FLAG_IMMUTABLE) { +#if DEBUG + /* Properly exposing this needs also an update to + * docs/net-protocol.txt (promote the paragraph + * provided as a note currently) and to the NUT RFC + * https://www.rfc-editor.org/info/rfc9271 + */ + snprintfcat(buf, sizeof(buf), " IMMUTABLE"); +#endif + upsdebugx(3, "%s: UPS[%s] variable %s is IMMUTABLE", + __func__, upsname, var); + } + if (node->flags & ST_FLAG_RW) snprintfcat(buf, sizeof(buf), " RW"); From 5b6f1930517eea5085cf4f899107230545447530 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 22 Jul 2024 18:29:46 +0200 Subject: [PATCH 02/12] server/netset.c: consider IMMUTABLE flag to reject setting a value [#266] Signed-off-by: Jim Klimov --- server/netset.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/server/netset.c b/server/netset.c index bd5f033a2e..b35559572d 100644 --- a/server/netset.c +++ b/server/netset.c @@ -152,6 +152,16 @@ static void set_var(nut_ctype_t *client, const char *upsname, const char *var, } } + /* FIXME: Consider null/non-null values for strings + * See note on sstate_getnode() idea above. + */ + if (sstate_getflags(ups, var) & ST_FLAG_IMMUTABLE) { + upsdebugx(3, "%s: UPS [%s]: value of %s is already set and immutable", + __func__, ups->name, var); + send_err(client, NUT_ERR_READONLY); + return; + } + /* must be OK now */ snprintf(cmd, sizeof(cmd), "SET %s \"%s\"", From 56a9063dbb90f6e47dc62ba4d62dba6568e83b35 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 22 Jul 2024 19:47:25 +0200 Subject: [PATCH 03/12] server/netget.c: sanity-check NUMBER values that they actually resolve as float or long int [#266] If we assume a value without flags is by default a NUMBER but is not, re-default it to STRING. If it is also flagged explicitly as a NUMBER but is not, yell a warning for upstream to fix the code. Eventually this should be a reason to report protocol error, but not now. Signed-off-by: Jim Klimov --- server/netget.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/server/netget.c b/server/netget.c index 9054342550..bd008cfcd7 100644 --- a/server/netget.c +++ b/server/netget.c @@ -24,6 +24,7 @@ #include "state.h" #include "desc.h" #include "neterr.h" +#include "nut_stdint.h" #include "netget.h" @@ -175,6 +176,55 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) __func__, upsname, var); } + /* Sanity-check current contents */ + if (node->val && *(node->val)) { + char *s; + float f; + long l; + int i, ok = 1; + size_t len = strlen(node->val); + + errno = 0; + i = sscanf(node->val, "%f", &f); + if (i < 1 || (uintmax_t)i > (uintmax_t)(len + 1) || errno || *(s = node->val + i) != '\0') { + upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not a float: %s", + __func__, upsname, var, node->val); + ok = 0; + } + + if (!ok) { + /* did not parse as a float... range issues or NaN? */ + errno = 0; + ok = 1; + i = sscanf(node->val, "%ld", &l); + if (i < 1 || (uintmax_t)i > (uintmax_t)(len + 1) || errno || *(s = node->val + i) != '\0') { + upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not a long int: %s", + __func__, upsname, var, node->val); + ok = 0; + } + } + + if (!ok && !(node->flags & ST_FLAG_NUMBER)) { + upsdebugx(3, "%s: assuming UPS[%s] variable %s is a STRING after all, by contents; " + "value='%s' len='%" PRIuSIZE "' aux='%ld'", + __func__, upsname, var, node->val, len, node->aux); + + sendback(client, "%s STRING:%ld\n", buf, node->aux); + return; + } + + if (!ok) { + /* FIXME: Should this return an error? + * Value was explicitly flagged as a NUMBER but is not by content. + * Note that state_addinfo() does not sanity-check; but + * netset.c::set_var() does though (for protocol clients). + */ + upslogx(LOG_WARNING, "%s: UPS[%s] variable %s is flagged as a NUMBER but is not " + "by contents (please report as a bug to NUT project)): %s", + __func__, upsname, var, node->val); + } + } + sendback(client, "%s NUMBER\n", buf); } From f73ef0674d9053b58ff0c14dc2646e426a73da71 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 22 Jul 2024 20:06:33 +0200 Subject: [PATCH 04/12] NEWS.adoc, UPGRADING.adoc: document changes of handling for IMMUTABLE, STRING and NUMBER flags [#266] Signed-off-by: Jim Klimov --- NEWS.adoc | 5 +++++ UPGRADING.adoc | 10 ++++++++++ docs/nut.dict | 4 +++- 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/NEWS.adoc b/NEWS.adoc index d3bd184fa1..2bab34b76a 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -137,6 +137,11 @@ https://github.com/networkupstools/nut/milestone/11 * `upsd_cleanup()` is now traced, to more easily see that the daemon is exiting (and/or start-up has aborted due to configuration or run-time issues). Warning about "world readable" files clarified. [#2417] + * sanity-check `NUMBER` values that they actually resolve as a `float` + or `long int`; report an un-flagged type as `STRING` if not (previously + blindly defaulted to `NUMBER` always). Debug-print presence of the + `IMMUTABLE` flag in `netget.c::get_type()`, and actually consider it + in the `netset.c::set_var()` method to abort early. [#266] - nut-scanner: * the tool relies on dynamic loading of shared objects (library files) diff --git a/UPGRADING.adoc b/UPGRADING.adoc index 908ce287e9..ece5e7ad83 100644 --- a/UPGRADING.adoc +++ b/UPGRADING.adoc @@ -66,6 +66,16 @@ Changes from 2.8.2 to 2.8.3 and one structure (`nutscan_ipmi_t`) updated in a (hopefully) backwards compatible manner. [PR #2523, issue #2244 and numerous PRs for it] +- The `netset.c::set_var()` was updated to consider the `IMMUTABLE` flag on + values to reject writing into them quickly. This is currently expected + to only impact `override.*` values in vanilla NUT codebase. [#266] + +- The `netset.c::get_type()` was updated to sanity-check `NUMBER`-flagged + values that the strings actually resolve into `float` or `long int` + values. Some codebase does not flag its values properly and warnings + can be emitted (to be reported for upstream NUT to fix these use-cases). + [#266] + - Internal API change for `sendsignalpid()` and `sendsignalfn()` methods, which can impact NUT forks which build using `libcommon.la` and similar libraries. Added new last argument with `const char *progname` (may be diff --git a/docs/nut.dict b/docs/nut.dict index 8a0c0b2b5b..d55e1313b9 100644 --- a/docs/nut.dict +++ b/docs/nut.dict @@ -1,4 +1,4 @@ -personal_ws-1.1 en 3197 utf-8 +personal_ws-1.1 en 3199 utf-8 AAC AAS ABI @@ -2380,8 +2380,10 @@ nd nds netcat netclient +netget netmask netserver +netset netsh netsnmp netvision From 1820cfa349f7dfb67eaf4465d9c3f8664d399346 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 23 Jul 2024 01:06:12 +0200 Subject: [PATCH 05/12] drivers/dstate.c, common/state.c: allow to dump and receive "IMMUTABLE" flag value [#266] Signed-off-by: Jim Klimov --- common/state.c | 5 +++++ drivers/dstate.c | 3 +++ 2 files changed, 8 insertions(+) diff --git a/common/state.c b/common/state.c index 51eb8db6ff..aee3175df5 100644 --- a/common/state.c +++ b/common/state.c @@ -536,6 +536,11 @@ void state_setflags(st_tree_t *root, const char *var, size_t numflags, char **fl for (i = 0; i < numflags; i++) { + if (!strcasecmp(flag[i], "IMMUTABLE")) { + sttmp->flags |= ST_FLAG_IMMUTABLE; + continue; + } + if (!strcasecmp(flag[i], "RW")) { sttmp->flags |= ST_FLAG_RW; continue; diff --git a/drivers/dstate.c b/drivers/dstate.c index 59ef0c94d4..5ed9bae489 100644 --- a/drivers/dstate.c +++ b/drivers/dstate.c @@ -638,6 +638,9 @@ static int st_tree_dump_conn(st_tree_t *node, conn_t *conn) /* build the list */ snprintf(flist, sizeof(flist), "%s", node->var); + if (node->flags & ST_FLAG_IMMUTABLE) { + snprintfcat(flist, sizeof(flist), " IMMUTABLE"); + } if (node->flags & ST_FLAG_RW) { snprintfcat(flist, sizeof(flist), " RW"); } From 5776fb2f830a9fc8db7b8fc597e1bc1e9f291866 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 23 Jul 2024 01:09:18 +0200 Subject: [PATCH 06/12] server/netget.c: get_type(): use str_to_*_strict() methods instead of reinventing the wheel [#266, #966] Signed-off-by: Jim Klimov --- server/netget.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/server/netget.c b/server/netget.c index bd008cfcd7..c960bd430b 100644 --- a/server/netget.c +++ b/server/netget.c @@ -178,17 +178,18 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) /* Sanity-check current contents */ if (node->val && *(node->val)) { - char *s; - float f; + char *s = NULL; + double d; long l; - int i, ok = 1; + int ok = 1; size_t len = strlen(node->val); errno = 0; - i = sscanf(node->val, "%f", &f); - if (i < 1 || (uintmax_t)i > (uintmax_t)(len + 1) || errno || *(s = node->val + i) != '\0') { - upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not a float: %s", + if (!str_to_double_strict(node->val, &d, 10)) { + upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not a double: %s", __func__, upsname, var, node->val); + upsdebugx(4, "%s: val=%f len=%" PRIuSIZE " errno=%d *s=%c (0x%02x)", + __func__, d, len, errno, s ? *s : 'z', s ? *s : 255); ok = 0; } @@ -196,10 +197,11 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) /* did not parse as a float... range issues or NaN? */ errno = 0; ok = 1; - i = sscanf(node->val, "%ld", &l); - if (i < 1 || (uintmax_t)i > (uintmax_t)(len + 1) || errno || *(s = node->val + i) != '\0') { + if (!str_to_long_strict(node->val, &l, 10)) { upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not a long int: %s", __func__, upsname, var, node->val); + upsdebugx(4, "%s: val=%ld len=%" PRIuSIZE " errno=%d *s=%c (0x%02x)", + __func__, l, len, errno, s ? *s : '*', s ? *s : 255); ok = 0; } } From 7fb4afe7f08dad796bb6f9ab63b9578b63837529 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 23 Jul 2024 01:23:22 +0200 Subject: [PATCH 07/12] server/netget.c: get_type(): simplify logging and vars used [#266] Signed-off-by: Jim Klimov --- server/netget.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/server/netget.c b/server/netget.c index c960bd430b..d9e9799a57 100644 --- a/server/netget.c +++ b/server/netget.c @@ -178,7 +178,6 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) /* Sanity-check current contents */ if (node->val && *(node->val)) { - char *s = NULL; double d; long l; int ok = 1; @@ -186,10 +185,10 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) errno = 0; if (!str_to_double_strict(node->val, &d, 10)) { - upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not a double: %s", + upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not (exclusively) a double: %s", __func__, upsname, var, node->val); - upsdebugx(4, "%s: val=%f len=%" PRIuSIZE " errno=%d *s=%c (0x%02x)", - __func__, d, len, errno, s ? *s : 'z', s ? *s : 255); + upsdebug_with_errno(4, "%s: val=%f len=%" PRIuSIZE, + __func__, d, len); ok = 0; } @@ -198,10 +197,10 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) errno = 0; ok = 1; if (!str_to_long_strict(node->val, &l, 10)) { - upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not a long int: %s", + upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not (exclusively) a long int: %s", __func__, upsname, var, node->val); - upsdebugx(4, "%s: val=%ld len=%" PRIuSIZE " errno=%d *s=%c (0x%02x)", - __func__, l, len, errno, s ? *s : '*', s ? *s : 255); + upsdebug_with_errno(4, "%s: val=%ld len=%" PRIuSIZE, + __func__, l, len); ok = 0; } } From a90432d2d5e39ae3c18fa76e061f9bb2c37688bc Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 23 Jul 2024 01:31:46 +0200 Subject: [PATCH 08/12] NEWS.adoc, UPGRADING.adoc: clarify that PR #266 adds IMMUTABLE flag propagation on socket protocol (and fix float=>double) Signed-off-by: Jim Klimov --- NEWS.adoc | 7 +++++-- UPGRADING.adoc | 7 +++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/NEWS.adoc b/NEWS.adoc index 2bab34b76a..d8ffdd7b68 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -137,11 +137,14 @@ https://github.com/networkupstools/nut/milestone/11 * `upsd_cleanup()` is now traced, to more easily see that the daemon is exiting (and/or start-up has aborted due to configuration or run-time issues). Warning about "world readable" files clarified. [#2417] - * sanity-check `NUMBER` values that they actually resolve as a `float` + * sanity-check `NUMBER` values that they actually resolve as a `double` or `long int`; report an un-flagged type as `STRING` if not (previously blindly defaulted to `NUMBER` always). Debug-print presence of the `IMMUTABLE` flag in `netget.c::get_type()`, and actually consider it - in the `netset.c::set_var()` method to abort early. [#266] + in the `netset.c::set_var()` method to abort early. Socket protocol + used between a driver and data server to exchange device state was + updated to pass `IMMUTABLE` flag, but this change should not impact + the NUT network protocol as published in RFC 9271. [#266] - nut-scanner: * the tool relies on dynamic loading of shared objects (library files) diff --git a/UPGRADING.adoc b/UPGRADING.adoc index ece5e7ad83..35ebf31c3f 100644 --- a/UPGRADING.adoc +++ b/UPGRADING.adoc @@ -68,10 +68,13 @@ Changes from 2.8.2 to 2.8.3 - The `netset.c::set_var()` was updated to consider the `IMMUTABLE` flag on values to reject writing into them quickly. This is currently expected - to only impact `override.*` values in vanilla NUT codebase. [#266] + to only impact `override.*` values in vanilla NUT codebase, and should + not impact the NUT network protocol as published in RFC 9271 (only the + socket protocol used between a driver and data server to exchange device + state). [#266] - The `netset.c::get_type()` was updated to sanity-check `NUMBER`-flagged - values that the strings actually resolve into `float` or `long int` + values that the strings actually resolve into `double` or `long int` values. Some codebase does not flag its values properly and warnings can be emitted (to be reported for upstream NUT to fix these use-cases). [#266] From a47ceb561485022a902788d142aff8246f394a82 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 23 Jul 2024 01:32:08 +0200 Subject: [PATCH 09/12] docs/sock-protocol.txt: update with propagation of IMMUTABLE flag [#266] Signed-off-by: Jim Klimov --- docs/sock-protocol.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/sock-protocol.txt b/docs/sock-protocol.txt index b92de60910..ca9b1f968a 100644 --- a/docs/sock-protocol.txt +++ b/docs/sock-protocol.txt @@ -112,7 +112,7 @@ flags are supported. Also note that they are not crammed together in This also replaces any previous flags for a given variable. -Currently supported flags include `RW`, `STRING` and `NUMBER` +Currently supported flags include `IMMUTABLE`, `RW`, `STRING` and `NUMBER` (detailed in the NUT Network Protocol documentation); unrecognized values are quietly ignored. From 3e5d04e7e68e3a0532190ee8851d392aa45273bc Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 23 Jul 2024 01:51:08 +0200 Subject: [PATCH 10/12] server/netget.c: do not actually report a STRING:N value so far [#266] Signed-off-by: Jim Klimov --- server/netget.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/server/netget.c b/server/netget.c index d9e9799a57..55d9f669a7 100644 --- a/server/netget.c +++ b/server/netget.c @@ -205,6 +205,15 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) } } +#if DEBUG + /* Need to figure out an "aux" value here (length of current + * string at least?) and propagate the flag into where netset + * would see it. Maybe this sanity-check should move into the + * core state.c logic, so dstate setting would already remember + * the defaulted flag (and maybe set another to clarify it is + * a guess). Currently that code does not concern itself with + * sanity-checks, it seems! + */ if (!ok && !(node->flags & ST_FLAG_NUMBER)) { upsdebugx(3, "%s: assuming UPS[%s] variable %s is a STRING after all, by contents; " "value='%s' len='%" PRIuSIZE "' aux='%ld'", @@ -213,6 +222,7 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) sendback(client, "%s STRING:%ld\n", buf, node->aux); return; } +#endif if (!ok) { /* FIXME: Should this return an error? From 38a675c2f4113222125b5204f26766fe65a03357 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 23 Jul 2024 02:21:58 +0200 Subject: [PATCH 11/12] server/netget.c: get_type(): reword logged messages [#266] Signed-off-by: Jim Klimov --- server/netget.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/netget.c b/server/netget.c index 55d9f669a7..c49510e276 100644 --- a/server/netget.c +++ b/server/netget.c @@ -185,7 +185,7 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) errno = 0; if (!str_to_double_strict(node->val, &d, 10)) { - upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not (exclusively) a double: %s", + upsdebugx(3, "%s: UPS[%s] variable %s is flagged a NUMBER but not (exclusively) a double: %s", __func__, upsname, var, node->val); upsdebug_with_errno(4, "%s: val=%f len=%" PRIuSIZE, __func__, d, len); @@ -197,7 +197,7 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) errno = 0; ok = 1; if (!str_to_long_strict(node->val, &l, 10)) { - upsdebugx(3, "%s: UPS[%s] variable %s is a NUMBER but not (exclusively) a long int: %s", + upsdebugx(3, "%s: UPS[%s] variable %s is flagged a NUMBER but not (exclusively) a long int: %s", __func__, upsname, var, node->val); upsdebug_with_errno(4, "%s: val=%ld len=%" PRIuSIZE, __func__, l, len); From 7e7e70b55e5cccb08f40453bbdc0a909e2714aa8 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Tue, 23 Jul 2024 02:34:29 +0200 Subject: [PATCH 12/12] server/netget.c: fix use of "#if DEBUG" when it is not defined [#266] Signed-off-by: Jim Klimov --- server/netget.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/netget.c b/server/netget.c index c49510e276..93856a841a 100644 --- a/server/netget.c +++ b/server/netget.c @@ -140,7 +140,7 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) snprintf(buf, sizeof(buf), "TYPE %s %s", upsname, var); if (node->flags & ST_FLAG_IMMUTABLE) { -#if DEBUG +#if defined DEBUG && DEBUG /* Properly exposing this needs also an update to * docs/net-protocol.txt (promote the paragraph * provided as a note currently) and to the NUT RFC @@ -205,7 +205,7 @@ static void get_type(nut_ctype_t *client, const char *upsname, const char *var) } } -#if DEBUG +#if defined DEBUG && DEBUG /* Need to figure out an "aux" value here (length of current * string at least?) and propagate the flag into where netset * would see it. Maybe this sanity-check should move into the