bootconfig: Preserve values and key paths during parsing and output

Hi Masami,

Thanks again for reviewing and picking up the earlier script fixes. Here are three fixes for the bootconfig parser and command-line tool, based on bootconfig/for-next.

The first fixes compact output closing a brace too early when a key has both a value and a subkey. Reading the output back can turn a.z into z.

The second trims unquoted trailing whitespace at EOF, so adding a final newline does not change the parsed value. The third keeps accepted values containing both quote types readable when the tool prints them again.

Each fix has regression coverage. I tested them independently and together on my Ubuntu machine, building the tool with -Werror and checking the parser results, compact and list output, command-line rendering, and test initrd images. The full bootconfig test suite passes after each patch as well as for the complete series.

The three fixes are independent and can be applied separately. Feedback is welcome.

Thanks,
Eduardo

Eduardo Mucelli Rezende Oliveira (3):
tools/bootconfig: Close only opened braces in compact output
bootconfig: Trim unquoted values at end of file tools/bootconfig: Preserve values containing both types of quotes

 lib/bootconfig.c                              |  5 ++--
 tools/bootconfig/main.c                       | 29 ++++++++++---------
 .../samples/exp-good-mixed-kv1.bconf          |  5 ++++
 tools/bootconfig/samples/good-mixed-kv1.bconf |  5 ++++
 tools/bootconfig/test-bootconfig.sh           | 29 +++++++++++++++++++
 5 files changed, 57 insertions(+), 16 deletions(-)

base-commit: 677f83795fdfc25ddc51b665a9111f5ff199f7ca

2.53.0

A key may have both a value and subkeys. When it has only one subkey, xbc_show_compact_tree() can print that subkey using dotted notation without opening a brace. During backtracking, however, it treats the value node followed by a subkey as evidence that a brace was opened.

For example, a.b=v, a.b.c=w and a.z=q are printed with the brace for a closed before z. Reading that output back changes a.z into the root key z, silently changing the configuration.

Remember the nodes for which opening braces were actually printed and close a brace only when backtracking through the matching node. Extend the existing mixed-value/subkey sample to cover a sibling after a valued parent with one subkey.

---
With this valid configuration:

a.b = v
a.b.c = w
a.z = q

the original compact output closes the brace for a before printing z. Reparsing it turns a.z into the root key z. Values and subkeys under the same key are allowed by the documented syntax:
https://docs.kernel.org/admin-guide/bootconfig.html#same-key-values

I checked parser round trips for valued parents, bare subkeys, arrays, deeper trees and a test initrd image. All 94 focused checks pass with the fix, independently and in the combined series. The full bootconfig test suite also passes.

 tools/bootconfig/main.c                           | 15 +++++++--------
 tools/bootconfig/samples/exp-good-mixed-kv1.bconf |  5 +++++
 tools/bootconfig/samples/good-mixed-kv1.bconf     |  5 +++++
 3 files changed, 17 insertions(+), 8 deletions(-)

index 56d76bdeaa09..1bab0e685e2d 100644
@@ -71,6 +71,7 @@ static int xbc_show_value(struct xbc_node *node, bool semicolon)
 static void xbc_show_compact_tree(void)
 {
 	struct xbc_node *node, *cnode = NULL, *vnode;
+	struct xbc_node *parent[XBC_DEPTH_MAX];
 	int depth = 0, i;
 
 	node = xbc_root_node();

@@ -100,7 +101,7 @@ static void xbc_show_compact_tree(void)
 		}
 		if (cnode && xbc_node_is_key(cnode)) {
 			printf("%s {\n", xbc_node_get_data(node));
-			depth++;
+			parent[depth++] = node;
 			node = cnode;
 			cnode = NULL;
 			continue;

@@ -128,14 +129,12 @@ static void xbc_show_compact_tree(void)
 			node = xbc_node_get_parent(node);
 			if (!node)
 				return;
-			if (!xbc_node_get_child(node)->next)
+			if (!depth || parent[depth - 1] != node)
 				continue;
-			if (depth) {
-				depth--;
-				for (i = 0; i < depth; i++)
-					printf("\t");
-				printf("}\n");
-			}
+			depth--;
+			for (i = 0; i < depth; i++)
+				printf("\t");
+			printf("}\n");
 		}
 		node = xbc_node_get_next(node);
 	}

index 8346287d9251..0b8b2f59da31 100644
@@ -1,2 +1,7 @@
 key = "value";
 key.subkey = "another-value";
