[PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

Now that abort no longer calls fflush there is no reason to avoid locking
the stdio streams anywhere.  This fixes a conformance issue and potential
heap corruption during exit.
---
 libio/genops.c | 43 ++++++++++---------------------------------
 libio/libioP.h |  1 -
 2 files changed, 10 insertions(+), 34 deletions(-)

diff --git a/libio/genops.c b/libio/genops.c
index 7131312704..fbd8dd9e75 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -682,7 +682,7 @@ _IO_adjust_column (unsigned start, const char *line, int count)
 libc_hidden_def (_IO_adjust_column)
 
 int
-_IO_flush_all_lockp (int do_lock)
+_IO_flush_all (void)
 {
   int result = 0;
   FILE *fp;
@@ -695,8 +695,7 @@ _IO_flush_all_lockp (int do_lock)
   for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
-      if (do_lock)
-	_IO_flockfile (fp);
+      _IO_flockfile (fp);
 
       if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
 	   || (_IO_vtable_offset (fp) == 0
@@ -706,8 +705,7 @@ _IO_flush_all_lockp (int do_lock)
 	  && _IO_OVERFLOW (fp, EOF) == EOF)
 	result = EOF;
 
-      if (do_lock)
-	_IO_funlockfile (fp);
+      _IO_funlockfile (fp);
       run_fp = NULL;
     }
 
@@ -718,14 +716,6 @@ _IO_flush_all_lockp (int do_lock)
 
   return result;
 }
-
-
-int
-_IO_flush_all (void)
-{
-  /* We want locking.  */
-  return _IO_flush_all_lockp (1);
-}
 libc_hidden_def (_IO_flush_all)
 
 void
@@ -791,6 +781,9 @@ _IO_unbuffer_all (void)
     {
       int legacy = 0;
 
+      run_fp = fp;
+      _IO_flockfile (fp);
+
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
       if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
 	legacy = 1;
@@ -800,18 +793,6 @@ _IO_unbuffer_all (void)
 	  /* Iff stream is un-orientated, it wasn't used. */
 	  && (legacy || fp->_mode != 0))
 	{
-#ifdef _IO_MTSAFE_IO
-	  int cnt;
-#define MAXTRIES 2
-	  for (cnt = 0; cnt < MAXTRIES; ++cnt)
-	    if (fp->_lock == NULL || _IO_lock_trylock (*fp->_lock) == 0)
-	      break;
-	    else
-	      /* Give the other thread time to finish up its use of the
-		 stream.  */
-	      __sched_yield ();
-#endif
-
 	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
@@ -825,17 +806,15 @@ _IO_unbuffer_all (void)
 
 	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
-
-#ifdef _IO_MTSAFE_IO
-	  if (cnt < MAXTRIES && fp->_lock != NULL)
-	    _IO_lock_unlock (*fp->_lock);
-#endif
 	}
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
       if (! legacy)
 	fp->_mode = -1;
+
+      _IO_funlockfile (fp);
+      run_fp = NULL;
     }
 
 #ifdef _IO_MTSAFE_IO
