diff mbox series

[v5] kconfig: avoid an infinite loop in oldconfig/syncconfig

Message ID 20231104222715.3967791-1-yoann.congal@smile.fr (mailing list archive)
State New, archived
Headers show
Series [v5] kconfig: avoid an infinite loop in oldconfig/syncconfig | expand

Commit Message

Yoann Congal Nov. 4, 2023, 10:27 p.m. UTC
Exit on error when asking for value and reading stdin returns an error
(mainly if it has reached EOF or is closed).

This infinite loop 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

CC: Brandon Maier <brandon.maier@collins.com>
Signed-off-by: Yoann Congal <yoann.congal@smile.fr>
---
v4->v5:
 * Switched to Masahiro Yamada's suggested code.
v3->v4:
 * Added Brandon Maier's "Tested-by". Thanks!
v2->v3:
 * Simplify the patch by fusing comments of :
   * Masahiro Yamada : Exit as soon as reading stdin hits an error
   * Randy Dunlap : Display the name of the currently read symbol
v1->v2:
 * Improve coding style
 * Put more info in the commit message
---
 scripts/kconfig/conf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Masahiro Yamada Nov. 5, 2023, 7:55 a.m. UTC | #1
On Sun, Nov 5, 2023 at 7:27 AM Yoann Congal <yoann.congal@smile.fr> wrote:
>
> Exit on error when asking for value and reading stdin returns an error
> (mainly if it has reached EOF or is closed).
>
> This infinite loop 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
>
> CC: Brandon Maier <brandon.maier@collins.com>
> Signed-off-by: Yoann Congal <yoann.congal@smile.fr>

Applied to linux-kbuild.
Thanks.


> ---
> v4->v5:
>  * Switched to Masahiro Yamada's suggested code.
> v3->v4:
>  * Added Brandon Maier's "Tested-by". Thanks!
> v2->v3:
>  * Simplify the patch by fusing comments of :
>    * Masahiro Yamada : Exit as soon as reading stdin hits an error
>    * Randy Dunlap : Display the name of the currently read symbol
> v1->v2:
>  * Improve coding style
>  * Put more info in the commit message
> ---
>  scripts/kconfig/conf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 33d19e419908..62de1fbaff97 100644
> --- 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);
> --
> 2.30.2
>
Nathan Chancellor Nov. 7, 2023, 9 p.m. UTC | #2
On Sun, Nov 05, 2023 at 04:55:57PM +0900, Masahiro Yamada wrote:
> On Sun, Nov 5, 2023 at 7:27 AM Yoann Congal <yoann.congal@smile.fr> wrote:
> >
> > Exit on error when asking for value and reading stdin returns an error
> > (mainly if it has reached EOF or is closed).
> >
> > This infinite loop 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
> >
> > CC: Brandon Maier <brandon.maier@collins.com>
> > Signed-off-by: Yoann Congal <yoann.congal@smile.fr>
> 
> Applied to linux-kbuild.
> Thanks.

For what it's worth, this change breaks our continuous integration [1]
because tuxmake explicitly uses /dev/null as stdin to make for
non-interactive commands (I think it does this as basically the
equivalent of "yes '' | make" in Python), so the error will always
occur.

Before:

$ curl -LSso .config https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/raw/main/config

# Same as 'make < /dev/null' but still
$ python3 -c "import subprocess; subprocess.run(['make', '-j128'], stdin=subprocess.DEVNULL)"
  SYNC    include/config/auto.conf
  HOSTCC  scripts/basic/fixdep
  HOSTCC  scripts/kconfig/conf.o
  HOSTCC  scripts/kconfig/confdata.o
  HOSTCC  scripts/kconfig/expr.o
  LEX     scripts/kconfig/lexer.lex.c
  YACC    scripts/kconfig/parser.tab.[ch]
  HOSTCC  scripts/kconfig/menu.o
  HOSTCC  scripts/kconfig/preprocess.o
  HOSTCC  scripts/kconfig/symbol.o
  HOSTCC  scripts/kconfig/util.o
  HOSTCC  scripts/kconfig/lexer.lex.o
  HOSTCC  scripts/kconfig/parser.tab.o
  HOSTLD  scripts/kconfig/conf
*
* Restart config...
*
...
Error in reading or end of file.

  GEN     arch/x86/include/generated/asm/orc_hash.h
  WRAP    arch/x86/include/generated/uapi/asm/bpf_perf_event.h
  WRAP    arch/x86/include/generated/uapi/asm/errno.h
...

After:

$ curl -LSso .config https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/raw/main/config

$ python3 -c "import subprocess; subprocess.run(['make', '-j128'], stdin=subprocess.DEVNULL)"
  SYNC    include/config/auto.conf
  HOSTCC  scripts/basic/fixdep
  HOSTCC  scripts/kconfig/conf.o
  HOSTCC  scripts/kconfig/confdata.o
  HOSTCC  scripts/kconfig/expr.o
  LEX     scripts/kconfig/lexer.lex.c
  YACC    scripts/kconfig/parser.tab.[ch]
  HOSTCC  scripts/kconfig/menu.o
  HOSTCC  scripts/kconfig/preprocess.o
  HOSTCC  scripts/kconfig/symbol.o
  HOSTCC  scripts/kconfig/util.o
  HOSTCC  scripts/kconfig/lexer.lex.o
  HOSTCC  scripts/kconfig/parser.tab.o
  HOSTLD  scripts/kconfig/conf
*
* Restart config...
*
...
Error in reading or end of file.
make[3]: *** [scripts/kconfig/Makefile:77: syncconfig] Error 1
...

We have been doing this for some time and never run across an infinite
loop in syncconfig. Can this be improved?

[1]: https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/6784349581

Cheers,
Nathan

> > ---
> > v4->v5:
> >  * Switched to Masahiro Yamada's suggested code.
> > v3->v4:
> >  * Added Brandon Maier's "Tested-by". Thanks!
> > v2->v3:
> >  * Simplify the patch by fusing comments of :
> >    * Masahiro Yamada : Exit as soon as reading stdin hits an error
> >    * Randy Dunlap : Display the name of the currently read symbol
> > v1->v2:
> >  * Improve coding style
> >  * Put more info in the commit message
> > ---
> >  scripts/kconfig/conf.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> > index 33d19e419908..62de1fbaff97 100644
> > --- 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);
> > --
> > 2.30.2
> >
> 
> 
> -- 
> Best Regards
> Masahiro Yamada
Masahiro Yamada Nov. 9, 2023, 4:26 a.m. UTC | #3
On Tue, Nov 7, 2023 at 11:00 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Sun, Nov 05, 2023 at 04:55:57PM +0900, Masahiro Yamada wrote:
> > On Sun, Nov 5, 2023 at 7:27 AM Yoann Congal <yoann.congal@smile.fr> wrote:
> > >
> > > Exit on error when asking for value and reading stdin returns an error
> > > (mainly if it has reached EOF or is closed).
> > >
> > > This infinite loop 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
> > >
> > > CC: Brandon Maier <brandon.maier@collins.com>
> > > Signed-off-by: Yoann Congal <yoann.congal@smile.fr>
> >
> > Applied to linux-kbuild.
> > Thanks.
>
> For what it's worth, this change breaks our continuous integration [1]
> because tuxmake explicitly uses /dev/null as stdin to make for
> non-interactive commands (I think it does this as basically the
> equivalent of "yes '' | make" in Python), so the error will always
> occur.
>
> Before:
>
> $ curl -LSso .config https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/raw/main/config
>
> # Same as 'make < /dev/null' but still
> $ python3 -c "import subprocess; subprocess.run(['make', '-j128'], stdin=subprocess.DEVNULL)"
>   SYNC    include/config/auto.conf
>   HOSTCC  scripts/basic/fixdep
>   HOSTCC  scripts/kconfig/conf.o
>   HOSTCC  scripts/kconfig/confdata.o
>   HOSTCC  scripts/kconfig/expr.o
>   LEX     scripts/kconfig/lexer.lex.c
>   YACC    scripts/kconfig/parser.tab.[ch]
>   HOSTCC  scripts/kconfig/menu.o
>   HOSTCC  scripts/kconfig/preprocess.o
>   HOSTCC  scripts/kconfig/symbol.o
>   HOSTCC  scripts/kconfig/util.o
>   HOSTCC  scripts/kconfig/lexer.lex.o
>   HOSTCC  scripts/kconfig/parser.tab.o
>   HOSTLD  scripts/kconfig/conf
> *
> * Restart config...
> *
> ...
> Error in reading or end of file.
>
>   GEN     arch/x86/include/generated/asm/orc_hash.h
>   WRAP    arch/x86/include/generated/uapi/asm/bpf_perf_event.h
>   WRAP    arch/x86/include/generated/uapi/asm/errno.h
> ...
>
> After:
>
> $ curl -LSso .config https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/raw/main/config
>
> $ python3 -c "import subprocess; subprocess.run(['make', '-j128'], stdin=subprocess.DEVNULL)"
>   SYNC    include/config/auto.conf
>   HOSTCC  scripts/basic/fixdep
>   HOSTCC  scripts/kconfig/conf.o
>   HOSTCC  scripts/kconfig/confdata.o
>   HOSTCC  scripts/kconfig/expr.o
>   LEX     scripts/kconfig/lexer.lex.c
>   YACC    scripts/kconfig/parser.tab.[ch]
>   HOSTCC  scripts/kconfig/menu.o
>   HOSTCC  scripts/kconfig/preprocess.o
>   HOSTCC  scripts/kconfig/symbol.o
>   HOSTCC  scripts/kconfig/util.o
>   HOSTCC  scripts/kconfig/lexer.lex.o
>   HOSTCC  scripts/kconfig/parser.tab.o
>   HOSTLD  scripts/kconfig/conf
> *
> * Restart config...
> *
> ...
> Error in reading or end of file.
> make[3]: *** [scripts/kconfig/Makefile:77: syncconfig] Error 1
> ...
>
> We have been doing this for some time and never run across an infinite
> loop in syncconfig. Can this be improved?