+a {
+	b = "v";
+	b.c = "w";
+	z = "q";
+}

index 1761547dd05c..35442ee62b75 100644
@@ -1,3 +1,8 @@
 # value -> subkey pattern
 key = value
 key.subkey = another-value
+
+# A valued parent with one subkey must not close its enclosing brace.
+a.b = v
+a.b.c = w
+a.z = q

2.53.0

__xbc_parse_value() trims an unquoted value when it encounters a delimiter, but leaves trailing whitespace intact when it reaches the end
of the input. Thus "key = value " has a different value depending on
whether the file ends with a newline. The extra whitespace also reaches the rendered kernel command line.

Trim unquoted values after the scan finishes, including at end of input. Skip trimming after a closing quote so intentional whitespace in quoted values remains unchanged. Add checks for scalars, arrays and quoted values without a final newline.

---
I tested inputs ending in unquoted spaces, tabs and carriage returns, including arrays and appended or overridden values. The checks compare the parser's actual values and the tool's rendered kernel command line. Quoted whitespace and inputs ending in delimiters are controls.

The original parser fails nine of the 18 focused checks. With the fix, all 18 pass, independently and in the combined series. The full bootconfig test suite also passes with the regression checks added.

 lib/bootconfig.c                    |  5 +++--
 tools/bootconfig/test-bootconfig.sh | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

index 819685d6c68d..bd071c394c36 100644
@@ -793,16 +793,17 @@ static int __init __xbc_parse_value(char **__v, char **__n)
 				return xbc_parse_error("No value delimiter", p);
 			if (*p)
 				p++;
-			break;
+			goto skip_trim;
 		}
 		if (strchr(",;\n#}", c)) {
 			*p++ = '\0';
-			v = strim(v);
 			break;
 		}
 	}
 	if (quotes)
 		return xbc_parse_error("No closing quotes", p);