@@ -861,9 +840,7 @@ __libio_freemem (void)
 int
 _IO_cleanup (void)
 {
-  /* We do *not* want locking.  Some threads might use streams but
-     that is their problem, we flush them underneath them.  */
-  int result = _IO_flush_all_lockp (0);
+  int result = _IO_flush_all ();
 
   /* We currently don't have a reliable mechanism for making sure that
      C++ static destructors are executed in the correct order.
diff --git a/libio/libioP.h b/libio/libioP.h
index d777553cb0..745278e076 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -537,7 +537,6 @@ extern int _IO_new_do_write (FILE *, const char *, size_t);
 extern int _IO_old_do_write (FILE *, const char *, size_t);
 extern int _IO_wdo_write (FILE *, const wchar_t *, size_t);
 libc_hidden_proto (_IO_wdo_write)
-extern int _IO_flush_all_lockp (int);
 extern int _IO_flush_all (void);
 libc_hidden_proto (_IO_flush_all)
 extern void _IO_flush_all_linebuffered (void);
-- 
2.41.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Florian Weimer]

* Andreas Schwab via Libc-alpha:

> Now that abort no longer calls fflush there is no reason to avoid locking
> the stdio streams anywhere.  This fixes a conformance issue and potential
> heap corruption during exit.

Mechanics of the patch look okay to me.  In the end, we may have to make
this configurable if too many applications hang during shutdown, but I
think we should put it into the upcoming release.

Thanks,
Florian

Re: [PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

On Jun 30 2023, Florian Weimer wrote:

> * Andreas Schwab via Libc-alpha:
>
>> Now that abort no longer calls fflush there is no reason to avoid locking
>> the stdio streams anywhere.  This fixes a conformance issue and potential
>> heap corruption during exit.
>
> Mechanics of the patch look okay to me.  In the end, we may have to make
> this configurable if too many applications hang during shutdown, but I
> think we should put it into the upcoming release.

This has been part of SUSE/openSUSE for several years, and I have not
seen any complaints so far.  It's more likely that you get a crash
during the unlocked access to the streams.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Florian Weimer]

* Andreas Schwab:

> On Jun 30 2023, Florian Weimer wrote:
>
>> * Andreas Schwab via Libc-alpha:
>>
>>> Now that abort no longer calls fflush there is no reason to avoid locking
>>> the stdio streams anywhere.  This fixes a conformance issue and potential
>>> heap corruption during exit.
>>
>> Mechanics of the patch look okay to me.  In the end, we may have to make
>> this configurable if too many applications hang during shutdown, but I
>> think we should put it into the upcoming release.
>
> This has been part of SUSE/openSUSE for several years, and I have not
> seen any complaints so far.  It's more likely that you get a crash
> during the unlocked access to the streams.

Oh, thanks for running the experiment, and upstreaming the result.  I
agree that it should make it likely that there are no issues.

Florian

[PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

Now that abort no longer calls fflush there is no reason to avoid locking
the stdio streams anywhere.  This fixes a conformance issue and potential
heap corruption during exit.
---
 libio/genops.c | 43 ++++++++++---------------------------------
 libio/libioP.h |  1 -
 2 files changed, 10 insertions(+), 34 deletions(-)

diff --git a/libio/genops.c b/libio/genops.c
index 7131312704..fbd8dd9e75 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -682,7 +682,7 @@ _IO_adjust_column (unsigned start, const char *line, int count)
 libc_hidden_def (_IO_adjust_column)
 
 int
-_IO_flush_all_lockp (int do_lock)
+_IO_flush_all (void)
 {
   int result = 0;
   FILE *fp;
@@ -695,8 +695,7 @@ _IO_flush_all_lockp (int do_lock)
   for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
-      if (do_lock)
-	_IO_flockfile (fp);
+      _IO_flockfile (fp);
 
       if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
 	   || (_IO_vtable_offset (fp) == 0
@@ -706,8 +705,7 @@ _IO_flush_all_lockp (int do_lock)
 	  && _IO_OVERFLOW (fp, EOF) == EOF)
 	result = EOF;
 
-      if (do_lock)
-	_IO_funlockfile (fp);
+      _IO_funlockfile (fp);
       run_fp = NULL;
     }
 
@@ -718,14 +716,6 @@ _IO_flush_all_lockp (int do_lock)
 
   return result;
 }
-
-
-int
-_IO_flush_all (void)
-{
-  /* We want locking.  */
-  return _IO_flush_all_lockp (1);
-}
 libc_hidden_def (_IO_flush_all)
 
 void
@@ -791,6 +781,9 @@ _IO_unbuffer_all (void)
     {
       int legacy = 0;
 
+      run_fp = fp;
+      _IO_flockfile (fp);
+
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
       if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
 	legacy = 1;
@@ -800,18 +793,6 @@ _IO_unbuffer_all (void)
 	  /* Iff stream is un-orientated, it wasn't used. */
 	  && (legacy || fp->_mode != 0))
 	{
-#ifdef _IO_MTSAFE_IO
-	  int cnt;
-#define MAXTRIES 2
-	  for (cnt = 0; cnt < MAXTRIES; ++cnt)
-	    if (fp->_lock == NULL || _IO_lock_trylock (*fp->_lock) == 0)
-	      break;
-	    else
-	      /* Give the other thread time to finish up its use of the
-		 stream.  */
-	      __sched_yield ();
-#endif
-
 	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
@@ -825,17 +806,15 @@ _IO_unbuffer_all (void)
 
 	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
-
-#ifdef _IO_MTSAFE_IO
-	  if (cnt < MAXTRIES && fp->_lock != NULL)
-	    _IO_lock_unlock (*fp->_lock);
-#endif
 	}
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
       if (! legacy)
 	fp->_mode = -1;
+
+      _IO_funlockfile (fp);
+      run_fp = NULL;
     }
 
 #ifdef _IO_MTSAFE_IO
