Merge 9a59218 into dfe6062

Author: jimklimov

NEWS.adoc

@@ -68,8 +68,16 @@ https://github.com/networkupstools/nut/milestone/9
      for this name": the way to extract IP address string was not portable
      and misfired on some platforms, and the way to print had a theoretical
      potential for buffer overflow. [#2915]
-
- - `usbhid-ups` updates:
+   * 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. 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. [issue #266, PR #2549]
+
+ - `usbhid-ups` driver updates:
    * The `cps-hid` subdriver's existing mechanism for fixing broken report
      descriptors was extended to cover a newly reported case of nominal UPS
      power being incorrectly reported due to an unrealistically low maximum

UPGRADING.adoc

@@ -26,6 +26,19 @@ Changes from 2.8.3 to 2.8.4
 
 - PLANNED: Keep track of any further API clean-up?
 
+- 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, 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). [issue #266, PR #2549]
+
+- The `netset.c::get_type()` was updated to sanity-check `NUMBER`-flagged
+  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).
+  [issue #266, PR #2549]
+
 Changes from 2.8.2 to 2.8.3
 ---------------------------
 

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;

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.
 

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3470 utf-8
+personal_ws-1.1 en 3472 utf-8
 AAC
 AAS
 ABI
@@ -2568,8 +2568,10 @@ nde
 nds
 netcat
 netclient
+netget
 netmask
 netserver
+netset
 netsh
 netsnmp
 netvision

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.
 

drivers/dstate.c

@@ -646,6 +646,9 @@ static int st_tree_dump_conn_one_node(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");
 		}

server/netget.c

@@ -24,6 +24,7 @@
 #include "state.h"
 #include "desc.h"
 #include "neterr.h"
+#include "nut_stdint.h"
 
 #include "netget.h"
 
@@ -138,6 +139,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 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
+		 * 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");
 
@@ -162,6 +176,66 @@ 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)) {
+		double	d;
+		long	l;
+		int	ok = 1;
+		size_t	len = strlen(node->val);
+
+		errno = 0;
+		if (!str_to_double_strict(node->val, &d, 10)) {
+			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);
+			ok = 0;
+		}
+
+		if (!ok) {
+			/* did not parse as a float... range issues or NaN? */
+			errno = 0;
+			ok = 1;
+			if (!str_to_long_strict(node->val, &l, 10)) {
+				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);
+				ok = 0;
+			}
+		}
+
+#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
+		 * 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'",
+				__func__, upsname, var, node->val, len, node->aux);
+
+			sendback(client, "%s STRING:%ld\n", buf, node->aux);
+			return;
+		}
+#endif
+
+		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);
 }
 

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\"",