+	v = strim(v);
+skip_trim:
 	if (c == '#') {
 		p = skip_comment(p);
 		c = '\n';	/* A comment must be treated as a newline */

index 530ce7e28d63..a2e8311d4d19 100755
@@ -171,6 +171,24 @@ $BOOTCONF $INITRD > $OUTFILE
 xfail grep -q 'val[[:space:]]' $OUTFILE
 xpass grep -q 'val2[[:space:]]' $OUTFILE
 
+echo "Remove unquoted trailing whitespace at end of file"
+printf 'foo = val \t' > $TEMPCONF
+$BOOTCONF -l $TEMPCONF > $OUTFILE
+printf 'foo = "val"\n' > $TEMPCONF
+xpass cmp $TEMPCONF $OUTFILE
+
+echo "Remove trailing whitespace from the last array value at end of file"
+printf 'foo = first, last \t' > $TEMPCONF
+$BOOTCONF -l $TEMPCONF > $OUTFILE
+printf 'foo = "first", "last"\n' > $TEMPCONF
+xpass cmp $TEMPCONF $OUTFILE
+
+echo "Keep quoted trailing whitespace at end of file"
+printf 'foo = "val "' > $TEMPCONF
+$BOOTCONF -l $TEMPCONF > $OUTFILE
+printf 'foo = "val "\n' > $TEMPCONF
+xpass cmp $TEMPCONF $OUTFILE
+
 echo "Showing correct line:column of no closing brace"
 cat > $TEMPCONF << EOF
 foo {

2.53.0

An unquoted bootconfig value can contain both single and double quote characters, for example:

message = He said "it's fine"

xbc_show_value() surrounds any value containing a double quote with single quotes. If the value also contains a single quote, both compact and list output become invalid bootconfig syntax and cannot be parsed again.

Print values containing both quote types without outer quotes. Such values can only come from unquoted input, so they do not contain the delimiters that would require quoting. Keep using single or double quotes for the other values. Add round-trip checks for compact and list output, including an array containing the mixed-quote value.

---
The parser accepts message = He said "it's fine", but the original tool prints a value surrounded by single quotes. The apostrophe in "it's" then makes that output invalid.

I checked scalar and array values through compact and list output, including literal backslashes, quote controls, and a test initrd image. With the fix, all 25 focused checks pass, independently and in the combined series, and the parsed values survive those round trips. The full bootconfig test suite also passes.

 tools/bootconfig/main.c             | 14 ++++++++------
 tools/bootconfig/test-bootconfig.sh | 11 +++++++++++
 2 files changed, 19 insertions(+), 6 deletions(-)

index 1bab0e685e2d..7042bb5fe280 100644
@@ -52,17 +52,19 @@ static int init_xbc_with_error(char *buf, int len)
 
 static int xbc_show_value(struct xbc_node *node, bool semicolon)
 {
-	const char *val, *eol;
-	char q;
+	const char *val, *eol, *q;
 	int i = 0;
 
 	eol = semicolon ? ";\n" : "\n";
 	xbc_array_for_each_value(node, val) {
-		if (strchr(val, '"'))
-			q = '\'';
+		if (!strchr(val, '"'))
+			q = "\"";
+		else if (!strchr(val, '\''))
+			q = "'";
 		else
-			q = '"';
-		printf("%c%s%c%s", q, val, q, xbc_node_is_array(node) ? ", " : eol);
+			/* Both types of quotes can only occur in unquoted values. */
+			q = "";
+		printf("%s%s%s%s", q, val, q, xbc_node_is_array(node) ? ", " : eol);
 		i++;
 	}
 	return i;

index a2e8311d4d19..a0578013e731 100755
@@ -146,6 +146,17 @@ $BOOTCONF $INITRD > $TEMPCONF
 cat $TEMPCONF
 xpass grep \'\"string\"\' $TEMPCONF
 
+echo "Round-trip values containing both types of quotes"
+cat > $TEMPCONF << 'EOF'
+key = "first", He said "it's fine", "last"
+EOF
+for mode in "" "-l"; do
+  $BOOTCONF $mode $TEMPCONF > $OUTFILE
+  xpass $BOOTCONF -a $OUTFILE $INITRD
+  $BOOTCONF -l $INITRD > $OUTFILE
+  xpass cmp $TEMPCONF $OUTFILE
+done
+
 echo "Repeat same-key tree"
 cat > $TEMPCONF << EOF
 foo

2.53.0

Hi Masami,

Thanks again for reviewing the earlier fixes. This v2 addresses the issue Sashiko reported in the first patch.

The parent array in v1 assumed that the parser always enforced XBC_DEPTH_MAX. I reproduced the reported overflow with AddressSanitizer using a 20-component chain where each parent also has a value. The existing verifier skips the subkeys in that case, so the parser accepts a deeper tree than the formatter's array can hold.

Patch 1 now checks the key children to determine whether a node opened braces, using the same conditions as the forward walk. It no longer keeps a depth-indexed array. The existing verifier gap is a separate issue; this revision removes the formatter's dependency on that check.

Changes in v2:
- Patch 1: derive brace ownership from the key children.
- Patches 2 and 3: unchanged.

I tested this on my Ubuntu machine. All 137 focused checks pass for the combined series, and the full bootconfig suite passes after each patch (86 passed, zero failed for the complete series). I also checked the revised first patch and the combined series with AddressSanitizer and UBSan using value-bearing chains up to 95 components.

Review: https://lore.kernel.org/all/[email protected]/
V1: https://lore.kernel.org/all/[email protected]/

Feedback is welcome.

Thanks,
Eduardo

Eduardo Mucelli Rezende Oliveira (3):
tools/bootconfig: Close only opened braces in compact output
bootconfig: Trim unquoted values at end of file tools/bootconfig: Preserve values containing both types of quotes

 lib/bootconfig.c                              |  5 ++-
 tools/bootconfig/main.c                       | 43 +++++++++++++------
 .../samples/exp-good-mixed-kv1.bconf          |  5 +++
 tools/bootconfig/samples/good-mixed-kv1.bconf |  5 +++
 tools/bootconfig/test-bootconfig.sh           | 29 +++++++++++++
 5 files changed, 72 insertions(+), 15 deletions(-)

base-commit: 677f83795fdfc25ddc51b665a9111f5ff199f7ca

2.53.0

A key may have both a value and subkeys. When it has only one subkey, xbc_show_compact_tree() can print that subkey using dotted notation without opening a brace. During backtracking, however, it treats the value node followed by a subkey as evidence that a brace was opened.

For example, a.b=v, a.b.c=w and a.z=q are printed with the brace for a closed before z. Reading that output back changes a.z into the root key z, silently changing the configuration.

Check the key children when deciding whether to close a brace. Skip the value child, and account for a sole subkey that itself has both a value and subkeys, since that combination is not compacted. This follows the conditions used to open braces without keeping a depth-indexed array. Extend the mixed-value/subkey sample to cover the lost sibling.

---
Changes in v2:
- Determine whether braces were opened from the key children instead of keeping the depth-indexed parent array used in v1.
- This handles the deeper value-bearing trees currently accepted by the parser without writing beyond a fixed-size array.

Sashiko review:
https://lore.kernel.org/all/[email protected]/

With this valid configuration:

a.b = v
a.b.c = w
a.z = q

the original compact output closes the brace for a before printing z. Reparsing it turns a.z into the root key z. Values and subkeys under the same key are allowed by the documented syntax:
https://docs.kernel.org/admin-guide/bootconfig.html#same-key-values

All 94 focused round-trip checks pass with the revision, independently and in the combined series. The full bootconfig suite also passes. AddressSanitizer and UBSan checks for 16-, 17-, 20-, 32-, 64- and 95-component value-bearing chains pass for both the first patch alone and the complete series.

 tools/bootconfig/main.c                       | 29 ++++++++++++++-----
 .../samples/exp-good-mixed-kv1.bconf          |  5 ++++
 tools/bootconfig/samples/good-mixed-kv1.bconf |  5 ++++
 3 files changed, 32 insertions(+), 7 deletions(-)

index 56d76bdeaa09..75516e9e4ac3 100644
@@ -68,6 +68,23 @@ static int xbc_show_value(struct xbc_node *node, bool semicolon)
 	return i;
 }
 
+/* Match the conditions under which the compact formatter opens braces. */
+static bool xbc_node_needs_braces(struct xbc_node *node)
+{
+	struct xbc_node *child = xbc_node_get_child(node);
+
+	if (child && xbc_node_is_value(child))
+		child = xbc_node_get_next(child);
+	if (!child)
+		return false;
+	if (child->next)
+		return true;
+
+	/* A single subkey with both a value and subkeys is not compacted. */
+	child = xbc_node_get_child(child);
+	return child && xbc_node_is_value(child) && child->next;
+}
+
 static void xbc_show_compact_tree(void)
 {
 	struct xbc_node *node, *cnode = NULL, *vnode;

@@ -128,14 +145,12 @@ static void xbc_show_compact_tree(void)
 			node = xbc_node_get_parent(node);
 			if (!node)
 				return;
-			if (!xbc_node_get_child(node)->next)
+			if (!depth || !xbc_node_needs_braces(node))
 				continue;
-			if (depth) {
-				depth--;
-				for (i = 0; i < depth; i++)
-					printf("\t");
-				printf("}\n");
-			}
+			depth--;
+			for (i = 0; i < depth; i++)
+				printf("\t");
+			printf("}\n");
 		}
 		node = xbc_node_get_next(node);
 	}

index 8346287d9251..0b8b2f59da31 100644
@@ -1,2 +1,7 @@
 key = "value";
 key.subkey = "another-value";
+a {
+	b = "v";
+	b.c = "w";
+	z = "q";
+}

index 1761547dd05c..35442ee62b75 100644
@@ -1,3 +1,8 @@
 # value -> subkey pattern
 key = value
 key.subkey = another-value
+
+# A valued parent with one subkey must not close its enclosing brace.
+a.b = v
+a.b.c = w
+a.z = q

2.53.0

__xbc_parse_value() trims an unquoted value when it encounters a delimiter, but leaves trailing whitespace intact when it reaches the end
of the input. Thus "key = value " has a different value depending on
whether the file ends with a newline. The extra whitespace also reaches the rendered kernel command line.

Trim unquoted values after the scan finishes, including at end of input. Skip trimming after a closing quote so intentional whitespace in quoted values remains unchanged. Add checks for scalars, arrays and quoted values without a final newline.

---
I tested inputs ending in unquoted spaces, tabs and carriage returns, including arrays and appended or overridden values. The checks compare the parser's actual values and the tool's rendered kernel command line. Quoted whitespace and inputs ending in delimiters are controls.

The original parser fails nine of the 18 focused checks. With the fix, all 18 pass, independently and in the combined series. The full bootconfig test suite also passes with the regression checks added.

 lib/bootconfig.c                    |  5 +++--
 tools/bootconfig/test-bootconfig.sh | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

index 819685d6c68d..bd071c394c36 100644
@@ -793,16 +793,17 @@ static int __init __xbc_parse_value(char **__v, char **__n)
 				return xbc_parse_error("No value delimiter", p);
 			if (*p)
 				p++;
-			break;
+			goto skip_trim;
 		}
 		if (strchr(",;\n#}", c)) {
 			*p++ = '\0';
-			v = strim(v);
 			break;
 		}
 	}
 	if (quotes)
 		return xbc_parse_error("No closing quotes", p);