In Linux, most int/hex entries have a default,
hence there is no practical issue.

I will drop this for now.

I will send an alternative solution.








--
Best Regards
Masahiro Yamada
Yoann Congal Nov. 9, 2023, 10:56 p.m. UTC | #4
Hi,

Le 09/11/2023 à 05:26, Masahiro Yamada a écrit :
> On Tue, Nov 7, 2023 at 11:00 PM Nathan Chancellor <nathan@kernel.org> wrote:
>> For what it's worth, this change breaks our continuous integration [1>> because tuxmake explicitly uses /dev/null as stdin to make for
>> non-interactive commands (I think it does this as basically the
>> equivalent of "yes '' | make" in Python), so the error will always
>> occur.
>>
>> Before:
>>
>> [...]
>>
>> After:
>>
>> $ curl -LSso .config https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/raw/main/config

Calling 'make olddefconfig' at this point should avoid opening the prompt on /dev/null in the next make.

I got tuxmake to that with a hack:
$ .../tuxmake/run --kconfig /dev/null --kconfig-add https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/raw/main/config
# --kconfig /dev/null      => Start from a blank config
# --kconfig-add https//... => ... add to it a partial config
=> tuxmake will "merge" the empty config and the URL one and then run 'make olddefconfig' and finally 'make < /dev/null' which does run

>> $ python3 -c "import subprocess; subprocess.run(['make', '-j128'], stdin=subprocess.DEVNULL)"
>>   SYNC    include/config/auto.conf
>>   HOSTCC  scripts/basic/fixdep
>>   HOSTCC  scripts/kconfig/conf.o
>>   HOSTCC  scripts/kconfig/confdata.o
>>   HOSTCC  scripts/kconfig/expr.o
>>   LEX     scripts/kconfig/lexer.lex.c
>>   YACC    scripts/kconfig/parser.tab.[ch]
>>   HOSTCC  scripts/kconfig/menu.o
>>   HOSTCC  scripts/kconfig/preprocess.o
>>   HOSTCC  scripts/kconfig/symbol.o
>>   HOSTCC  scripts/kconfig/util.o
>>   HOSTCC  scripts/kconfig/lexer.lex.o
>>   HOSTCC  scripts/kconfig/parser.tab.o
>>   HOSTLD  scripts/kconfig/conf
>> *
>> * Restart config...
>> *
>> ...
>> Error in reading or end of file.
>> make[3]: *** [scripts/kconfig/Makefile:77: syncconfig] Error 1
>> ...
>>
>> We have been doing this for some time and never run across an infinite
>> loop in syncconfig. Can this be improved?
> 
> In Linux, most int/hex entries have a default,
> hence there is no practical issue.

I agree. I never met such case in Linux but only on downstream kbuild user (u-boot in this case).

> I will drop this for now.

Okay!

> I will send an alternative solution.

Please tell me how I can help!

For what it's worth the v2 of this patch[0] tried to exit *only* where the infinite loop would start.
I've just tested it, it allows tuxmake to run smoothly and avoids the infinite loop in case of a hex config without a valid default value.

[0]:https://lore.kernel.org/lkml/20230805095709.6717-1-yoann.congal@smile.fr/

Regards,
diff mbox series

Patch

diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
index 33d19e419908..62de1fbaff97 100644
--- 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);