diff mbox series

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

Message ID f6757145-6d1a-e1e5-6b2b-9f7b7e65e26b@alibaba-inc.com
State New
Headers show
Series [RFC] libio: Fix deadlock between freopen and fclose [BZ #24963] | expand

Commit Message

=?UTF-8?B?6LCi5a6c55SfKOavheaZnyk=?= Sept. 21, 2019, 3:21 a.m. UTC
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

Comments

Szabolcs Nagy Sept. 23, 2019, 10:05 a.m. UTC | #1
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
>
Szabolcs Nagy Sept. 23, 2019, 10:16 a.m. UTC | #2
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.
diff mbox series

Patch

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