Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a3990928-b239-9939-5b40-8937d9301674@infradead.org>
Date:   Mon, 7 Aug 2023 13:25:07 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: [PATCH v2] kconfig: avoid an infinite loop in
 oldconfig/syncconfig
Content-Language: en-US
To:     Masahiro Yamada <masahiroy@kernel.org>,
        Yoann Congal <yoann.congal@smile.fr>
Cc:     linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230805095709.6717-1-yoann.congal@smile.fr>
 <CAK7LNAS8a=8n9r7kQrLTPpKwqXG1d1sd0WjJ8PQhOXHXxnSyNQ@mail.gmail.com>
From:   Randy Dunlap <rdunlap@infradead.org>
In-Reply-To: <CAK7LNAS8a=8n9r7kQrLTPpKwqXG1d1sd0WjJ8PQhOXHXxnSyNQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 8/7/23 12:25, Masahiro Yamada wrote:
> On Sat, Aug 5, 2023 at 6:57 PM Yoann Congal <yoann.congal@smile.fr> wrote:
>>
>> Exit on error when asking for value that has an invalid default value
>> and stdin has reached EOF. This happens in particular for hex/int
>> configs without an explicit default value.
>>
>> Previously, this case would loop:
>> * oldconfig prompts for the value but stdin has reached EOF
>> * It gets the global default value : an empty string
>> * This is not a valid hex/int value so it prompts again, hence the infinite loop.
>>
>> This case happens with a configuration like this (a hex config without a
>> valid default value):
>>   config TEST_KCONFIG
>>        hex "Test KConfig"
>>        # default 0x0
>>
>> And using:
>>   make oldconfig < /dev/null
>>
>> This was discovered when working on Yocto bug[0] on a downstream
>> kconfig user (U-boot)
>>
>> [0]: https://bugzilla.yoctoproject.org/show_bug.cgi?id=14136
>>
>> Signed-off-by: Yoann Congal <yoann.congal@smile.fr>
>> ---
>> v1->v2:
>>  * Improve coding style
>>  * Put more info in the commit message
>>
>>  scripts/kconfig/conf.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
>> index 7cf63261d951c..8f32cbbce4805 100644
>> --- a/scripts/kconfig/conf.c
>> +++ b/scripts/kconfig/conf.c
>> @@ -377,8 +377,16 @@ static int conf_string(struct menu *menu)
>>                         line[strlen(line)-1] = 0;
>>                         def = line;
>>                 }
>> -               if (def && sym_set_string_value(sym, def))
>> +               if (def && sym_set_string_value(sym, def)) {
>>                         return 0;
>> +               } else {
>> +                       if (feof(stdin) && !sym_string_valid(sym, sym_get_string_value(sym))) {
>> +                               fprintf(stderr,
>> +                                       "Symbol %s has invalid default value and stdin reached EOF\n",
>> +                                       sym->name);
>> +                               exit(1);
>> +                       }
>> +               }
>>         }
>>  }
>>
>> --
>> 2.30.2
>>
> 
> 
> It is strange (and consistent) to bail out
> only for particular types.
> 

It's still very helpful to know the symbol name that is causing
the issue.

> 
> I would change the code simply as follows:
> 
> 
> 
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -76,8 +76,10 @@ static void strip(char *str)
>  /* Helper function to facilitate fgets() by Jean Sacren. */
>  static void xfgets(char *str, int size, FILE *in)
>  {
> -       if (!fgets(str, size, in))
> +       if (!fgets(str, size, in)) {
>                 fprintf(stderr, "\nError in reading or end of file.\n");
> +               exit(1);
> +       }
> 
>         if (!tty_stdio)
>                 printf("%s", str);
> 
> 
> 
> 
> 
> 
> yes "" | make config
> 
> will succeed.
> 
> 
> 
> make config < /dev/null
> 
> will fail.
> 
> 
> 
> 
> 
> People expecting the closed stdin to succeed
> may start complaining, but I believe
> they must fix their wrong scripts.
> 
> 

-- 
~Randy