Re: [PATCH v2] kconfig: nconf: stop endless search-up loops
On Sat, Apr 10, 2021 at 4:00 PM Mihai Moldovan wrote: > > * On 4/10/21 7:47 AM, Masahiro Yamada wrote: > > On Sun, Mar 28, 2021 at 6:52 PM Mihai Moldovan wrote: > >> + if ((index == -1) && (index == match_start)) > >> + return -1; > > > > We know 'index' is -1 in the second comparison. > > So, you can also write like this: > > > >if (match_start == -1 && index == -1) > > return -1; > > I know, but I sided for the other form for semantic reasons - this more > closely > directly describes what we actually care about (both being the same value and > either one being -1). > > > > But, it is not the correct fix, either. > > > > The root cause of the bug is match_start > > becoming -1. > > > > > > The following is the correct way to fix the bug > > without increasing the number of lines. > > > > > > > > diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c > > index e0f965529166..af814b39b876 100644 > > [...] > > + match_start = (match_start + items_num) % items_num; > > index = match_start; > > - index = (index + items_num) % items_num; > > This is probably more elegant and fixes two issues at the same time: > match_start > becoming -1 or n (which is likewise invalid, but was implicitly handled > through > the remainder operation). > > No objections from my side. Could you send v3 please? Then, I will apply it. -- Best Regards Masahiro Yamada
Re: [PATCH v2] kconfig: nconf: stop endless search-up loops
* On 4/10/21 7:47 AM, Masahiro Yamada wrote: > On Sun, Mar 28, 2021 at 6:52 PM Mihai Moldovan wrote: >> + if ((index == -1) && (index == match_start)) >> + return -1; > > We know 'index' is -1 in the second comparison. > So, you can also write like this: > >if (match_start == -1 && index == -1) > return -1; I know, but I sided for the other form for semantic reasons - this more closely directly describes what we actually care about (both being the same value and either one being -1). > But, it is not the correct fix, either. > > The root cause of the bug is match_start > becoming -1. > > > The following is the correct way to fix the bug > without increasing the number of lines. > > > > diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c > index e0f965529166..af814b39b876 100644 > [...] > + match_start = (match_start + items_num) % items_num; > index = match_start; > - index = (index + items_num) % items_num; This is probably more elegant and fixes two issues at the same time: match_start becoming -1 or n (which is likewise invalid, but was implicitly handled through the remainder operation). No objections from my side. Mihai
Re: [PATCH v2] kconfig: nconf: stop endless search-up loops
On Sun, Mar 28, 2021 at 6:52 PM Mihai Moldovan wrote:
>
> If the user selects the very first entry in a page and performs a
> search-up operation (e.g., via [/][a][Up Arrow]), nconf will never
> terminate searching the page.
>
> The reason is that in this case, the starting point will be set to -1,
> which is then translated into (n - 1) (i.e., the last entry of the
> page) and finally the search begins. This continues to work fine until
> the index reaches 0, at which point it will be decremented to -1, but
> not checked against the starting point right away. Instead, it's
> wrapped around to the bottom again, after which the starting point
> check occurs... and naturally fails.
>
> We can easily avoid it by checking against the starting point directly
> if the current index is -1 (which should be safe, since it's the only
> magic value that can occur) and terminate the matching function.
>
> Amazingly, nobody seems to have been hit by this for 11 years - or at
> the very least nobody bothered to debug and fix this.
>
> Signed-off-by: Mihai Moldovan
> ---
> v2: swap constant in comparison to right side, as requested by
> Randy Dunlap
>
> scripts/kconfig/nconf.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
> index e0f965529166..db0dc46bc5ee 100644
> --- a/scripts/kconfig/nconf.c
> +++ b/scripts/kconfig/nconf.c
> @@ -515,6 +515,15 @@ static int get_mext_match(const char *match_str, match_f
> flag)
> --index;
> else
> ++index;
> + /*
> +* It's fine for index to become negative - think of an
> +* initial value for match_start of 0 with a match direction
> +* of up, eventually making it -1.
> +*
> +* Handle this as a special case.
> +*/
> + if ((index == -1) && (index == match_start))
> + return -1;
We know 'index' is -1 in the second comparison.
So, you can also write like this:
if (match_start == -1 && index == -1)
return -1;
But, it is not the correct fix, either.
The root cause of the bug is match_start
becoming -1.
The following is the correct way to fix the bug
without increasing the number of lines.
diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index e0f965529166..af814b39b876 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -504,8 +504,8 @@ static int get_mext_match(const char *match_str,
match_f flag)
else if (flag == FIND_NEXT_MATCH_UP)
--match_start;
+ match_start = (match_start + items_num) % items_num;
index = match_start;
- index = (index + items_num) % items_num;
while (true) {
char *str = k_menu_items[index].str;
if (strcasestr(str, match_str) != NULL)
--
Best Regards
Masahiro Yamada
[PATCH v2] kconfig: nconf: stop endless search-up loops
If the user selects the very first entry in a page and performs a search-up operation (e.g., via [/][a][Up Arrow]), nconf will never terminate searching the page. The reason is that in this case, the starting point will be set to -1, which is then translated into (n - 1) (i.e., the last entry of the page) and finally the search begins. This continues to work fine until the index reaches 0, at which point it will be decremented to -1, but not checked against the starting point right away. Instead, it's wrapped around to the bottom again, after which the starting point check occurs... and naturally fails. We can easily avoid it by checking against the starting point directly if the current index is -1 (which should be safe, since it's the only magic value that can occur) and terminate the matching function. Amazingly, nobody seems to have been hit by this for 11 years - or at the very least nobody bothered to debug and fix this. Signed-off-by: Mihai Moldovan --- v2: swap constant in comparison to right side, as requested by Randy Dunlap scripts/kconfig/nconf.c | 9 + 1 file changed, 9 insertions(+) diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c index e0f965529166..db0dc46bc5ee 100644 --- a/scripts/kconfig/nconf.c +++ b/scripts/kconfig/nconf.c @@ -515,6 +515,15 @@ static int get_mext_match(const char *match_str, match_f flag) --index; else ++index; + /* +* It's fine for index to become negative - think of an +* initial value for match_start of 0 with a match direction +* of up, eventually making it -1. +* +* Handle this as a special case. +*/ + if ((index == -1) && (index == match_start)) + return -1; index = (index + items_num) % items_num; if (index == match_start) return -1; -- 2.30.1
