[PATCH] libio: Fix deadlock between freopen, fflush (NULL) and fclose (bug 24963)
Arjun Shankar
arjun@redhat.com
Wed Feb 11 23:07:18 GMT 2026
Hi Florian,
> 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.
I have some questions and comments below but the overall direction
looks good to me.
>
> ---
> libio/fileops.c | 24 +++++++++--
> libio/freopen.c | 19 +++++++--
> libio/freopen64.c | 19 +++++++--
> libio/genops.c | 33 ++++++++-------
> libio/iofclose.c | 20 ++++++---
> libio/libio.h | 5 +++
> libio/libioP.h | 1 +
> sysdeps/pthread/Makefile | 1 +
> sysdeps/pthread/tst-bug24963.c | 94 ++++++++++++++++++++++++++++++++++++++++++
> 9 files changed, 187 insertions(+), 29 deletions(-)
>
> diff --git a/libio/fileops.c b/libio/fileops.c
> index 1852c9ea12..11b1d2b57a 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)
OK. This "new" function allows closing with an optional unlink. The
original close with unconditional unlink is now declared right below,
wrapping this function.
> {
> 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);
OK. The unlink is optional.
OK. Regarding the preservation of _IO_LINKED: if this function was
called with unlink=true, then by this point _IO_LINKED has already
been reset and will remain reset. If this function was called with
unlink=false, then whether or not _IO_LINKED is true, it still
correctly holds the status of the file at this point and therefore
should be preserved.
> 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)
OK. This is the old _IO_new_file_close_it behaviour, where the unlink
was unconditional.
>
> 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 link in the stream. It is already on
> + 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);
> +
OK. Below in the freopen changes we set NOCLOSE. So we are inside a
reopen here, and the file is already in the list and should not be
linked in again.
> return fp;
> }
> libc_hidden_def (_IO_file_open)
> diff --git a/libio/freopen.c b/libio/freopen.c
> index c1ea706f80..d032516c01 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);
OK. Earlier, during freopen, we would close (which unconditionally
unlinks), then open.
The earlier lock order was:
1. lock file.
2. close file, which would unconditionally unlink: lock list,
recursively lock file, recursively unlock file, unlock list
3. open file, which calls link_in: lock list, recursively lock file,
recursively unlock file, unlock list.
4. unlock file
This was out of the canonical order seen across genops.c.
Now, we close without unlinking so that the file stays in the list.
Then we open (that I just reviewed changes to above), which doesn't
need to put it back into the list.
The new lock/operation order is:
1. lock file
2. close file without unlink, so no additional locking
3. open file, which does not call link_in due to NOCLOSE being set during reopen
4. unlock file
OK. So the list lock is never taken.
> _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,18 @@ freopen (const char *filename, const char *mode, FILE *fp)
> __close (fd);
>
> end:
> + if (result == NULL)
> + /* Skip future flushing. */
> + fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
> +
OK. So this protects against flushing a stream in an inconsistent
state during the time between unlocking and unlinking.
Might be useful to update the comment to point out how
_IO_FLAGS2_NOCLOSE (now) interacts with flush_all.
> _IO_release_lock (fp);
> - if (result == NULL && (fp->_flags & _IO_IS_FILEBUF) != 0)
> - _IO_deallocate_file (fp);
OK. Earlier we would have already unlinked the file. So if we failed
to reopen it, all we needed to do was deallocate it.
> +
> + /* 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);
> + }
OK. Now, if we fail to reopen it, since we avoided unlinking it
earlier in the function, we need to unlink it now and then deallocate
it.
> return result;
> }
> diff --git a/libio/freopen64.c b/libio/freopen64.c
> index 2848af38db..ac6d001716 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);
OK. Similar to freopen.
> _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,18 @@ freopen64 (const char *filename, const char *mode, FILE *fp)
> __close (fd);
>
> end:
> + if (result == NULL)
> + /* Skip future flushing. */
> + fp->_flags2 |= _IO_FLAGS2_NOCLOSE;
> +
OK. Similar to freopen.
> _IO_release_lock (fp);
> - if (result == NULL && (fp->_flags & _IO_IS_FILEBUF) != 0)
> - _IO_deallocate_file (fp);
OK. Similar to freopen.
> +
> + /* 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);
> + }
OK. Similar to freopen.
The two files are quite similar minus some SHLIB_COMPAT differences.
Would it be useful to merge them eventually? Not for now. I'm just
wondering.
> return result;
> }
> diff --git a/libio/genops.c b/libio/genops.c
OK. All list locking and unlocking happens via functions in this file.
_IO_un_link, _IO_link_in, _IO_flush_all, _IO_flush_all_linebuffered,
and _IO_unbuffer_all - all of these follow the same sequence: lock
list -> lock fp -> unlock fp -> unlock list.
So the lock acquisition order is correct for all of them.
> index 439ba88e35..036875758e 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;
So this bit flushes "fp". We take it and...
> + /* 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)
> + {
...we make it conditional to _IO_FLAGS2_NOCLOSE being unset.
> + 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;
OK. This is the flush, now conditional. This is because flush_all
might be called while freopen is running in a separate thread. There
is a section of code in freopen where if the freopen fails to re"open"
after its close, it gives up the lock on the file and then unlinks it.
During that period, a flush_all should be stopped from flushing the
file handle that is still in the list but in an inconsistent state.
> + }
>
> _IO_funlockfile (fp);
> run_fp = NULL;
Question: what about _IO_flush_all_linebuffered, which apparently is
exported by glibc as _flushlbf?
> diff --git a/libio/iofclose.c b/libio/iofclose.c
> index a945dff396..a7d52846e3 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);
> -
OK. Don't unlink for now.
> _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;
> + }
OK. Still don't unlink when closing.
> 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 callhere synchronizes with fflush (NULL). Future
> + interaction with fflush (NULL) is not possible because the stream
> + is no longer on the list. */
> + _IO_un_link ((struct _IO_FILE_plus *) fp);
> +
OK. Unlink at the end.
I'm going to trace the lock acquisition/release sequences for this function:
The earlier operation and lock order was:
1. unlink, i.e.: lock list -> lock file -> unlock file -> unlock list
2. lock file
3. close file, which unlinks unconditionally, but: the unlink is a
no-op and takes no lock because of the unset _IO_LINKED flag
4. unlock file
Now, the order of operations including lock order is:
1. lock file
2. close file without unlink
3. release file lock
4. unlink: lock list, lock file, unlock file, unlock list
Actually, both orders seem fine to me.
The setting of NOCLOSE, which is an additional change, is only made
necessary by the new delayed unlink. The early unlink would ensure
that a concurrent flush_all never sees this file mid-closure.
I believe the fix still works if we don't change this function. Just
to test the idea out, I reverted the change to this function and
re-ran the test with 1+5+5 threads (instead of 1+2+2) for 300 seconds
(and with a longer timeout) and it ran to completion.
> _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. */
OK. If my conjecture about not needing to change the operation
ordering during file close is correct, this comment might need to be
accordingly adjusted.
> #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;
OK.
> 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 \
OK. New test.
> 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..3afec8e05d
> --- /dev/null
> +++ b/sysdeps/pthread/tst-bug24963.c
> @@ -0,0 +1,94 @@
> +/* 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 <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 *
> +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);
OK. One thread will repeatedly flush all files.
> +
> + pthread_t fopens[2];
> + for (int i = 0; i < array_length(fopens); ++i)
> + fopens[i] = xpthread_create (NULL, fopen_thread, NULL);
OK. Two threads will repeatedly open and then close.
> +
> + 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]);
> + }
OK. Two threads will repeatedly reopen.
> +
> + usleep (2 * 1000 * 1000);
> + running = false;
OK. Do this for two seconds.
> +
> + 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 (fflush_thr);
OK. Cleanup.
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
>
> base-commit: 78fdb2d6b1c34ea8e779fd48f9436dfbd50b6387
>
--
Arjun Shankar
he/him/his
More information about the Libc-alpha
mailing list