Always do locking when accessing streams (bug 15142)
diff mbox series

Message ID mvmpnhwvx68.fsf@suse.de
State New
Headers show
Series
  • Always do locking when accessing streams (bug 15142)
Related show

Commit Message

Andreas Schwab Nov. 13, 2019, 11:21 a.m. UTC
During exit, skip files that are currently locked to avoid deadlock.
---
 libio/genops.c | 54 ++++++++++++++++++++++++++------------------------
 libio/libio.h  |  4 ++++
 libio/libioP.h |  1 -
 3 files changed, 32 insertions(+), 27 deletions(-)

Comments

Florian Weimer Nov. 21, 2019, 12:25 p.m. UTC | #1
* Andreas Schwab:

> During exit, skip files that are currently locked to avoid deadlock.

Which locks are taken during the deadlock?

Does this fix or re-open bug 15142?

> +  /* We want to skip locked streams.  Some threads might use streams but
> +     that is their problem, we don't flush those.  */
> +  int result = _IO_flush_all_lockp (true);

Is this still conforming to POSIX?

Sorry, it's not clear to me how this patch improves matters, and if the
direction is ultimately the right one.

Thanks,
Florian
Andreas Schwab Nov. 21, 2019, 1:14 p.m. UTC | #2
On Nov 21 2019, Florian Weimer wrote:

> * Andreas Schwab:
>
>> During exit, skip files that are currently locked to avoid deadlock.
>
> Which locks are taken during the deadlock?

Presumably any FILE locks, if there are other threads still running.

> Does this fix or re-open bug 15142?

Why re-open?

>> +  /* We want to skip locked streams.  Some threads might use streams but
>> +     that is their problem, we don't flush those.  */
>> +  int result = _IO_flush_all_lockp (true);
>
> Is this still conforming to POSIX?

Are deadlocks conforming to POSIX?

Andreas.
Florian Weimer Nov. 21, 2019, 1:20 p.m. UTC | #3
* Andreas Schwab:

> On Nov 21 2019, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> During exit, skip files that are currently locked to avoid deadlock.
>>
>> Which locks are taken during the deadlock?
>
> Presumably any FILE locks, if there are other threads still running.

Okay, so the problem is simply lack of progress after calling exit?

>> Does this fix or re-open bug 15142?
>
> Why re-open?

Lack of flushing of streams on process exit.

>>> +  /* We want to skip locked streams.  Some threads might use streams but
>>> +     that is their problem, we don't flush those.  */
>>> +  int result = _IO_flush_all_lockp (true);
>>
>> Is this still conforming to POSIX?
>
> Are deadlocks conforming to POSIX?

I think the last time we discussed this, the conclusion was that lack of
progress was conforming to POSIX (actually required by it).

What has changed since then?

Thanks,
Florian
Andreas Schwab Nov. 21, 2019, 1:27 p.m. UTC | #4
On Nov 21 2019, Florian Weimer wrote:

>>> Does this fix or re-open bug 15142?
>>
>> Why re-open?
>
> Lack of flushing of streams on process exit.

I don't understand what you are trying to tell me.

> I think the last time we discussed this,

Do you have a pointer?

Andreas.
Florian Weimer Nov. 25, 2019, 1:30 p.m. UTC | #5
* Andreas Schwab:

> On Nov 21 2019, Florian Weimer wrote:
>
>>>> Does this fix or re-open bug 15142?
>>>
>>> Why re-open?
>>
>> Lack of flushing of streams on process exit.
>
> I don't understand what you are trying to tell me.

Hmm.  Bug 15142 is not the bug, but don't we have a bug somewhere about
not flushing streams on exit (as opposed to abort)?

>> I think the last time we discussed this,
>
> Do you have a pointer?

I can't find the discussion.  But I think we discussed it somewhere.
The main question is whether it is acceptable for programs to hang
indefinitely if a flushable stream is locked for some reason.  POSIX
actually seems to require that behavior, but I'm not sure how
backwards-compatible it is.  There are probably quite a few libraries
which store a FILE * internally, use flockfile to get performance, and
synchronize access to the file by other means.

