From b657f72fa34192b9afdd4cfab7fcf8e039d8888c Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Mon, 23 Feb 2026 14:19:38 +0100 Subject: [PATCH] libio: Fix deadlock between freopen, fflush (NULL) and fclose (bug 24963) 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. Reviewed-by: Arjun Shankar --- 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(-) create mode 100644 sysdeps/pthread/tst-bug24963.c 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 + 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. */ + 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. */ + 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) _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 + 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 + . */ + +#include +#include +#include +#include +#include +#include +#include + +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; +} + +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); + + 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); + xpthread_join (fflush_thr); + + return 0; +} + +#include -- 2.47.3