@@ -861,9 +840,7 @@ __libio_freemem (void)
 int
 _IO_cleanup (void)
 {
-  /* We do *not* want locking.  Some threads might use streams but
-     that is their problem, we flush them underneath them.  */
-  int result = _IO_flush_all_lockp (0);
+  int result = _IO_flush_all ();
 
   /* We currently don't have a reliable mechanism for making sure that
      C++ static destructors are executed in the correct order.
diff --git a/libio/libioP.h b/libio/libioP.h
index d777553cb0..745278e076 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -537,7 +537,6 @@ extern int _IO_new_do_write (FILE *, const char *, size_t);
 extern int _IO_old_do_write (FILE *, const char *, size_t);
 extern int _IO_wdo_write (FILE *, const wchar_t *, size_t);
 libc_hidden_proto (_IO_wdo_write)
-extern int _IO_flush_all_lockp (int);
 extern int _IO_flush_all (void);
 libc_hidden_proto (_IO_flush_all)
 extern void _IO_flush_all_linebuffered (void);
-- 
2.41.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

Now that abort no longer calls fflush there is no reason to avoid locking
the stdio streams anywhere.  This fixes a conformance issue and potential
heap corruption during exit.  The test nptl/tst-stdio1 is removed as that
was expecting the problematic behaviour.
---
 libio/genops.c               | 43 +++++++---------------------
 libio/libioP.h               |  1 -
 sysdeps/pthread/Makefile     |  1 -
 sysdeps/pthread/tst-stdio1.c | 55 ------------------------------------
 4 files changed, 10 insertions(+), 90 deletions(-)
 delete mode 100644 sysdeps/pthread/tst-stdio1.c

diff --git a/libio/genops.c b/libio/genops.c
index 7131312704..fbd8dd9e75 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -682,7 +682,7 @@ _IO_adjust_column (unsigned start, const char *line, int count)
 libc_hidden_def (_IO_adjust_column)
 
 int
-_IO_flush_all_lockp (int do_lock)
+_IO_flush_all (void)
 {
   int result = 0;
   FILE *fp;
@@ -695,8 +695,7 @@ _IO_flush_all_lockp (int do_lock)
   for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
-      if (do_lock)
-	_IO_flockfile (fp);
+      _IO_flockfile (fp);
 
       if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
 	   || (_IO_vtable_offset (fp) == 0
@@ -706,8 +705,7 @@ _IO_flush_all_lockp (int do_lock)
 	  && _IO_OVERFLOW (fp, EOF) == EOF)
 	result = EOF;
 
-      if (do_lock)
-	_IO_funlockfile (fp);
+      _IO_funlockfile (fp);
       run_fp = NULL;
     }
 
@@ -718,14 +716,6 @@ _IO_flush_all_lockp (int do_lock)
 
   return result;
 }
-
-
-int
-_IO_flush_all (void)
-{
-  /* We want locking.  */
-  return _IO_flush_all_lockp (1);
-}
 libc_hidden_def (_IO_flush_all)
 
 void
@@ -791,6 +781,9 @@ _IO_unbuffer_all (void)
     {
       int legacy = 0;
 
+      run_fp = fp;
+      _IO_flockfile (fp);
+
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
       if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
 	legacy = 1;
@@ -800,18 +793,6 @@ _IO_unbuffer_all (void)
 	  /* Iff stream is un-orientated, it wasn't used. */
 	  && (legacy || fp->_mode != 0))
 	{
-#ifdef _IO_MTSAFE_IO
-	  int cnt;
-#define MAXTRIES 2
-	  for (cnt = 0; cnt < MAXTRIES; ++cnt)
-	    if (fp->_lock == NULL || _IO_lock_trylock (*fp->_lock) == 0)
-	      break;
-	    else
-	      /* Give the other thread time to finish up its use of the
-		 stream.  */
-	      __sched_yield ();
-#endif
-
 	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
@@ -825,17 +806,15 @@ _IO_unbuffer_all (void)
 
 	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
-
-#ifdef _IO_MTSAFE_IO
-	  if (cnt < MAXTRIES && fp->_lock != NULL)
-	    _IO_lock_unlock (*fp->_lock);
-#endif
 	}
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
       if (! legacy)
 	fp->_mode = -1;
+
+      _IO_funlockfile (fp);
+      run_fp = NULL;
     }
 
 #ifdef _IO_MTSAFE_IO
@@ -861,9 +840,7 @@ __libio_freemem (void)
 int
 _IO_cleanup (void)
 {
-  /* We do *not* want locking.  Some threads might use streams but
-     that is their problem, we flush them underneath them.  */
-  int result = _IO_flush_all_lockp (0);
+  int result = _IO_flush_all ();
 
   /* We currently don't have a reliable mechanism for making sure that
      C++ static destructors are executed in the correct order.
diff --git a/libio/libioP.h b/libio/libioP.h
index d777553cb0..745278e076 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -537,7 +537,6 @@ extern int _IO_new_do_write (FILE *, const char *, size_t);
 extern int _IO_old_do_write (FILE *, const char *, size_t);
 extern int _IO_wdo_write (FILE *, const wchar_t *, size_t);
 libc_hidden_proto (_IO_wdo_write)
-extern int _IO_flush_all_lockp (int);
 extern int _IO_flush_all (void);
 libc_hidden_proto (_IO_flush_all)
 extern void _IO_flush_all_linebuffered (void);
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index 5df1109dd3..855c273652 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -266,7 +266,6 @@ tests += \
   tst-spin3 \
   tst-spin4 \
   tst-stack1 \
-  tst-stdio1 \
   tst-stdio2 \
   tst-thrd-detach \
   tst-thrd-sleep \
diff --git a/sysdeps/pthread/tst-stdio1.c b/sysdeps/pthread/tst-stdio1.c
deleted file mode 100644
index 1266181505..0000000000
--- a/sysdeps/pthread/tst-stdio1.c
+++ /dev/null
@@ -1,55 +0,0 @@
-/* Copyright (C) 2002-2023 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <pthread.h>
-#include <signal.h>
-#include <stdio.h>
-#include <unistd.h>
-
-static int do_test (void);
-
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
-
-static void *tf (void *a)
-{
-  flockfile (stdout);
-  /* This call should never return.  */
-  return a;
-}
-
-
-int
-do_test (void)
-{
-  pthread_t th;
-
-  flockfile (stdout);
-
-  if (pthread_create (&th, NULL, tf, NULL) != 0)
-    {
-      write_message ("create failed\n");
-      _exit (1);
-    }
-
-  delayed_exit (1);
-  xpthread_join (th);
-
-  puts ("join returned");
-
-  return 1;
-}
-- 
2.41.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

