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

From: Nathan Chancellor <nathan@kernel.org>
To: Masahiro Yamada <masahiroy@kernel.org>
Cc: Yoann Congal <yoann.congal@smile.fr>,
	linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org,
	Randy Dunlap <rdunlap@infradead.org>,
	Brandon Maier <brandon.maier@collins.com>,
	Nick Desaulniers <ndesaulniers@google.com>,
	llvm@lists.linux.dev
Subject: Re: [PATCH v5] kconfig: avoid an infinite loop in oldconfig/syncconfig
Date: Tue, 7 Nov 2023 14:00:04 -0700	[thread overview]
Message-ID: <20231107210004.GA2065849@dev-arch.thelio-3990X> (raw)
In-Reply-To: <CAK7LNAS6J5Nh8nOUHbaf123yd1Z-1q--FvB1ok8GQcoNorAROw@mail.gmail.com>

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