parseconf: accept literal hashes inside double quotes

NEWS.adoc

@@ -447,10 +447,16 @@ https://github.com/networkupstools/nut/milestone/13
       not do so for single-driver runs -- addressed with this release. [#3302]
 
  - common code:
+    * The configuration and protocol parser now accepts a literal `#`
+      inside double quotes, including in UPS and `dummy-ups` outlet
+      descriptions. Unquoted `#` still starts a comment, and escaped
+      hashes remain supported. Encoded output continues to escape hashes
+      for older peers; default `upsc` text output is unchanged.
+      [issues #607, #1306, PR #3633]
     * The common state store now retains value changes which differ only in
       letter case, including updates to opaque strings such as `device.contact`.
       Variable names remain case-insensitive and immutable values remain
-      protected from changes. [issue #3622]
+      protected from changes. [issue #3622, PR #3630]
     * POSIX daemons now warn if their real or effective UID remains zero after
       the common credential switch. [issue #3471, PR #3609]
     * Refactored `common::background()` method used by numerous NUT daemons

UPGRADING.adoc

@@ -41,12 +41,19 @@ Changes from 2.8.5 to 2.8.6
 
 - PLANNED: Keep track of any further API clean-up?
 
+- The common configuration and protocol parser now accepts literal `#`
+  characters inside double quotes. An unescaped `#` outside double quotes
+  still starts a comment. Existing `\#` escapes remain supported and are
+  still needed when sharing configuration files with older NUT versions.
+  Encoded protocol output continues to escape hashes for older peers;
+  default `upsc` text output is unchanged. [issues #607, #1306, PR #3633]
+
 - Correct malformed numeric values in `upsd.conf` when upgrading: `MAXAGE`,
   `TRACKINGDELAY`, `MAXCONN` and numeric `CERTREQUEST` values are now checked
   in full and must fit their destination ranges. Values such as `15seconds`
   were previously read as `15`; these and overflowing values are now logged
-  and ignored, retaining the previous setting. Valid decimal values,
-  leading zeroes and trailing whitespace remain accepted. [issue #676]
+  and ignored, retaining the previous setting. Valid decimal values, leading
+  zeroes and trailing whitespace remain accepted. [issue #676, PR #3631]
 
 - Potentially a breaking change for C++ clients that rushed to use the new
   SSL support options in `libnutclient`: for Mozilla NSS setup, the way to

common/parseconf.c

@@ -283,26 +283,19 @@ static int findeol(PCONF_CTX_t *ctx)
 	return STATE_FINDEOL;
 }
 
-/* set up the error reporting details */
+#if 0
+/* Retained for future syntax errors; quoted hashes are now valid. */
 static void pconf_seterr(PCONF_CTX_t *ctx, const char *errmsg)
 {
 	snprintf(ctx->errmsg, PCONF_ERR_LEN, "%s", errmsg);
 
 	ctx->error = 1;
 }
+#endif
 
 /* quote characters inside a word bounded by "quotes" */
 static int quotecollect(PCONF_CTX_t *ctx)
 {
-	/* user is trying to break us */
-	if (ctx->ch == '#') {
-		pconf_seterr(ctx, "Unbalanced word due to unescaped # in quotes");
-		endofword(ctx);
-
-		/* this makes us drop all the way out of the caller */
-		return STATE_PARSEERR;
-	}
-
 	/* another " means we're done with this word */
 	if (ctx->ch == '"') {
 		endofword(ctx);

docs/config-notes.txt

@@ -35,8 +35,10 @@ Details about the configuration files
 Generalities
 ~~~~~~~~~~~~
 
-All configuration files within this package are parsed with a common
-state machine, which means they all can use a number of extras described here.
+Most configuration files within this package are parsed with a common
+state machine, which means they can use a number of extras described here.
+The service configuration file linkman:nut.conf[5] has its own syntax
+requirements, described in its manual page.
 
 First, most of the programs use an upper-case word to declare a
 configuration directive.  This may be something like MONITOR, NOTIFYCMD,
@@ -45,13 +47,23 @@ or ACCESS.  The case does matter here.  "monitor" won't be recognized.
 Next, the parser does not care about whitespace between words.  If you
 like to indent things with tabs or spaces, feel free to do it here.
 
-If you need to set a value to something containing spaces, it has to be
-contained within "quotes" to keep the parser from splitting up the line.
+If you need to set a value to something containing spaces, put it within
+double quotes or escape each space with a backslash to keep the parser
+from splitting up the line.
 That is, you want to use something like this:
 
 	SHUTDOWNCMD "/sbin/shutdown -h +0"
 
-Without the quotes, it would only see the first word on the line.
+or, with each space escaped:
+
+	SHUTDOWNCMD /sbin/shutdown\ -h\ +0
+
+Without quoting or escaping the spaces, a directive expecting one value
+would only see the first word of that value.
+
+Double quotes must begin at the start of a token. A closing double quote
+completes the token: `"one"two` yields two tokens, `one` and `two`.
+Single quotes (apostrophes) are ordinary characters and do not group words.
 
 OK, so let's say you really need to embed that kind of quote within your
 configuration directive for some reason.  You can do that too.
@@ -65,27 +77,35 @@ string, you just escape it.
 
 	NOTIFYCMD "/bin/notifyme c:\\dos\\style\\path"
 
-The `\` can actually be used to escape any character, but you only really
-need it for `\`, `"`, and `#` as they have special meanings to the parser.
+The `\` can actually be used to escape any permitted character, inside or
+outside double quotes. It quotes the next character once: `\n` means the
+letter `n`, not a newline. It is useful for `\`, `"`, and, outside double
+quotes, spaces, `=`, and `#`, which have special meanings to the parser.
 
 When using file names with space characters, you may end up having tricky
 things since you need to write them inside `""` which must be escaped:
 
 	NOTIFYCMD "\"c:\\path with space\\notifyme\" \"c:\\path with space\\name\""
 
-`#` is the comment character.  Anything after an unescaped `#` is ignored.
+Outside double quotes, `#` is the comment character. Anything after an
+unescaped `#` is ignored until the end of the physical line.
 
 Something like this...
 
 	identity = my#1ups
 
 will actually turn into `identity = my`, since the `#` stops the
 parsing. If you really need to have a `#` in your configuration, then
-escape it.
+escape it or put the value inside double quotes.
 
 	identity = my\#1ups
 
-Much better.
+or:
+
+	identity = "my#1ups"
+
+Older versions of the parser required `\#` even inside double quotes.
+That spelling remains supported for compatibility with those versions.
 
 The `=` character should be used with care too. There should be only one
 "simple" `=` character in a line: between the parameter name and its value.

docs/man/dummy-ups.txt

@@ -141,8 +141,38 @@ This definition file, specified by the `port` argument in the example above,
 is generally named `something.dev` or `something.seq`. It contains a list of
 all valid variables and associated values (you can later use `upsrw`
 only to modify values of these variables), and has the same format as an
-linkman:upsc[8] data dump (`<varname>: <value>`). This means you can easily
-create definition files from an existing UPS using `upsc > file.dev`.
+linkman:upsc[8] data dump (`<varname>: <value>`). You can use
+`upsc > file.dev` as a starting point for a definition file.
+
+Values are read with the common NUT configuration parser. Unquoted value
+tokens are joined with a single space; double quotes preserve spaces
+inside a token. Single quotes (apostrophes) are literal characters.
+An unescaped `#` outside double quotes starts a comment, while a `#`
+inside double quotes is literal. For example:
+
+	outlet.1.desc: Outlet #1
+	outlet.2.desc: "Outlet \#2"
+	outlet.3.desc: "Outlet #3"
+	outlet.4.desc: Outlet \#4
+
+These values become `Outlet`, `Outlet #2`, `Outlet #3`, and `Outlet #4`,
+respectively. The last example has two value tokens, `Outlet` and `#4`,
+which *dummy-ups* joins with a space. Escaping that space makes one token
+with the same value:
+
+	outlet.4.desc: Outlet\ \#4
+
+Older versions of the parser require `\#` even inside
+double quotes. Backslashes quote the next permitted character once, so
+`\n` becomes `n`, while `\\` becomes a single literal backslash.
+A backslash followed by a physical newline continues the logical line.
+An unescaped `=` outside double quotes is a separate token; quote or
+escape it when it is part of a value.
+
+The default `upsc` output does not add quoting or escaping. Before reading
+such a dump back with *dummy-ups*, quote or escape values containing these
+special characters as needed. Its JSON output preserves string values
+unambiguously, but JSON is not a *dummy-ups* definition file format.
 
 Note that the Network UPS project provides an extensive
 link:https://www.networkupstools.org/ddl/index.html[DDL (Devices Dumps Library)]

docs/net-protocol.txt

@@ -67,6 +67,24 @@ be interpreted with parseconf (NUT parser) or something similar.
 It is an ASCII text-based line-oriented protocol, with end of each line
 signaled by a single `\n` (`0x0D`) character.
 
+Double quotes start a quoted element only at the beginning of an element.
+The closing quote completes that element, so `"one"two` produces two
+elements, `one` and `two`. Single quotes (apostrophes) are ordinary
+characters; they do not group words. Outside double quotes, unescaped
+whitespace separates elements, and an unescaped `=` becomes a separate
+element of its own.
+
+An unescaped `#` starts a comment only outside double quotes. Inside
+double quotes it is literal, so `"Outlet #3"` represents `Outlet #3`.
+For compatibility with older parsers which rejected an unescaped `#`
+inside quotes, `pconf_encode()` continues to send it as `\#`.
+
+A backslash quotes the next character once, inside or outside double
+quotes, subject to the character restrictions of the parser. For example,
+`one\ two` is one element, while `\n` represents the letter `n`, not a
+newline. A backslash followed by a physical newline continues the logical
+line. These rules are NUT syntax, not shell or JSON string syntax.
+
 
 Revision history
 ----------------

tests/Makefile.am

@@ -100,6 +100,10 @@ nodist_test_cgilib_SOURCES = cgilib.c
 test_cgilib_CFLAGS = $(AM_CFLAGS) -I$(top_srcdir)/clients
 test_cgilib_LDADD = $(NUT_LIBCOMMON)
 
+TESTS += parseconf-test
+parseconf_test_SOURCES = parseconf-test.c
+parseconf_test_LDADD = $(NUT_LIBCOMMON)
+
 TESTS += test_upsdconf
 test_upsdconf_SOURCES = test_upsdconf.c
 test_upsdconf_CFLAGS = $(AM_CFLAGS) -I$(top_srcdir)/server

tests/NIT/Makefile.am

@@ -10,6 +10,7 @@
 @NUT_AM_MAKE_CAN_EXPORT@@NUT_AM_EXPORT_CCACHE_PATH@export PATH=@PATH_DURING_CONFIGURE@
 
 EXTRA_DIST = nit.sh README.adoc
+EXTRA_DIST += parseconf-test.py
 
 if WITH_CHECK_NIT
 check: check-NIT

tests/NIT/nit.sh

@@ -24,6 +24,7 @@
 #	NUT_DEBUG_LEVEL_UPSSCHED=3	to set debug level for particular
 #			NUT daemons or tools
 #	NUT_PORT=12345	custom port for upsd to listen and clients to query
+#	NIT_PARSECONF_TIMEOUT=60	per-query watchdog seconds for parser tests
 #	NUT_FOREGROUND_WITH_PID=true	default foregrounding is without
 #			PID files, this option tells daemons to save them
 #	TESTDIR=/tmp/nut-NIT	to propose a location for "etc" and "run"
@@ -4759,6 +4760,64 @@ testgroup_sandbox_python() {
     sandbox_forget_configs
 }
 
+testcase_sandbox_parseconf() {
+    if ! isTestablePython || [ -z "${PYTHON}" ] \
+    || ! $PYTHON -c 'import json' >/dev/null 2>&1 ; then
+        case "${NIT_CASE}" in
+        testcase_sandbox_parseconf|testgroup_sandbox_parseconf)
+            log_error "[testcase_sandbox_parseconf] Python with the json module is required"
+            FAILED="`expr $FAILED + 1`"
+            FAILED_FUNCS="$FAILED_FUNCS testcase_sandbox_parseconf:missing-prerequisites"
+            ;;
+        *)
+            log_warn "[testcase_sandbox_parseconf] SKIPPED: Python with the json module is unavailable"
+            SKIPPED="`expr $SKIPPED + 1`"
+            SKIPPED_FUNCS="$SKIPPED_FUNCS testcase_sandbox_parseconf"
+            ;;
+        esac
+        return
+    fi
+
+    log_separator
+    log_info "[testcase_sandbox_parseconf] Check exact text and decoded JSON values with $PYTHON"
+    if (
+        # Match execcmd's per-client debug setting without changing later tests.
+        if [ -n "${NUT_DEBUG_LEVEL_UPSC-}" ]; then
+            NUT_DEBUG_LEVEL="${NUT_DEBUG_LEVEL_UPSC}"
+            export NUT_DEBUG_LEVEL
+        fi
+        $PYTHON "${TOP_SRCDIR}/tests/NIT/parseconf-test.py" \
+            "${TOP_BUILDDIR}/clients/upsc${EXEEXT}" "$NUT_PORT"
+    ) ; then
+        PASSED="`expr $PASSED + 1`"
+        log_info "[testcase_sandbox_parseconf] PASSED: exact text and decoded JSON values"
+    else
+        log_error "[testcase_sandbox_parseconf] FAILED: parser checks complained, check above"
+        FAILED="`expr $FAILED + 1`"
+        FAILED_FUNCS="$FAILED_FUNCS testcase_sandbox_parseconf"
+    fi
+}
+
+testgroup_sandbox_parseconf() {
+    # Start a fresh sandbox so these punctuation fixtures do not change
+    # the model, timer and language-binding tests' expected data.
+    if ! isTestablePython || [ -z "${PYTHON}" ] \
+    || ! $PYTHON -c 'import json' >/dev/null 2>&1 ; then
+        testcase_sandbox_parseconf
+        return
+    fi
+
+    stop_daemons
+    sandbox_forget_configs
+    sandbox_generate_configs
+    $PYTHON "${TOP_SRCDIR}/tests/NIT/parseconf-test.py" prepare "$NUT_CONFPATH" \
+        || die "[testgroup_sandbox_parseconf] Could not prepare parser fixtures"
+    testcase_sandbox_start_drivers_after_upsd
+    testcase_sandbox_parseconf
+    log_separator
+    sandbox_forget_configs
+}
+
 testgroup_sandbox_perl() {
     # Arrange for quick test iterations
     testcase_sandbox_start_drivers_after_upsd
@@ -4885,6 +4944,7 @@ case "${NIT_CASE}" in
         testgroup_upsd_invalid_configs
         testgroup_upsd_questionable_configs
         testgroup_sandbox
+        testgroup_sandbox_parseconf
         ;;
     *)  die "Unsupported NIT_CASE='$NIT_CASE' was requested" ;;
 esac