Now that abort no longer calls fflush there is no reason to avoid locking
the stdio streams anywhere.  This fixes a conformance issue and potential
heap corruption during exit.  The test nptl/tst-stdio1 is removed as that
was expecting the problematic behaviour.
---
 libio/genops.c               | 43 +++++++---------------------
 libio/libioP.h               |  1 -
 sysdeps/pthread/Makefile     |  2 +-
 sysdeps/pthread/tst-stdio1.c | 55 ------------------------------------
 4 files changed, 11 insertions(+), 90 deletions(-)
 delete mode 100644 sysdeps/pthread/tst-stdio1.c

diff --git a/libio/genops.c b/libio/genops.c
index 1b629eb695..21fb118eae 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -682,7 +682,7 @@ _IO_adjust_column (unsigned start, const char *line, int count)
 libc_hidden_def (_IO_adjust_column)
 
 int
-_IO_flush_all_lockp (int do_lock)
+_IO_flush_all (void)
 {
   int result = 0;
   FILE *fp;
@@ -695,8 +695,7 @@ _IO_flush_all_lockp (int do_lock)
   for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
-      if (do_lock)
-	_IO_flockfile (fp);
+      _IO_flockfile (fp);
 
       if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
 	   || (_IO_vtable_offset (fp) == 0
@@ -706,8 +705,7 @@ _IO_flush_all_lockp (int do_lock)
 	  && _IO_OVERFLOW (fp, EOF) == EOF)
 	result = EOF;
 
-      if (do_lock)
-	_IO_funlockfile (fp);
+      _IO_funlockfile (fp);
       run_fp = NULL;
     }
 
@@ -718,14 +716,6 @@ _IO_flush_all_lockp (int do_lock)
 
   return result;
 }
-
-
-int
-_IO_flush_all (void)
-{
-  /* We want locking.  */
-  return _IO_flush_all_lockp (1);
-}
 libc_hidden_def (_IO_flush_all)
 
 void
@@ -791,6 +781,9 @@ _IO_unbuffer_all (void)
     {
       int legacy = 0;
 
+      run_fp = fp;
+      _IO_flockfile (fp);
+
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
       if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
 	legacy = 1;
@@ -800,18 +793,6 @@ _IO_unbuffer_all (void)
 	  /* Iff stream is un-orientated, it wasn't used. */
 	  && (legacy || fp->_mode != 0))
 	{
-#ifdef _IO_MTSAFE_IO
-	  int cnt;
-#define MAXTRIES 2
-	  for (cnt = 0; cnt < MAXTRIES; ++cnt)
-	    if (fp->_lock == NULL || _IO_lock_trylock (*fp->_lock) == 0)
-	      break;
-	    else
-	      /* Give the other thread time to finish up its use of the
-		 stream.  */
-	      __sched_yield ();
-#endif
-
 	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
@@ -825,17 +806,15 @@ _IO_unbuffer_all (void)
 
 	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
-
-#ifdef _IO_MTSAFE_IO
-	  if (cnt < MAXTRIES && fp->_lock != NULL)
-	    _IO_lock_unlock (*fp->_lock);
-#endif
 	}
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
       if (! legacy)
 	fp->_mode = -1;
+
+      _IO_funlockfile (fp);
+      run_fp = NULL;
     }
 
 #ifdef _IO_MTSAFE_IO
@@ -861,9 +840,7 @@ libc_freeres_fn (buffer_free)
 int
 _IO_cleanup (void)
 {
-  /* We do *not* want locking.  Some threads might use streams but
-     that is their problem, we flush them underneath them.  */
-  int result = _IO_flush_all_lockp (0);
+  int result = _IO_flush_all ();
 
   /* We currently don't have a reliable mechanism for making sure that
      C++ static destructors are executed in the correct order.
diff --git a/libio/libioP.h b/libio/libioP.h
index ba4fdbd200..88342b1bfa 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -487,7 +487,6 @@ extern int _IO_new_do_write (FILE *, const char *, size_t);
 extern int _IO_old_do_write (FILE *, const char *, size_t);
 extern int _IO_wdo_write (FILE *, const wchar_t *, size_t);
 libc_hidden_proto (_IO_wdo_write)
-extern int _IO_flush_all_lockp (int);
 extern int _IO_flush_all (void);
 libc_hidden_proto (_IO_flush_all)
 extern int _IO_cleanup (void);
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index 7d1670da87..b88914b755 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -111,7 +111,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
 	 tst-signal4 tst-signal5 tst-signal6 tst-signal8 \
 	 tst-spin1 tst-spin2 tst-spin3 tst-spin4 \
 	 tst-stack1 \
-	 tst-stdio1 tst-stdio2 \
+	 tst-stdio2 \
 	 tst-pt-sysconf \
 	 tst-pt-tls1 tst-pt-tls2 \
 	 tst-tsd1 tst-tsd2 tst-tsd5 tst-tsd6 \
diff --git a/sysdeps/pthread/tst-stdio1.c b/sysdeps/pthread/tst-stdio1.c
deleted file mode 100644
index 568e516a30..0000000000
--- a/sysdeps/pthread/tst-stdio1.c
+++ /dev/null
@@ -1,55 +0,0 @@
-/* Copyright (C) 2002-2022 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <pthread.h>
-#include <signal.h>
-#include <stdio.h>
-#include <unistd.h>
-
-static int do_test (void);
-
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
-
-static void *tf (void *a)
-{
-  flockfile (stdout);
-  /* This call should never return.  */
-  return a;
-}
-
-
-int
-do_test (void)
-{
-  pthread_t th;
-
-  flockfile (stdout);
-
-  if (pthread_create (&th, NULL, tf, NULL) != 0)
-    {
-      write_message ("create failed\n");
-      _exit (1);
-    }
-
-  delayed_exit (1);
-  xpthread_join (th);
-
-  puts ("join returned");
-
-  return 1;
-}
-- 
2.37.1


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