Thanks,
Florian
Andreas Schwab Nov. 25, 2019, 1:34 p.m. UTC | #6
On Nov 25 2019, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Nov 21 2019, Florian Weimer wrote:
>>
>>>>> Does this fix or re-open bug 15142?
>>>>
>>>> Why re-open?
>>>
>>> Lack of flushing of streams on process exit.
>>
>> I don't understand what you are trying to tell me.
>
> Hmm.  Bug 15142 is not the bug, but don't we have a bug somewhere about
> not flushing streams on exit (as opposed to abort)?

Bug 14697 perhaps?

Andreas.
Florian Weimer Nov. 25, 2019, 1:35 p.m. UTC | #7
* Andreas Schwab:

> On Nov 25 2019, Florian Weimer wrote:
>
>> * Andreas Schwab:
>>
>>> On Nov 21 2019, Florian Weimer wrote:
>>>
>>>>>> Does this fix or re-open bug 15142?
>>>>>
>>>>> Why re-open?
>>>>
>>>> Lack of flushing of streams on process exit.
>>>
>>> I don't understand what you are trying to tell me.
>>
>> Hmm.  Bug 15142 is not the bug, but don't we have a bug somewhere about
>> not flushing streams on exit (as opposed to abort)?
>
> Bug 14697 perhaps?

Yes, that's it.

Thanks,
Florian

Patch
diff mbox series

diff --git a/libio/genops.c b/libio/genops.c
index f871e7751b..30daa169ef 100644
--- a/libio/genops.c
+++ b/libio/genops.c
@@ -681,8 +681,8 @@  _IO_adjust_column (unsigned start, const char *line, int count)
 }
 libc_hidden_def (_IO_adjust_column)
 
-int
-_IO_flush_all_lockp (int do_lock)
+static int
+_IO_flush_all_lockp (bool skip_locked)
 {
   int result = 0;
   FILE *fp;
@@ -695,7 +695,16 @@  _IO_flush_all_lockp (int do_lock)
   for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain)
     {
       run_fp = fp;
-      if (do_lock)
+      if (skip_locked)
+	{
+	  /* Skip files that are currently locked.  */
+	  if (_IO_ftrylockfile (fp))
+	    {
+	      run_fp = NULL;
+	      continue;
+	    }
+	}
+      else
 	_IO_flockfile (fp);
 
       if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
@@ -706,8 +715,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;
     }
 
@@ -724,7 +732,7 @@  int
 _IO_flush_all (void)
 {
   /* We want locking.  */
-  return _IO_flush_all_lockp (1);
+  return _IO_flush_all_lockp (false);
 }
 libc_hidden_def (_IO_flush_all)
 
@@ -789,6 +797,14 @@  _IO_unbuffer_all (void)
 
   for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain)
     {
+      run_fp = fp;
+      /* Skip files that are currently locked.  */
+      if (_IO_ftrylockfile (fp))
+	{
+	  run_fp = NULL;
+	  continue;
+	}
+
       int legacy = 0;
 
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1)
@@ -800,18 +816,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 +829,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 +863,9 @@  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);
+  /* We want to skip locked streams.  Some threads might use streams but
+     that is their problem, we don't flush those.  */
+  int result = _IO_flush_all_lockp (true);
 
   /* 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/libio.h b/libio/libio.h
index bed324e57d..3e660bd454 100644
--- a/libio/libio.h
+++ b/libio/libio.h
@@ -291,11 +291,15 @@  libc_hidden_proto (_IO_sgetn)
   if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_lock (*(_fp)->_lock)
 #  define _IO_funlockfile(_fp) \
   if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_unlock (*(_fp)->_lock)
+#  define _IO_ftrylockfile(_fp) \
+  (((_fp)->_flags & _IO_USER_LOCK) == 0 ? _IO_lock_trylock (*(_fp)->_lock) : 0)
 # else
 #  define _IO_flockfile(_fp) \
   if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_flockfile (_fp)
 #  define _IO_funlockfile(_fp) \
   if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_funlockfile (_fp)
+#  define _IO_ftrylockfile(_fp) \
+  (((_fp)->_flags & _IO_USER_LOCK) == 0 ? _IO_ftrylockfile (_fp) : 0)
 # endif
 #endif /* _IO_MTSAFE_IO */
 
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);