From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp1.axis.com (smtp1.axis.com [195.60.68.17]) by sourceware.org (Postfix) with ESMTPS id 5E16B385734C for ; Mon, 6 Jun 2022 18:20:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5E16B385734C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=axis.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=axis.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axis.com; q=dns/txt; s=axis-central1; t=1654539605; x=1686075605; h=from:to:subject:mime-version:content-transfer-encoding: message-id:date; bh=pRzuV881DEsnez+fbUDYVODT7gOPdOVdPjbFmJ7gAC8=; b=GgDjhznpBNUmaRX18OFESxq3jzBnNpQSPYQ7NQJ3l65zLJzpwNOgrQ5d RFrdx72iVIPk8MYliarYPjJiF02FYAkqT9H9QzcItezu6l7TMIOiDoFX1 E0blWLxnZO2SYahVgF0kNCGKpWGLD6zi5xscutAKSioI0IT4yeMWGIy6/ KY874QFhiewfdCR04g8O4oaG5vdSa7/r/t6lRoNgTrWIaa97mnHHYQbHm npQbVdtAG/SZhXQrqu6VW/FvfWdYdQX+MtTBkmBs90Au8LewcikrPS3s4 2xclaVLvJN85hAV+7sDB6sZHNRFDRZa82ZR1Etf6QCluW05B9rXT5X4uY A==; From: Hans-Peter Nilsson To: Subject: [PATCH] Fix stdio init handling MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 8BIT Message-ID: <20220606182003.DB8A72041A@pchp3.se.axis.com> Date: Mon, 6 Jun 2022 20:20:03 +0200 X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: newlib@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Newlib mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Jun 2022 18:20:07 -0000 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 #include #include 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