[RFC] libio: Fix deadlock between freopen and fclose [BZ #24963]

  we find a deadlock between fclose and freopen as following:
CPU0                                          CPU1
freopen                                       fclose
  ->_IO_acquire_lock (fp)                         ->_IO_un_link
  ->_IO_file_close_it                              ->_IO_lock_lock(list_all_lock)
    ->_IO_un_link
      ->_IO_lock_lock (list_all_lock)<-wait here
                                                   ->_IO_flockfile((_IO_FILE *) fp); <-- wait here

As Carlos pointed that this maybe the bug of in _IO_new_fclose, which
can be fixed by locking fp first, then with fp acquired lock the whole
list.

Signed-off-by: Yisheng Xie <yisheng.xys@alibaba-inc.com>
---
 libio/iofclose.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.23.0
  

On 21/09/2019 04:21, 谢宜生(毅晟) wrote:
> we find a deadlock between fclose and freopen as following:

> CPU0                                          CPU1

> freopen                                       fclose

>   ->_IO_acquire_lock (fp)                         ->_IO_un_link

>   ->_IO_file_close_it                              ->_IO_lock_lock(list_all_lock)

>     ->_IO_un_link

>       ->_IO_lock_lock (list_all_lock)<-wait here

>                                                    ->_IO_flockfile((_IO_FILE *) fp); <-- wait here

> 

> As Carlos pointed that this maybe the bug of in _IO_new_fclose, which

> can be fixed by locking fp first, then with fp acquired lock the whole

> list.


if we want to keep freopen as is then an fp cannot
be locked after list_all_lock is held, if another
thread might freopen(fp).

this affects more than just fclose, e.g.
fflush(NULL) would have the same lock ordering
(first locks list_all_lock, then locks each fp)

this tells me that a different fix is needed,
either way there should be a bug report about
it in bugzilla (and the id referenced in the
changelog of a patch submission assuming you
have the fsf copyright assignment sorted out).

> 

> Signed-off-by: Yisheng Xie <yisheng.xys@alibaba-inc.com>

> ---

>  libio/iofclose.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/libio/iofclose.c b/libio/iofclose.c

> index 398b86d597..fe262cf6aa 100644

> --- a/libio/iofclose.c

> +++ b/libio/iofclose.c

> @@ -44,11 +44,11 @@ _IO_new_fclose (FILE *fp)

>      return _IO_old_fclose (fp);

>  #endif

> 

> +  _IO_acquire_lock (fp);

>    /* First unlink the stream.  */

>    if (fp->_flags & _IO_IS_FILEBUF)

>      _IO_un_link ((struct _IO_FILE_plus *) fp);

> 

> -  _IO_acquire_lock (fp);

>    if (fp->_flags & _IO_IS_FILEBUF)

>      status = _IO_file_close_it (fp);

>    else

> --

> 2.23.0

>
  

On 23/09/2019 11:05, Szabolcs Nagy wrote:
> either way there should be a bug report about

> it in bugzilla (and the id referenced in the


sorry, missed the bugzilla id in the subject,
i commented on the bug now.