__fwalk mutex problem

[Antony KING]

Hi,

This has reminded me of a patch I posted a couple of years ago to fix an
almost identical issue I was seeing with our RTOS. See the following for
details:

  http://sourceware.org/ml/newlib/2006/msg00871.html (original patch)
  http://sourceware.org/ml/newlib/2007/msg00840.html (follow up)
  http://sourceware.org/ml/newlib/2007/msg00845.html (follow up)

The fwalk issue is now addressed in the CVS head (although the now
unnecessary "if (fp->_flags != 0)" could be removed), but the other fix
in the patch (against fclose.c) is still relevant (for my RTOS at least
:-) ).

Cheers,

Antony.

Jeff Johnston wrote:
> Hans,
> 
> There is a known problem with the locking stuff in stdio.  I had already
> fixed fwalk.c to not
> lock and unlock the files (i.e. it was up to the called function to do
> so if needed).  That is already checked
> in the repository.
> 
> Now, there is also a deadlock caused by not enforcing the ordering of
> locks for reading/fclose.  What is happening is fread gets an fp-lock. 
> fclose comes along in another thread and gets the sfp-lock, then tries
> for the fp-lock but has to wait for the read to finish.  The read can
> possibly try and get the sfp-lock and is blocked because fclose has it
> but can't finish.  Deadlock.  The read must try the sfp-lock before
> locking fp.
> 
> I had created a patch for another user to try out, but they didn't
> report any success.  If you would like to try it out, see the attached
> patch.  What it does is ensure that we always lock the sfp lock before
> any file locks.  If you find any problems, feel free to let me know.
> 
> BTW: the lflush function calls fflush which has file locks in it already.
> 
> -- Jeff J.
> 
> Hans-Erik Floryd wrote:
>> Hello,
>>
>> The __fwalk and __fwalk_reent functions lock the file mutex before
>> calling the walk function. They unlock the mutex when the function
>> returns.
>>
>>   for (g = &ptr->__sglue; g != NULL; g = g->_next)
>>     for (fp = g->_iobs, n = g->_niobs; --n >= 0; fp++)
>>       if (fp->_flags != 0)
>>         {
>>           _flockfile (fp);
>>           if (fp->_flags != 0 && fp->_file != -1)
>>             ret |= (*function) (fp);
>>           _funlockfile (fp);
>>         }
>>
>> When you call __fwalk(ptr, fclose), as _cleanup_r does, there is a
>> problem because fclose will release the mutex. When the function
>> returns  __fwalk will call _funlockfile on a file that is closed.
>>
>> With our mutex implementation that gives an error, because a released
>> mutex is flagged as invalid.
>>
>> Is it necessary for __fwalk to lock the file before calling the walk
>> function? When __fwalk is called with fclose and fflush it seems
>> unnecessary, because both of those functions lock the mutex internally.
>>
>> In __srefill_r there is a call to _fwalk with lflush as the walk
>> function. It calls fflush after inspecting fp->_flags, is the lock
>> needed in this case? If so lflush could be modified to lock the file
>> internally.
>>
>> (There is also __fp_lock_all and __fp_unlock_all to consider. I am not
>> sure how these are used though, couldn't find a reference anywhere).
>>
>> Cheers,
>>  Hans-Erik Floryd