Now that abort no longer calls fflush there is no reason to avoid locking
the stdio streams anywhere.  This fixes a conformance issue and potential
heap corruption during exit.  The test nptl/tst-stdio1 is removed as that
was expecting the problematic behaviour.
---
 libio/genops.c    | 43 +++++++++---------------------------
 libio/libioP.h    |  1 -
 nptl/Makefile     |  2 +-
 nptl/tst-stdio1.c | 56 -----------------------------------------------
 4 files changed, 11 insertions(+), 91 deletions(-)
 delete mode 100644 nptl/tst-stdio1.c

diff --git a/libio/genops.c b/libio/genops.c
index 28419cc963..0fbdbeca7c 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -682,7 +682,7 @@ _IO_adjust_column (unsigned start, const char *line, int count)
 libc_hidden_def (_IO_adjust_column)
 
 int
-_IO_flush_all_lockp (int do_lock)
+_IO_flush_all (void)
 {
   int result = 0;
   FILE *fp;
@@ -695,8 +695,7 @@ _IO_flush_all_lockp (int do_lock)
   for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
-      if (do_lock)
-	_IO_flockfile (fp);
+      _IO_flockfile (fp);
 
       if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
 	   || (_IO_vtable_offset (fp) == 0
@@ -706,8 +705,7 @@ _IO_flush_all_lockp (int do_lock)
 	  && _IO_OVERFLOW (fp, EOF) == EOF)
 	result = EOF;
 
-      if (do_lock)
-	_IO_funlockfile (fp);
+      _IO_funlockfile (fp);
       run_fp = NULL;
     }
 
@@ -718,14 +716,6 @@ _IO_flush_all_lockp (int do_lock)
 
   return result;
 }
-
-
-int
-_IO_flush_all (void)
-{
-  /* We want locking.  */
-  return _IO_flush_all_lockp (1);
-}
 libc_hidden_def (_IO_flush_all)
 
 void
@@ -791,6 +781,9 @@ _IO_unbuffer_all (void)
     {
       int legacy = 0;
 
+      run_fp = fp;
+      _IO_flockfile (fp);
+
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
       if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
 	legacy = 1;
@@ -800,18 +793,6 @@ _IO_unbuffer_all (void)
 	  /* Iff stream is un-orientated, it wasn't used. */
 	  && (legacy || fp->_mode != 0))
 	{
-#ifdef _IO_MTSAFE_IO
-	  int cnt;
-#define MAXTRIES 2
-	  for (cnt = 0; cnt < MAXTRIES; ++cnt)
-	    if (fp->_lock == NULL || _IO_lock_trylock (*fp->_lock) == 0)
-	      break;
-	    else
-	      /* Give the other thread time to finish up its use of the
-		 stream.  */
-	      __sched_yield ();
-#endif
-
 	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
@@ -825,17 +806,15 @@ _IO_unbuffer_all (void)
 
 	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
-
-#ifdef _IO_MTSAFE_IO
-	  if (cnt < MAXTRIES && fp->_lock != NULL)
-	    _IO_lock_unlock (*fp->_lock);
-#endif
 	}
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
       if (! legacy)
 	fp->_mode = -1;
+
+      _IO_funlockfile (fp);
+      run_fp = NULL;
     }
 
 #ifdef _IO_MTSAFE_IO
