[PATCH] Fix stdio init handling
Hans-Peter Nilsson
hp@axis.com
Mon Jun 6 18:20:03 GMT 2022
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
More information about the Newlib
mailing list