[PATCH v2] libio: Fix deadlock between freopen, fflush (NULL) and fclose (bug 24963)
Arjun Shankar
arjun@redhat.com
Mon Feb 16 17:03:07 GMT 2026
> The canonical lock ordering for stream list processing is
> locking list_all_lock first, then individual streams as needed.
> The fclose implementation reversed that, and the freopen
> implementation performed list operations under the reverse order,
> too.
>
> Unlinking in fclose was already unconditional, and the early unlinking
> looks unnecessary: _IO_file_close_it would call it even for
> !_IO_IS_FILEBUF streams.
>
> There is still a remaining concurrency defect because
> _IO_new_file_init_internal links in the stream before it is
> fully initialized, and it is not locked at this point.
This looks good to me. Review below.
Reviewed-by: Arjun Shankar <arjun@redhat.com>
>
> ---
> v2: Fix typo identified by Adhemerval. Update freopen/freopen64
> comment. Add NOCLOSE flag guard to _flushlbf. Add _flushlbf
> thread to test. The updated test still reproduces the deadlock.
v1 was here:
https://inbox.sourceware.org/libc-alpha/lhuqzruiwt2.fsf@oldenburg.str.redhat.com/
* The typo Adhemerval pointed out is fixed.
* My v1 comments are addressed:
1. Update freopen/64 comments - Done
2. Address _flushlbf - Done
3. Whether iofclose needs to be changed - you responded to this in
the v1 thread.
>
> libio/fileops.c | 24 ++++++++--
> libio/freopen.c | 20 ++++++--
> libio/freopen64.c | 20 ++++++--
> libio/genops.c | 39 +++++++++------
> libio/iofclose.c | 20 ++++++--
> libio/libio.h | 5 ++
> libio/libioP.h | 1 +
> sysdeps/pthread/Makefile | 1 +
> sysdeps/pthread/tst-bug24963.c | 106 +++++++++++++++++++++++++++++++++++++++++
> 9 files changed, 206 insertions(+), 30 deletions(-)
>
> diff --git a/libio/fileops.c b/libio/fileops.c
> index 1852c9ea12..8067c0a9cf 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -124,8 +124,10 @@ _IO_new_file_init (struct _IO_FILE_plus *fp)
> _IO_new_file_init_internal (fp);
> }
>
> +/* Close FP, but do not deallocate it. If UNLINK, call _IO_un_link to
> + remove the file from the global list of files if necessary. */
> int
> -_IO_new_file_close_it (FILE *fp)
> +_IO_file_close_maybe_unlink (FILE *fp, bool unlink)
> {
> int flush_status = 0;
> if (!_IO_file_is_open (fp))
> @@ -188,13 +190,22 @@ _IO_new_file_close_it (FILE *fp)
> _IO_setg (fp, NULL, NULL, NULL);
> _IO_setp (fp, NULL, NULL);
>
> - _IO_un_link ((struct _IO_FILE_plus *) fp);
> - fp->_flags = _IO_MAGIC|CLOSED_FILEBUF_FLAGS;
> + if (unlink)
> + _IO_un_link ((struct _IO_FILE_plus *) fp);
> + /* Preserve the _IO_LINKED flag, so that _IO_un_link called from
> + fclose still unlinks the stream. */
> + fp->_flags = _IO_MAGIC | CLOSED_FILEBUF_FLAGS | (fp->_flags & _IO_LINKED);
> fp->_fileno = -1;
> fp->_offset = _IO_pos_BAD;
>
> return close_status ? close_status : flush_status;
> }
> +
> +int
> +_IO_new_file_close_it (FILE *fp)
> +{
> + return _IO_file_close_maybe_unlink (fp, true);
> +}
> libc_hidden_ver (_IO_new_file_close_it, _IO_file_close_it)
>
> void
> @@ -236,7 +247,12 @@ _IO_file_open (FILE *fp, const char *filename, int posix_mode, int prot,
> return NULL;
> }
> }
> - _IO_link_in ((struct _IO_FILE_plus *) fp);
> +
> + /* During reopen, do not try to link in the stream. It is already on
OK. Fixed up comment.
> + the list. This avoids deadlocks due to lock ordering issues. */
> + if ((fp->_flags2 & _IO_FLAGS2_NOCLOSE) == 0)
> + _IO_link_in ((struct _IO_FILE_plus *) fp);
> +
> return fp;
> }
> libc_hidden_def (_IO_file_open)
> diff --git a/libio/freopen.c b/libio/freopen.c
> index c1ea706f80..c3047facd4 100644
> --- a/libio/freopen.c
> +++ b/libio/freopen.c
> @@ -72,7 +72,10 @@ freopen (const char *filename, const char *mode, FILE *fp)
> else
> #endif
> {
> - _IO_file_close_it (fp);
> + /* Do not unlink the stream because it has to stay on the list.
> + Flushing through fflush (NULL) is prevented because the
> + stream is still locked. */
> + _IO_file_close_maybe_unlink (fp, false);
> _IO_JUMPS_FILE_plus (fp) = &_IO_file_jumps;
> if (_IO_vtable_offset (fp) == 0 && fp->_wide_data != NULL)
> fp->_wide_data->_wide_vtable = &_IO_wfile_jumps;
> @@ -112,8 +115,19 @@ freopen (const char *filename, const char *mode, FILE *fp)
> __close (fd);
>
> end:
> + if (result == NULL)
> + /* After the unlock below, _IO_flush_all could run and try to
> + flush the partially closed stream. Setting the flag prevents that. */
OK. Clearer comment.
> + fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
> +
> _IO_release_lock (fp);
> - if (result == NULL && (fp->_flags & _IO_IS_FILEBUF) != 0)
> - _IO_deallocate_file (fp);
> +
> + /* See fclose for the concurrency impact. */
> + if (result == NULL)
> + {
> + _IO_un_link ((struct _IO_FILE_plus *) fp);
> + if (fp->_flags & _IO_IS_FILEBUF)
> + _IO_deallocate_file (fp);
> + }
> return result;
> }
> diff --git a/libio/freopen64.c b/libio/freopen64.c
> index 2848af38db..c499a8375c 100644
> --- a/libio/freopen64.c
> +++ b/libio/freopen64.c
> @@ -52,7 +52,10 @@ freopen64 (const char *filename, const char *mode, FILE *fp)
> = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename);
>
> fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
> - _IO_file_close_it (fp);
> + /* Do not unlink the stream because it has to stay on the list.
> + Flushing through fflush (NULL) is prevented because the
> + stream is still locked. */
> + _IO_file_close_maybe_unlink (fp, false);
> _IO_JUMPS_FILE_plus (fp) = &_IO_file_jumps;
> if (_IO_vtable_offset (fp) == 0 && fp->_wide_data != NULL)
> fp->_wide_data->_wide_vtable = &_IO_wfile_jumps;
> @@ -91,8 +94,19 @@ freopen64 (const char *filename, const char *mode, FILE *fp)
> __close (fd);
>
> end:
> + if (result == NULL)
> + /* After the unlock below, _IO_flush_all could run and try to
> + flush the partially closed stream. Setting the flag prevents that. */
OK. Clearer comment.
> + fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
> +
> _IO_release_lock (fp);
> - if (result == NULL && (fp->_flags & _IO_IS_FILEBUF) != 0)
> - _IO_deallocate_file (fp);
> +
> + /* See fclose for the concurrency impact. */
> + if (result == NULL)
> + {
> + _IO_un_link ((struct _IO_FILE_plus *) fp);
> + if (fp->_flags & _IO_IS_FILEBUF)
> + _IO_deallocate_file (fp);
> + }
> return result;
> }
> diff --git a/libio/genops.c b/libio/genops.c
> index 439ba88e35..cc1684e00a 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -724,20 +724,25 @@ _IO_flush_all (void)
> run_fp = fp;
> _IO_flockfile (fp);
>
> - if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
> - || (_IO_vtable_offset (fp) == 0
> - && fp->_mode > 0 && (fp->_wide_data->_IO_write_ptr
> - > fp->_wide_data->_IO_write_base))
> - )
> - && _IO_OVERFLOW (fp, EOF) == EOF)
> - result = EOF;
> - if (_IO_fileno (fp) >= 0
> - && ((fp->_mode <= 0 && fp->_IO_read_ptr < fp->_IO_read_end)
> - || (_IO_vtable_offset (fp) == 0
> - && fp->_mode > 0 && (fp->_wide_data->_IO_read_ptr
> - < fp->_wide_data->_IO_read_end)))
> - && _IO_SYNC (fp) != 0)
> - result = EOF;
> + /* If fp is in an freopen operation or about to be closed, do
> + not flush it again. Flushing is handled by these operations. */
> + if ((fp->_flags2 & _IO_FLAGS2_NOCLOSE) == 0)
> + {
> + if (((fp->_mode <= 0 && fp->_IO_write_ptr > fp->_IO_write_base)
> + || (_IO_vtable_offset (fp) == 0
> + && fp->_mode > 0 && (fp->_wide_data->_IO_write_ptr
> + > fp->_wide_data->_IO_write_base))
> + )
> + && _IO_OVERFLOW (fp, EOF) == EOF)
> + result = EOF;
> + if (_IO_fileno (fp) >= 0
> + && ((fp->_mode <= 0 && fp->_IO_read_ptr < fp->_IO_read_end)
> + || (_IO_vtable_offset (fp) == 0
> + && fp->_mode > 0 && (fp->_wide_data->_IO_read_ptr
> + < fp->_wide_data->_IO_read_end)))
> + && _IO_SYNC (fp) != 0)
> + result = EOF;
> + }
>
> _IO_funlockfile (fp);
> run_fp = NULL;
> @@ -767,7 +772,11 @@ _IO_flush_all_linebuffered (void)
> run_fp = fp;
> _IO_flockfile (fp);
>
> - if ((fp->_flags & _IO_NO_WRITES) == 0 && fp->_flags & _IO_LINE_BUF)
> + /* Regarding _IO_FLAGS2_NOCLOSE: If fp is in an freopen
> + operation or about to be closed, do not flush it again.
> + Flushing is handled by these operations. */
> + if ((fp->_flags & _IO_NO_WRITES) == 0 && fp->_flags & _IO_LINE_BUF
> + && (fp->_flags2 & _IO_FLAGS2_NOCLOSE) == 0)
OK. Additionally skip flushing files marked NOCLOSE during _flushlbf,
just like we do with the flush_all.
> _IO_OVERFLOW (fp, EOF);
>
> _IO_funlockfile (fp);
> diff --git a/libio/iofclose.c b/libio/iofclose.c
> index a945dff396..89782e99d7 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);
> -
> _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;
> + }
> 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 call here synchronizes with fflush (NULL). Future
OK. Typo fix.
> + interaction with fflush (NULL) is not possible because the stream
> + is no longer on the list. */
> + _IO_un_link ((struct _IO_FILE_plus *) fp);
> +
> _IO_FINISH (fp);
> if (fp->_mode > 0)
> {
> diff --git a/libio/libio.h b/libio/libio.h
> index 3e3773484e..6cbaf29f19 100644
> --- a/libio/libio.h
> +++ b/libio/libio.h
> @@ -86,7 +86,12 @@ typedef struct
> #define _IO_FLAGS2_MMAP 1
> #define _IO_FLAGS2_NOTCANCEL 2
> #define _IO_FLAGS2_USER_WBUF 8
> +
> +/* The file is in a freopen operation, or it is about to be closed.
> + Closing it does not deallocate its underlying file descriptor, and
> + fflush (NULL) will not flush this file. */
> #define _IO_FLAGS2_NOCLOSE 32
> +
> #define _IO_FLAGS2_CLOEXEC 64
> #define _IO_FLAGS2_NEED_LOCK 128
>
> diff --git a/libio/libioP.h b/libio/libioP.h
> index d3b9e5ea5f..1485d22619 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -639,6 +639,7 @@ libc_hidden_proto (_IO_file_finish)
>
> extern FILE* _IO_new_file_attach (FILE *, int);
> extern int _IO_new_file_close_it (FILE *);
> +int _IO_file_close_maybe_unlink (FILE *, bool) attribute_hidden;
> extern void _IO_new_file_finish (FILE *, int);
> extern FILE* _IO_new_file_fopen (FILE *, const char *, const char *,
> int);
> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index 81ea70897f..4d43386658 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -71,6 +71,7 @@ tests += \
> tst-basic5 \
> tst-basic6 \
> tst-basic7 \
> + tst-bug24963 \
> tst-call-once \
> tst-cancel-self \
> tst-cancel-self-cancelstate \
> diff --git a/sysdeps/pthread/tst-bug24963.c b/sysdeps/pthread/tst-bug24963.c
> new file mode 100644
> index 0000000000..7777967baf
> --- /dev/null
> +++ b/sysdeps/pthread/tst-bug24963.c
> @@ -0,0 +1,106 @@
> +/* Test lock ordering of fflush (NULL) vs freopen, fclose (bug 24963).
> + Copyright (C) 2026 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <array_length.h>
> +#include <stdint.h>
> +#include <stdio_ext.h>
OK. Needed for _flushlbf.
> +#include <support/check.h>
> +#include <support/xstdio.h>
> +#include <support/xthread.h>
> +#include <unistd.h>
> +
> +static _Atomic bool running = true;
> +
> +static void *
> +fflush_thread (void *ignored)
> +{
> + while (running)
> + TEST_COMPARE (fflush (NULL), 0);
> +
> + return NULL;
> +}
> +
> +static void *
> +flushlbf_thread (void *ignored)
> +{
> + while (running)
> + _flushlbf ();
> +
> + return NULL;
> +}
OK. Test for _flushlbf.
> +
> +static void *
> +fopen_thread (void *ignored)
> +{
> + while (running)
> + {
> + FILE *fp = xfopen ("/etc/passwd", "r");
> + (void) fgetc (fp);
> + xfclose (fp);
> + }
> + return NULL;
> +}
> +
> +static void *
> +freopen_thread (void *fp)
> +{
> + while (running)
> + {
> + uintptr_t old_address = (uintptr_t) fp;
> + FILE *fpnew = xfreopen ("/etc/passwd", "r", fp);
> + TEST_COMPARE (old_address, (uintptr_t) fpnew);
> + }
> + return NULL;
> +}
> +
> +static int
> +do_test (void)
> +{
> + pthread_t fflush_thr = xpthread_create (NULL, fflush_thread, NULL);
> + pthread_t flushlbf_thr = xpthread_create (NULL, flushlbf_thread, NULL);
OK. Again, _flushlbf.
> +
> + pthread_t fopens[2];
> + for (int i = 0; i < array_length(fopens); ++i)
> + fopens[i] = xpthread_create (NULL, fopen_thread, NULL);
> +
> + FILE *fps[2];
> + pthread_t freopens[array_length (fps)];
> + for (int i = 0; i < array_length(fps); ++i)
> + {
> + fps[i] = xfopen ("/etc/passwd", "r");
> + freopens[i] = xpthread_create (NULL, freopen_thread, fps[i]);
> + }
> +
> + usleep (2 * 1000 * 1000);
> + running = false;
> +
> + for (int i = 0; i < array_length(fopens); ++i)
> + xpthread_join (fopens[i]);
> + for (int i = 0; i < array_length(fps); ++i)
> + {
> + xpthread_join (freopens[i]);
> + xfclose (fps[i]);
> + }
> +
> + xpthread_join (flushlbf_thr);
OK. Same.
> + xpthread_join (fflush_thr);
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
>
> base-commit: 419245719ccbc7dad6a97f24465e7f09c090327a
>
--
Arjun Shankar
he/him/his
More information about the Libc-alpha
mailing list