@@ -861,9 +840,7 @@ libc_freeres_fn (buffer_free)
 int
 _IO_cleanup (void)
 {
-  /* We do *not* want locking.  Some threads might use streams but
-     that is their problem, we flush them underneath them.  */
-  int result = _IO_flush_all_lockp (0);
+  int result = _IO_flush_all ();
 
   /* We currently don't have a reliable mechanism for making sure that
      C++ static destructors are executed in the correct order.
diff --git a/libio/libioP.h b/libio/libioP.h
index 7e446e3030..55697b27cb 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -487,7 +487,6 @@ extern int _IO_new_do_write (FILE *, const char *, size_t);
 extern int _IO_old_do_write (FILE *, const char *, size_t);
 extern int _IO_wdo_write (FILE *, const wchar_t *, size_t);
 libc_hidden_proto (_IO_wdo_write)
-extern int _IO_flush_all_lockp (int);
 extern int _IO_flush_all (void);
 libc_hidden_proto (_IO_flush_all)
 extern int _IO_cleanup (void);
diff --git a/nptl/Makefile b/nptl/Makefile
index 6f210d60e3..d248f1ddb6 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -277,7 +277,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
 	tst-signal6 \
 	tst-exec1 tst-exec2 tst-exec3 tst-exec4 tst-exec5 \
 	tst-exit1 tst-exit2 tst-exit3 \
-	tst-stdio1 tst-stdio2 \
+	tst-stdio2 \
 	tst-stack1 tst-stack2 tst-stack3 tst-stack4 \
 	tst-pthread-attr-affinity tst-pthread-mutexattr \
 	tst-unload \
diff --git a/nptl/tst-stdio1.c b/nptl/tst-stdio1.c
deleted file mode 100644
index 66696a92ee..0000000000
--- a/nptl/tst-stdio1.c
+++ /dev/null
@@ -1,56 +0,0 @@
-/* Copyright (C) 2002-2020 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <pthread.h>
-#include <signal.h>
-#include <stdio.h>
-#include <unistd.h>
-
-static int do_test (void);
-
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
-
-static void *tf (void *a)
-{
-  flockfile (stdout);
-  /* This call should never return.  */
-  return a;
-}
-
-
-int
-do_test (void)
-{
-  pthread_t th;
-
-  flockfile (stdout);
-
-  if (pthread_create (&th, NULL, tf, NULL) != 0)
-    {
-      write_message ("create failed\n");
-      _exit (1);
-    }
-
-  delayed_exit (1);
-  xpthread_join (th);
-
-  puts ("join returned");
-
-  return 1;
-}
-- 
2.25.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Florian Weimer]

* Andreas Schwab:

> Now that abort no longer calls fflush there is no reason to avoid locking
> the stdio streams anywhere.  This fixes a conformance issue and potential
> heap corruption during exit.  The test nptl/tst-stdio1 is removed as that
> was expecting the problematic behaviour.

I'm not sure if the justification is correct.  We don't know if there
are programs which call flockfile on some file and expect exit to
still work (and terminate the program), which is part of what
nptl/tst-stdio1 tests.

If there are such programs, we may need to make the new behavior
conditional on a new symbol version for exit.  But I guess we can put
in your patch now and see if testing shows any problems.

Re: [PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

On Feb 16 2020, Florian Weimer wrote:

> I'm not sure if the justification is correct.  We don't know if there
> are programs which call flockfile on some file and expect exit to
> still work (and terminate the program), which is part of what
> nptl/tst-stdio1 tests.

But that's exactly what the bug is about.  Random memory corruption
cannot be the answer.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Florian Weimer]

* Andreas Schwab:

> On Feb 16 2020, Florian Weimer wrote:
>
>> I'm not sure if the justification is correct.  We don't know if there
>> are programs which call flockfile on some file and expect exit to
>> still work (and terminate the program), which is part of what
>> nptl/tst-stdio1 tests.
>
> But that's exactly what the bug is about.  Random memory corruption
> cannot be the answer.

Sure, but breaking existing programs so that they never terminate is
not acceptable, either.

