[PATCH] libio: Fix deadlock between freopen, fflush (NULL) and fclose (bug 24963)

[Florian Weimer]

* Arjun Shankar:

>> diff --git a/libio/iofclose.c b/libio/iofclose.c
>> index a945dff396..a7d52846e3 100644
>> --- a/libio/iofclose.c
>> +++ b/libio/iofclose.c
>> @@ -44,16 +44,26 @@ _IO_new_fclose (FILE *fp)
>>      return _IO_old_fclose (fp);
>>  #endif
>>
>> -  /* First unlink the stream.  */
>> -  if (fp->_flags & _IO_IS_FILEBUF)
>> -    _IO_un_link ((struct _IO_FILE_plus *) fp);
>> -
>
> OK. Don't unlink for now.
>
>>    _IO_acquire_lock (fp);
>>    if (fp->_flags & _IO_IS_FILEBUF)
>> -    status = _IO_file_close_it (fp);
>> +    {
>> +      status = _IO_file_close_maybe_unlink (fp, false);
>> +      /* Skip future flushing.  */
>> +      fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
>> +    }
>
> OK. Still don't unlink when closing.
>
>>    else
>>      status = fp->_flags & _IO_ERR_SEEN ? -1 : 0;
>>    _IO_release_lock (fp);
>> +
>> +  /* Unlink after releasing the lock on fp.  This maintains the usual
>> +     locking order (list_all_lock acquired first, then the fp lock).
>> +     The only valid reference to fp after a call to fclose is the
>> +     implicit reference to it as part of fflush (NULL).  The
>> +     _IO_un_link callhere synchronizes with fflush (NULL).  Future
>> +     interaction with fflush (NULL) is not possible because the stream
>> +     is no longer on the list.  */
>> +  _IO_un_link ((struct _IO_FILE_plus *) fp);
>> +
>
> OK. Unlink at the end.
>
> I'm going to trace the lock acquisition/release sequences for this function:
>
> The earlier operation and lock order was:
> 1. unlink, i.e.: lock list -> lock file -> unlock file -> unlock list
> 2. lock file
> 3. close file, which unlinks unconditionally, but: the unlink is a
> no-op and takes no lock because of the unset _IO_LINKED flag
> 4. unlock file
>
> Now, the order of operations including lock order is:
> 1. lock file
> 2. close file without unlink
> 3. release file lock
> 4. unlink: lock list, lock file, unlock file, unlock list
>
> Actually, both orders seem fine to me.
>
> The setting of NOCLOSE, which is an additional change, is only made
> necessary by the new delayed unlink. The early unlink would ensure
> that a concurrent flush_all never sees this file mid-closure.
>
> I believe the fix still works if we don't change this function. Just
> to test the idea out, I reverted the change to this function and
> re-ran the test with 1+5+5 threads (instead of 1+2+2) for 300 seconds
> (and with a longer timeout) and it ran to completion.

I'm not sure we can test the fclose behavior without triggering
undefined behavior.  The approach suggested on bug 24963 is undefined.
As the test doesn't cover it, removing these changes does not cause the
test to fail.

I still think it makes sense to adjust fclose lock ordering to match.

Thanks,
Florian