Re: [PATCH] Fix stdio init handling

[Jeff Johnston <jjohnstn@redhat.com>]

Thanks Han-Peter,

I'm inclined to allow Sebastian a chance to fix this in line with his
overall design.  Sebastian, do you have a fix for this?  If you are
absent, we can merge this patch and you can modify later.  BTW: Sebastian,
I had given you permission to commit your last set of patches if
you had accounted for Corinna's existing comments, but I haven't seen
anything.

-- Jeff J.


On Mon, Jun 6, 2022 at 2:20 PM Hans-Peter Nilsson <hp@axis.com> wrote:

> Ok to commit?
>
> ----- 8< -----
> After commit e826fbb2ae88 "Fix stdio exit handling", for trivial
> newlib using targets such as cris-elf and arm-eabihf, i.e.
> (!_REENT_SMALL && !_REENT_GLOBAL_STDIO_STREAMS && !cygwin) stdio
> initialization no longer happens properly.  At that commit and
> after, programs like the following are broken for such targets;
> at the first opened file, the first FILE record seems
> pre-allocated, unused and free, and is returned as the "FILE *"
> at the fopen.  At the subsequent fgetc, it's *initialized* as
> stdin and the fgetc returns EOF (without errno), yielding
> "fgetc-EOF: Success" and the program aborted instead of the
> expected "all-ok".
>
> ===============
> /* There must exist a file "./fff" with the first byte an 'f'.  */
> #include <stdio.h>
> #include <errno.h>
> #include <stdlib.h>
> int main(void)
> {
>   int e = (errno = 0);
>   FILE *f = fopen("fff", "r");
>   int c = fgetc(f);
>
>   if (f == NULL)
>     {
>       perror("fopen");
>       abort();
>     }
>
>   if (c == EOF)
>     {
>       perror("fgetc-EOF");
>       abort();
>     }
>
>   if (c != 'f')
>     {
>       perror("fgetc-not-f");
>       abort();
>     }
>
>   if (fclose (f) != 0)
>     {
>       perror("fclose");
>       abort();
>     }
>
>   puts("all-ok");
>   return 0;
> }
> ===============
>
> But the problem started already with commit 26747c47bc0a "Add
> stdio_exit_handler()" which made a half-way transition from
> guarding initialization from _GLOBAL_REENT->__cleanup == NULL
> and stdio initialization in __sinit, to __stdio_exit_handler ==
> NULL and global_stdio_init.  The solution is to advance that
> transition, making __sinit just a lock-accounting wrapper for
> global_stdio_init.  Both "guards" are now initialized, allowing
> either _GLOBAL_REENT->__cleanup or __stdio_exit_handler to be
> checked and any other struct _reent should work too.
>
> I'm also fixing what appears to be a deadlock problem:
> consider what happens *before* the patch, with the "sfp
> lock" for _REENT_SMALL && !_REENT_GLOBAL_STDIO_STREAMS when
> __sinit is called and global_stdio_init hasn't been called.
>
> An alternative would be to revert 26747c47bc0a and depending
> follow-up commits and move back to __sinit and
> _GLOBAL_REENT->__cleanup.  (I don't really see *why* it's
> important to avoid using _GLOBAL_REENT in exit() and the
> conversation in the email archives doesn't tell me.)
>
> Tested cris-elf, fixing the test-case as well as the gcc
> testsuite regressions I noticed when attempting to update
> newlib from 01c734b0d7c1 to e93bb6f9852a for my cris-elf
> autotester.
>
> Caveats and observations:
>
> I'm not touching newlib/libc/machine/spu/stdio.c:__sinit,
> which also wasn't updated for that patchset, and even before
> that it also appears to not handle neither
> _REENT_GLOBAL_STDIO_STREAMS nor _REENT_SMALL.  To wit, I
> suspect it's broken since that time.
>
> Perhaps the comment in reent.h should also be updated;
> struct _reent doesn't contain all state (not
> __stdio_exit_handler), at least since 26747c47bc0a but also
> since the introduction of __atexit and __atexit0.
>
> There's (still) redundancy between stdio_exit_handler and
> cleanup_stdio.
>
> Whomever calls __sinit or __sfp first, better do it with
> _GLOBAL_REENT; there's confusion whenever the struct _reent
> argument is something else.
>
> There are cleanup opportunities wrt. the "sfp lock", to make
> __sinit and __sfp use "names" at the same level, avoiding
> confusion.
>
>         * libc/stdio/findfp.c (__sfp_nolock): New function with the
> contents
>         of __sfp.
>         (__sfp): Keep just locking parts.
>         (global_stdio_init): Add struct _reent parameter.  Include the
>         remaining contents of __sinit but call __sfp_nolock instead of
> __sfp.
> ---
>  newlib/libc/stdio/findfp.c | 67 ++++++++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c
> index 6933ff1dbcef..ff112167a7ad 100644
> --- a/newlib/libc/stdio/findfp.c
> +++ b/newlib/libc/stdio/findfp.c
> @@ -160,8 +160,11 @@ stdio_exit_handler (void)
>    (void) _fwalk_sglue (_GLOBAL_REENT, CLEANUP_FILE, &__sglue);
>  }
>
> +static FILE *__sfp_nolock (struct _reent *);
> +static void cleanup_stdio (struct _reent *);
> +
>  static void
> -global_stdio_init (void)
> +global_stdio_init (struct _reent *s)
>  {
>    if (__stdio_exit_handler == NULL) {
>      __stdio_exit_handler = stdio_exit_handler;
> @@ -171,22 +174,38 @@ global_stdio_init (void)
>      stderr_init (&__sf[2]);
>  #endif
>    }
> +
> +  if (s->__cleanup == NULL) {
> +    /* Make sure we clean up on exit.  */
> +    s->__cleanup = cleanup_stdio;
> +
> +#ifdef _REENT_SMALL
> +# ifndef _REENT_GLOBAL_STDIO_STREAMS
> +    s->_stdin = __sfp_nolock(s);
> +    s->_stdout = __sfp_nolock(s);
> +    s->_stderr = __sfp_nolock(s);
> +# endif /* _REENT_GLOBAL_STDIO_STREAMS */
> +#endif
> +
> +#ifndef _REENT_GLOBAL_STDIO_STREAMS
> +    stdin_init (s->_stdin);
> +    stdout_init (s->_stdout);
> +    stderr_init (s->_stderr);
> +#endif /* _REENT_GLOBAL_STDIO_STREAMS */
> +  }
>  }
>
>  /*
>   * Find a free FILE for fopen et al.
>   */
>
> -FILE *
> -__sfp (struct _reent *d)
> +static FILE *
> +__sfp_nolock (struct _reent *d)
>  {
>    FILE *fp;
>    int n;
>    struct _glue *g;
>
> -  _newlib_sfp_lock_start ();
> -  global_stdio_init ();
> -
>    for (g = &__sglue;; g = g->_next)
>      {
>        for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
> @@ -196,7 +215,6 @@ __sfp (struct _reent *d)
>           (g->_next = sfmoreglue (d, NDYNAMIC)) == NULL)
>         break;
>      }
> -  _newlib_sfp_lock_exit ();
>    d->_errno = ENOMEM;
>    return NULL;
>
> @@ -207,7 +225,6 @@ found:
>  #ifndef __SINGLE_THREAD__
>    __lock_init_recursive (fp->_lock);
>  #endif
> -  _newlib_sfp_lock_end ();
>
>    fp->_p = NULL;               /* no current pointer */
>    fp->_w = 0;                  /* nothing to read or write */
> @@ -225,6 +242,19 @@ found:
>    return fp;
>  }
>
> +FILE *
> +__sfp (struct _reent *d)
> +{
> +  FILE *fp;
> +
> +  _newlib_sfp_lock_start ();
> +  global_stdio_init (d);
> +  fp = __sfp_nolock (d);
> +  _newlib_sfp_lock_end ();
> +
> +  return fp;
> +}
> +
>  /*
>   * exit() calls _cleanup() through *__cleanup, set whenever we
>   * open or buffer a file.  This chicanery is done so that programs
> @@ -262,26 +292,7 @@ __sinit (struct _reent *s)
>        __sfp_lock_release ();
>        return;
>      }
> -
> -  /* make sure we clean up on exit */
> -  s->__cleanup = cleanup_stdio;        /* conservative */
> -
> -#ifdef _REENT_SMALL
> -# ifndef _REENT_GLOBAL_STDIO_STREAMS
> -  s->_stdin = __sfp(s);
> -  s->_stdout = __sfp(s);
> -  s->_stderr = __sfp(s);
> -# endif /* _REENT_GLOBAL_STDIO_STREAMS */
> -#endif
> -
> -  global_stdio_init ();
> -
> -#ifndef _REENT_GLOBAL_STDIO_STREAMS
> -  stdin_init (s->_stdin);
> -  stdout_init (s->_stdout);
> -  stderr_init (s->_stderr);
> -#endif /* _REENT_GLOBAL_STDIO_STREAMS */
> -
> +  global_stdio_init (s);
>    __sfp_lock_release ();
>  }
>
> --
> 2.30.2
>
>