(They might employ external locking and not have any data races, so
they don't need this change.)

Just to be clear, I think your patch is worth a try, we just have to
be prepared to revert it and replace it with symbol versioning for
exit.

Re: [PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

On Feb 17 2020, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Feb 16 2020, Florian Weimer wrote:
>>
>>> I'm not sure if the justification is correct.  We don't know if there
>>> are programs which call flockfile on some file and expect exit to
>>> still work (and terminate the program), which is part of what
>>> nptl/tst-stdio1 tests.
>>
>> But that's exactly what the bug is about.  Random memory corruption
>> cannot be the answer.
>
> Sure, but breaking existing programs so that they never terminate is
> not acceptable, either.

They were always erroneous.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Florian Weimer]

* Andreas Schwab:

> On Feb 17 2020, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> On Feb 16 2020, Florian Weimer wrote:
>>>
>>>> I'm not sure if the justification is correct.  We don't know if there
>>>> are programs which call flockfile on some file and expect exit to
>>>> still work (and terminate the program), which is part of what
>>>> nptl/tst-stdio1 tests.
>>>
>>> But that's exactly what the bug is about.  Random memory corruption
>>> cannot be the answer.
>>
>> Sure, but breaking existing programs so that they never terminate is
>> not acceptable, either.
>
> They were always erroneous.

Yes, but we might be heading towards another memcpy or glob situation
here, where we had to bump symbol versions to keep important (but broken)
software running.

[PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

Now that abort no longer calls fflush there is no reason to avoid locking
the stdio streams anywhere.  This fixes a conformance issue and potential
heap corruption during exit.  The test nptl/tst-stdio1 is removed as that
was expecting the problematic behaviour.
---
 libio/genops.c    | 43 +++++++++---------------------------
 libio/libioP.h    |  1 -
 nptl/Makefile     |  2 +-
 nptl/tst-stdio1.c | 56 -----------------------------------------------
 4 files changed, 11 insertions(+), 91 deletions(-)
 delete mode 100644 nptl/tst-stdio1.c

diff --git a/libio/genops.c b/libio/genops.c
index f871e7751b..9afe81bd00 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -682,7 +682,7 @@ _IO_adjust_column (unsigned start, const char *line, int count)
 libc_hidden_def (_IO_adjust_column)
 
 int
-_IO_flush_all_lockp (int do_lock)
+_IO_flush_all (void)
 {
   int result = 0;
   FILE *fp;
@@ -695,8 +695,7 @@ _IO_flush_all_lockp (int do_lock)
   for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
-      if (do_lock)
-	_IO_flockfile (fp);
+      _IO_flockfile (fp);
 
       if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
 	   || (_IO_vtable_offset (fp) == 0
@@ -706,8 +705,7 @@ _IO_flush_all_lockp (int do_lock)
 	  && _IO_OVERFLOW (fp, EOF) == EOF)
 	result = EOF;
 
-      if (do_lock)
-	_IO_funlockfile (fp);
+      _IO_funlockfile (fp);
       run_fp = NULL;
     }
 
@@ -718,14 +716,6 @@ _IO_flush_all_lockp (int do_lock)
 
   return result;
 }
-
-
-int
-_IO_flush_all (void)
-{
-  /* We want locking.  */
-  return _IO_flush_all_lockp (1);
-}
 libc_hidden_def (_IO_flush_all)
 
 void
@@ -791,6 +781,9 @@ _IO_unbuffer_all (void)
     {
       int legacy = 0;
 
+      run_fp = fp;
+      _IO_flockfile (fp);
+
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
       if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
 	legacy = 1;
@@ -800,18 +793,6 @@ _IO_unbuffer_all (void)
 	  /* Iff stream is un-orientated, it wasn't used. */
 	  && (legacy || fp->_mode != 0))
 	{
-#ifdef _IO_MTSAFE_IO
-	  int cnt;
-#define MAXTRIES 2
-	  for (cnt = 0; cnt < MAXTRIES; ++cnt)
-	    if (fp->_lock == NULL || _IO_lock_trylock (*fp->_lock) == 0)
-	      break;
-	    else
-	      /* Give the other thread time to finish up its use of the
-		 stream.  */
-	      __sched_yield ();
-#endif
-
 	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
@@ -825,17 +806,15 @@ _IO_unbuffer_all (void)
 
 	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
-
-#ifdef _IO_MTSAFE_IO
-	  if (cnt < MAXTRIES && fp->_lock != NULL)
-	    _IO_lock_unlock (*fp->_lock);
-#endif
 	}
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
       if (! legacy)
 	fp->_mode = -1;
+
+      _IO_funlockfile (fp);
+      run_fp = NULL;
     }
 
 #ifdef _IO_MTSAFE_IO
@@ -861,9 +840,7 @@ libc_freeres_fn (buffer_free)
 int
 _IO_cleanup (void)
 {
-  /* We do *not* want locking.  Some threads might use streams but
-     that is their problem, we flush them underneath them.  */
-  int result = _IO_flush_all_lockp (0);
+  int result = _IO_flush_all ();
 
   /* We currently don't have a reliable mechanism for making sure that
      C++ static destructors are executed in the correct order.
diff --git a/libio/libioP.h b/libio/libioP.h
index 3787605cfb..2187b8edbe 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -487,7 +487,6 @@ extern int _IO_new_do_write (FILE *, const char *, size_t);
 extern int _IO_old_do_write (FILE *, const char *, size_t);
 extern int _IO_wdo_write (FILE *, const wchar_t *, size_t);
 libc_hidden_proto (_IO_wdo_write)
-extern int _IO_flush_all_lockp (int);
 extern int _IO_flush_all (void);
 libc_hidden_proto (_IO_flush_all)
 extern int _IO_cleanup (void);
diff --git a/nptl/Makefile b/nptl/Makefile
index fffd9462ff..74e0de0ce2 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -297,7 +297,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
 	tst-signal6 \
 	tst-exec1 tst-exec2 tst-exec3 tst-exec4 tst-exec5 \
 	tst-exit1 tst-exit2 tst-exit3 \
-	tst-stdio1 tst-stdio2 \
+	tst-stdio2 \
 	tst-stack1 tst-stack2 tst-stack3 tst-stack4 tst-pthread-getattr \
 	tst-pthread-attr-affinity tst-pthread-mutexattr \
 	tst-unload \
diff --git a/nptl/tst-stdio1.c b/nptl/tst-stdio1.c
deleted file mode 100644
index aec2f658d4..0000000000
--- a/nptl/tst-stdio1.c
+++ /dev/null
@@ -1,56 +0,0 @@
-/* Copyright (C) 2002-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <pthread.h>
-#include <signal.h>
-#include <stdio.h>
-#include <unistd.h>
-
-static int do_test (void);
-
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
-
-static void *tf (void *a)
-{
-  flockfile (stdout);
-  /* This call should never return.  */
-  return a;
-}
-
-
-int
-do_test (void)
-{
-  pthread_t th;
-
-  flockfile (stdout);
-
-  if (pthread_create (&th, NULL, tf, NULL) != 0)
-    {
-      write_message ("create failed\n");
-      _exit (1);
-    }
-
-  delayed_exit (1);
-  xpthread_join (th);
-
-  puts ("join returned");
-
-  return 1;
-}
-- 
2.24.1

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Andreas Schwab]

