2023-08-05 11:08:15

by Yoann Congal

Subject: [PATCH v2] kconfig: avoid an infinite loop in oldconfig/syncconfig

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 <[email protected]>
---
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

2023-08-07 19:42:26

by Masahiro Yamada

Subject: Re: [PATCH v2] kconfig: avoid an infinite loop in oldconfig/syncconfig

On Sat, Aug 5, 2023 at 6:57 PM Yoann Congal <[email protected]> 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 <[email protected]>
> ---
> 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.


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.


--
Best Regards
Masahiro Yamada

2023-08-07 21:05:22

by Randy Dunlap

Subject: Re: [PATCH v2] kconfig: avoid an infinite loop in oldconfig/syncconfig

On 8/7/23 12:25, Masahiro Yamada wrote:
> On Sat, Aug 5, 2023 at 6:57 PM Yoann Congal <[email protected]> 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 <[email protected]>
>> ---
>> 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

2023-09-12 18:09:11

by Yoann Congal

Subject: Re: [PATCH v2] kconfig: avoid an infinite loop in oldconfig/syncconfig

Hi,

On 8/7/23 22:25, Randy Dunlap wrote:
> On 8/7/23 12:25, Masahiro Yamada wrote:
>> 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:
>> *snip*>>
>> 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.

I've sent a v3 trying to fuse both of your suggestions.

Regards,
--
Yoann Congal
Smile ECS - Tech Expert