+	v = strim(v);
+skip_trim:
 	if (c == '#') {
 		p = skip_comment(p);
 		c = '\n';	/* A comment must be treated as a newline */

index 530ce7e28d63..a2e8311d4d19 100755
@@ -171,6 +171,24 @@ $BOOTCONF $INITRD > $OUTFILE
 xfail grep -q 'val[[:space:]]' $OUTFILE
 xpass grep -q 'val2[[:space:]]' $OUTFILE
 
+echo "Remove unquoted trailing whitespace at end of file"
+printf 'foo = val \t' > $TEMPCONF
+$BOOTCONF -l $TEMPCONF > $OUTFILE
+printf 'foo = "val"\n' > $TEMPCONF
+xpass cmp $TEMPCONF $OUTFILE
+
+echo "Remove trailing whitespace from the last array value at end of file"
+printf 'foo = first, last \t' > $TEMPCONF
+$BOOTCONF -l $TEMPCONF > $OUTFILE
+printf 'foo = "first", "last"\n' > $TEMPCONF
+xpass cmp $TEMPCONF $OUTFILE
+
+echo "Keep quoted trailing whitespace at end of file"
+printf 'foo = "val "' > $TEMPCONF
+$BOOTCONF -l $TEMPCONF > $OUTFILE
+printf 'foo = "val "\n' > $TEMPCONF
+xpass cmp $TEMPCONF $OUTFILE
+
 echo "Showing correct line:column of no closing brace"
 cat > $TEMPCONF << EOF
 foo {

2.53.0

An unquoted bootconfig value can contain both single and double quote characters, for example:

message = He said "it's fine"

xbc_show_value() surrounds any value containing a double quote with single quotes. If the value also contains a single quote, both compact and list output become invalid bootconfig syntax and cannot be parsed again.

Print values containing both quote types without outer quotes. Such values can only come from unquoted input, so they do not contain the delimiters that would require quoting. Keep using single or double quotes for the other values. Add round-trip checks for compact and list output, including an array containing the mixed-quote value.

---
The parser accepts message = He said "it's fine", but the original tool prints a value surrounded by single quotes. The apostrophe in "it's" then makes that output invalid.

I checked scalar and array values through compact and list output, including literal backslashes, quote controls, and a test initrd image. With the fix, all 25 focused checks pass, independently and in the combined series, and the parsed values survive those round trips. The full bootconfig test suite also passes.

 tools/bootconfig/main.c             | 14 ++++++++------
 tools/bootconfig/test-bootconfig.sh | 11 +++++++++++
 2 files changed, 19 insertions(+), 6 deletions(-)

index 75516e9e4ac3..e7513742fd2f 100644
@@ -52,17 +52,19 @@ static int init_xbc_with_error(char *buf, int len)
 
 static int xbc_show_value(struct xbc_node *node, bool semicolon)
 {
-	const char *val, *eol;
-	char q;
+	const char *val, *eol, *q;
 	int i = 0;
 
 	eol = semicolon ? ";\n" : "\n";
 	xbc_array_for_each_value(node, val) {
-		if (strchr(val, '"'))
-			q = '\'';
+		if (!strchr(val, '"'))
+			q = "\"";
+		else if (!strchr(val, '\''))
+			q = "'";
 		else
-			q = '"';
-		printf("%c%s%c%s", q, val, q, xbc_node_is_array(node) ? ", " : eol);
+			/* Both types of quotes can only occur in unquoted values. */
+			q = "";
+		printf("%s%s%s%s", q, val, q, xbc_node_is_array(node) ? ", " : eol);
 		i++;
 	}
 	return i;

index a2e8311d4d19..a0578013e731 100755
@@ -146,6 +146,17 @@ $BOOTCONF $INITRD > $TEMPCONF
 cat $TEMPCONF
 xpass grep \'\"string\"\' $TEMPCONF
 
+echo "Round-trip values containing both types of quotes"
+cat > $TEMPCONF << 'EOF'
+key = "first", He said "it's fine", "last"
+EOF
+for mode in "" "-l"; do
+  $BOOTCONF $mode $TEMPCONF > $OUTFILE
+  xpass $BOOTCONF -a $OUTFILE $INITRD
+  $BOOTCONF -l $INITRD > $OUTFILE
+  xpass cmp $TEMPCONF $OUTFILE
+done
+
 echo "Repeat same-key tree"
 cat > $TEMPCONF << EOF
 foo

2.53.0