Now that abort no longer calls fflush there is no reason to avoid locking
the stdio streams anywhere.
---
 libio/genops.c | 43 ++++++++++---------------------------------
 libio/libioP.h |  1 -
 2 files changed, 10 insertions(+), 34 deletions(-)

diff --git a/libio/genops.c b/libio/genops.c
index f871e7751b..9afe81bd00 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -682,7 +682,7 @@ _IO_adjust_column (unsigned start, const char *line, int count)
 libc_hidden_def (_IO_adjust_column)
 
 int
-_IO_flush_all_lockp (int do_lock)
+_IO_flush_all (void)
 {
   int result = 0;
   FILE *fp;
@@ -695,8 +695,7 @@ _IO_flush_all_lockp (int do_lock)
   for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
-      if (do_lock)
-	_IO_flockfile (fp);
+      _IO_flockfile (fp);
 
       if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
 	   || (_IO_vtable_offset (fp) == 0
@@ -706,8 +705,7 @@ _IO_flush_all_lockp (int do_lock)
 	  && _IO_OVERFLOW (fp, EOF) == EOF)
 	result = EOF;
 
-      if (do_lock)
-	_IO_funlockfile (fp);
+      _IO_funlockfile (fp);
       run_fp = NULL;
     }
 
@@ -718,14 +716,6 @@ _IO_flush_all_lockp (int do_lock)
 
   return result;
 }
-
-
-int
-_IO_flush_all (void)
-{
-  /* We want locking.  */
-  return _IO_flush_all_lockp (1);
-}
 libc_hidden_def (_IO_flush_all)
 
 void
@@ -791,6 +781,9 @@ _IO_unbuffer_all (void)
     {
       int legacy = 0;
 
+      run_fp = fp;
+      _IO_flockfile (fp);
+
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
       if (__glibc_unlikely (_IO_vtable_offset (fp) != 0))
 	legacy = 1;
@@ -800,18 +793,6 @@ _IO_unbuffer_all (void)
 	  /* Iff stream is un-orientated, it wasn't used. */
 	  && (legacy || fp->_mode != 0))
 	{
-#ifdef _IO_MTSAFE_IO
-	  int cnt;
-#define MAXTRIES 2
-	  for (cnt = 0; cnt < MAXTRIES; ++cnt)
-	    if (fp->_lock == NULL || _IO_lock_trylock (*fp->_lock) == 0)
-	      break;
-	    else
-	      /* Give the other thread time to finish up its use of the
-		 stream.  */
-	      __sched_yield ();
-#endif
-
 	  if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF))
 	    {
 	      fp->_flags |= _IO_USER_BUF;
@@ -825,17 +806,15 @@ _IO_unbuffer_all (void)
 
 	  if (! legacy && fp->_mode > 0)
 	    _IO_wsetb (fp, NULL, NULL, 0);
-
-#ifdef _IO_MTSAFE_IO
-	  if (cnt < MAXTRIES && fp->_lock != NULL)
-	    _IO_lock_unlock (*fp->_lock);
-#endif
 	}
 
       /* Make sure that never again the wide char functions can be
 	 used.  */
       if (! legacy)
 	fp->_mode = -1;
+
+      _IO_funlockfile (fp);
+      run_fp = NULL;
     }
 
 #ifdef _IO_MTSAFE_IO
@@ -861,9 +840,7 @@ libc_freeres_fn (buffer_free)
 int
 _IO_cleanup (void)
 {
-  /* We do *not* want locking.  Some threads might use streams but
-     that is their problem, we flush them underneath them.  */
-  int result = _IO_flush_all_lockp (0);
+  int result = _IO_flush_all ();
 
   /* We currently don't have a reliable mechanism for making sure that
      C++ static destructors are executed in the correct order.
diff --git a/libio/libioP.h b/libio/libioP.h
index 3787605cfb..2187b8edbe 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -487,7 +487,6 @@ extern int _IO_new_do_write (FILE *, const char *, size_t);
 extern int _IO_old_do_write (FILE *, const char *, size_t);
 extern int _IO_wdo_write (FILE *, const wchar_t *, size_t);
 libc_hidden_proto (_IO_wdo_write)
-extern int _IO_flush_all_lockp (int);
 extern int _IO_flush_all (void);
 libc_hidden_proto (_IO_flush_all)
 extern int _IO_cleanup (void);
-- 
2.24.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Always do locking when accessing streams (bug 15142, bug 14697)

[Szabolcs Nagy]

On 25/11/2019 14:20, Andreas Schwab wrote:
> Now that abort no longer calls fflush there is no reason to avoid locking
> the stdio streams anywhere.

the patch looks ok to me.

note that it may expose an issue mentioned in

https://sourceware.org/bugzilla/show_bug.cgi?id=24963

concurrent fflush(0) and freopen(fp) may deadlock
as they choose different lock ordering.