[RFA]: race safe fwalk

[Jeff Johnston]

Thomas Pfaff wrote:
> Jeff Johnston wrote:
> 
>> Thomas Pfaff wrote:
>>
>>> This time with attachment.
>>>
>>> There is a possible race between fwalk and fopen:
>>>
>>> When a thread make a call to fopen the FILE * _flags will be set to 1 
>>> in findfp to mark it used and later it will be changed to the real 
>>> FILE flag.
>>>
>>> When another thread calls fwalk during that time fwalk will treat the 
>>> FILE as already opened and calls the callback functions with the yet 
>>> unopened and only partially initialized FILE *.
>>>
>>> This can be avoided by checking for fp->_flags != 0 && fp->_flags != 
>>> 1. Since _flags is signed short i did not check for _flags > 1. The 
>>> flag should be set as the last step in an open call.
>>> I do not think that 1 is a valid _flag for an open file. Correct me 
>>> if am wrong.
>>>
>>
>> Unfortunately, 1 is also line-buffered: __SLBF.
>>
>> What if instead, we use the _file field to check for a valid file.  It 
>> gets set to -1 by __sfp.  Now, if we set _file inside the __sfp_lock 
>> and then had fwalk() use the __sfp lock as well and check for _file != 
>> -1, plus have the open routines set the _file field last, this should 
>> work equally as well.  Comments?
>>
> 
> I do not think that this can be done that easy anymore. The whole file 
> list stuff must be better protected.
> 
> Attached is a new patch. I am sorry for the delay.
>

No problem.  I had to do some tweaking to get Linux to build with the change and 
I added a missing include in freopen64.c.  Patch checked in.  Thanks.

-- Jeff J.

> 2004-03-22  Thomas Pfaff  <tpfaff@gmx.net>
> 
>     * libc/stdio/fclose.c (fclose): Protect file pointer list when
>     releasing a file.
>      * libc/stdio/fcloseall.c (_fcloseall_r): Close all files via
>     fwalk.
>     * libc/stdio/fdopen.c (_fdopen_r): Add calls to
>     _flockfile/_funlockfile.
>     * libc/stdio/findfp.c: Move __sfp_lock. Change __sfp_lock type
>     to recursive.
>     Change __lock_acquire/__lock_release calls for __sfp_lock to
>     __sfp_lock_acquire/__sfp_lock_release throughout.
>     (std): Make sure that file lock is only initialized once.
>     (__sfp): Move _file initialization. Initialize file lock.
>     (__sfp_lock_acquire): New function.
>     (__sfp_lock_release): Ditto.
>     (__fp_lock_all): Remove __sfp_lock_acquire call.
>     (__fp_unlock_all): Remove __sfp_lock_release call.
>     * libc/stdio/fopen.c (_fopen_r): Protect file pointer list.
>     Add calls to _flockfile/_funlockfile. Remove
>     __lock_init_recursive call.
>     * libc/stdio/freopen.c (_freopen_r): Protect file pointer list.
>     * libc/stdio/fwalk.c (__fwalk): New static function.
>     (_fwalk): Protect file pointer list. Use __fwalk to walk through
>     file pointers.
>     * libc/stdio/local.h: Add defines for
>     __sfp_lock_acquire/__sfp_lock_release when
>     single threaded. Add function prototypes otherwise.
>     * libc/stdio64/fdopen64.c (_fdopen64_r): Add calls to
>     _flockfile/_funlockfile.
>     * libc/stdio/fopen64.c (_fopen64_r): Protect file pointer list.
>     Add calls to _flockfile/_funlockfile. Remove
>      __lock_init_recursive call.
>     * libc/stdio/freopen64.c (_freopen64_r): Protect file pointer
>     list.