Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210327120155.500-1-ionic@ionic.de> <20210328095227.24323-1-ionic@ionic.de>
In-Reply-To: <20210328095227.24323-1-ionic@ionic.de>
From:   Masahiro Yamada <masahiroy@kernel.org>
Date:   Sat, 10 Apr 2021 14:47:46 +0900
Message-ID: <CAK7LNATzx9ujmapPbPzjL1Yo-A0RAXz0Kma+ve8pUeDDVx8GGw@mail.gmail.com>
Subject: Re: [PATCH v2] kconfig: nconf: stop endless search-up loops
To:     Mihai Moldovan <ionic@ionic.de>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Mar 28, 2021 at 6:52 PM Mihai Moldovan <ionic@ionic.de> 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 <ionic@ionic.de>
> ---
> v2: swap constant in comparison to right side, as requested by
>     Randy Dunlap <rdunlap@infradead.org>
>
>  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