lib/bootconfig: three bug fixes

[PATCH v2 1/3] lib/bootconfig: fix off-by-one in xbc_verify_tree() unclosed brace error

Posted by Josh Law 3 weeks, 4 days ago

From: Josh Law <objecting@objecting.org>

__xbc_open_brace() pushes entries with post-increment
(open_brace[brace_index++]), so brace_index always points one past
the last valid entry.  xbc_verify_tree() reads open_brace[brace_index]
to report which brace is unclosed, but this is one past the last
pushed entry and contains stale/zero data, causing the error message
to reference the wrong node.

Use open_brace[brace_index - 1] to correctly identify the unclosed
brace.  brace_index is known to be > 0 here since we are inside the
if (brace_index) guard.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 lib/bootconfig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 2bcd5c2aa87e..a1e6a2e14b01 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -802,7 +802,7 @@ static int __init xbc_verify_tree(void)
 
 	/* Brace closing */
 	if (brace_index) {
-		n = &xbc_nodes[open_brace[brace_index]];
+		n = &xbc_nodes[open_brace[brace_index - 1]];
 		return xbc_parse_error("Brace is not closed",
 					xbc_node_get_data(n));
 	}
-- 
2.34.1

Re: [PATCH v2 1/3] lib/bootconfig: fix off-by-one in xbc_verify_tree() unclosed brace error

Posted by Steven Rostedt 3 weeks, 4 days ago

On Thu, 12 Mar 2026 19:11:41 +0000
Josh Law <hlcj1234567@gmail.com> wrote:

> From: Josh Law <objecting@objecting.org>
> 
> __xbc_open_brace() pushes entries with post-increment
> (open_brace[brace_index++]), so brace_index always points one past
> the last valid entry.  xbc_verify_tree() reads open_brace[brace_index]
> to report which brace is unclosed, but this is one past the last
> pushed entry and contains stale/zero data, causing the error message
> to reference the wrong node.
> 
> Use open_brace[brace_index - 1] to correctly identify the unclosed
> brace.  brace_index is known to be > 0 here since we are inside the
> if (brace_index) guard.
> 
> Signed-off-by: Josh Law <objecting@objecting.org>

Nice catch. May I ask how you found this.

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve

> ---
>  lib/bootconfig.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> index 2bcd5c2aa87e..a1e6a2e14b01 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -802,7 +802,7 @@ static int __init xbc_verify_tree(void)
>  
>  	/* Brace closing */
>  	if (brace_index) {
> -		n = &xbc_nodes[open_brace[brace_index]];
> +		n = &xbc_nodes[open_brace[brace_index - 1]];
>  		return xbc_parse_error("Brace is not closed",
>  					xbc_node_get_data(n));
>  	}

Re: [PATCH v2 1/3] lib/bootconfig: fix off-by-one in xbc_verify_tree() unclosed brace error

Posted by Josh Law 3 weeks, 4 days ago

12 Mar 2026 21:02:51 Steven Rostedt <rostedt@goodmis.org>:

> On Thu, 12 Mar 2026 19:11:41 +0000
> Josh Law <hlcj1234567@gmail.com> wrote:
>
>> From: Josh Law <objecting@objecting.org>
>>
>> __xbc_open_brace() pushes entries with post-increment
>> (open_brace[brace_index++]), so brace_index always points one past
>> the last valid entry.  xbc_verify_tree() reads open_brace[brace_index]
>> to report which brace is unclosed, but this is one past the last
>> pushed entry and contains stale/zero data, causing the error message
>> to reference the wrong node.
>>
>> Use open_brace[brace_index - 1] to correctly identify the unclosed
>> brace.  brace_index is known to be > 0 here since we are inside the
>> if (brace_index) guard.
>>
>> Signed-off-by: Josh Law <objecting@objecting.org>
>
> Nice catch. May I ask how you found this.
>
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
>
> -- Steve
>
>> ---
>> lib/bootconfig.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
>> index 2bcd5c2aa87e..a1e6a2e14b01 100644
>> --- a/lib/bootconfig.c
>> +++ b/lib/bootconfig.c
>> @@ -802,7 +802,7 @@ static int __init xbc_verify_tree(void)
>>
>>     /* Brace closing */
>>     if (brace_index) {
>> -       n = &xbc_nodes[open_brace[brace_index]];
>> +       n = &xbc_nodes[open_brace[brace_index - 1]];
>>         return xbc_parse_error("Brace is not closed",
>>                     xbc_node_get_data(n));
>>     }

Hi Steve,
Thanks for the review!
I found this while doing a manual audit of the bootconfig parser's error handling. I noticed that the post-increment in __xbc_open_brace() didn't seem to align with how xbc_verify_tree() was accessing the index. I verified it by intentionally passing a malformed config with an unclosed brace and saw it reporting a 'stale' or incorrect node location

Re: [PATCH v2 1/3] lib/bootconfig: fix off-by-one in xbc_verify_tree() unclosed brace error

Posted by Masami Hiramatsu (Google) 3 weeks, 4 days ago

On Thu, 12 Mar 2026 21:03:52 +0000
Josh Law <hlcj1234567@gmail.com> wrote:

> 12 Mar 2026 21:02:51 Steven Rostedt <rostedt@goodmis.org>:
> 
> > On Thu, 12 Mar 2026 19:11:41 +0000
> > Josh Law <hlcj1234567@gmail.com> wrote:
> >
> >> From: Josh Law <objecting@objecting.org>
> >>
> >> __xbc_open_brace() pushes entries with post-increment
> >> (open_brace[brace_index++]), so brace_index always points one past
> >> the last valid entry.  xbc_verify_tree() reads open_brace[brace_index]
> >> to report which brace is unclosed, but this is one past the last
> >> pushed entry and contains stale/zero data, causing the error message
> >> to reference the wrong node.
> >>
> >> Use open_brace[brace_index - 1] to correctly identify the unclosed
> >> brace.  brace_index is known to be > 0 here since we are inside the
> >> if (brace_index) guard.
> >>
> >> Signed-off-by: Josh Law <objecting@objecting.org>
> >
> > Nice catch. May I ask how you found this.
> >
> > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Thanks for patch and review!

> >
> > -- Steve
> >
> >> ---
> >> lib/bootconfig.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> >> index 2bcd5c2aa87e..a1e6a2e14b01 100644
> >> --- a/lib/bootconfig.c
> >> +++ b/lib/bootconfig.c
> >> @@ -802,7 +802,7 @@ static int __init xbc_verify_tree(void)
> >>
> >>     /* Brace closing */
> >>     if (brace_index) {
> >> -       n = &xbc_nodes[open_brace[brace_index]];
> >> +       n = &xbc_nodes[open_brace[brace_index - 1]];
> >>         return xbc_parse_error("Brace is not closed",
> >>                     xbc_node_get_data(n));
> >>     }
> 
> Hi Steve,
> Thanks for the review!
> I found this while doing a manual audit of the bootconfig parser's error handling. I noticed that the post-increment in __xbc_open_brace() didn't seem to align with how xbc_verify_tree() was accessing the index. I verified it by intentionally passing a 
malformed config with an unclosed brace and saw it reporting a 'stale' or incorrect node location

Thanks, I confirmed it with below config.

$ cat samples/bad-non-closed-brace.bconf 
foo {
	bar {
		buz
}

This closed the 2nd `{`, but not close the first one.

Without patch;
$ ./bootconfig samples/bad-non-closed-brace.bconf 
Parse Error: Brace is not closed at 2:2

With this fix;
$ ./bootconfig samples/bad-non-closed-brace.bconf 
Parse Error: Brace is not closed at 1:1

Than you!

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>