From patchwork Mon Jan 12 17:07:53 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 127914 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from vm01.sourceware.org (localhost [127.0.0.1]) by sourceware.org (Postfix) with ESMTP id 270AE4BA2E25 for ; Mon, 12 Jan 2026 17:08:33 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 270AE4BA2E25 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=VlIvaDPH X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id C274D4BA540B for ; Mon, 12 Jan 2026 17:07:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C274D4BA540B Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org C274D4BA540B Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1768237678; cv=none; b=lKBVfPGd/nmXPzCOviAIbvR2KVVq/4Fe55V5Kh/Un7yTmD37SPMLm27F8PVKxtwONRne1YTKgvbFig4qucpCeuSaub07x8r2rYb2xxoG1ESLrfXaiRR8rT53i3mCLoKmW64rJyz+ZUTqrbA8t3a2c37WaKtSHwWmRS8u8yCp58o= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1768237678; c=relaxed/simple; bh=QJM8Y7M/siFIwTrTnR/6KsyjD6HG+8xDr6yLkISulY4=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=ZbcMo2Y02CuGa0skGourEsmghURFK/Er+OdMxiZ3lz62QtfyKCE7GS4nb5n1dKV7frIZK2xndlVvvatvR0xg6vKZE5FFIjR33FyPlJhZKuX2pEBcKYv9NPqMqM+j7giADUqJkONqH0B4OuYsacIJzqQd0CIU/oJHSjyKJP8njnU= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C274D4BA540B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1768237678; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type; bh=rKFHj4Qx1iubhOhEtwAWbzLTqUiOHhh2OtPQrq+VQLU=; b=VlIvaDPH53l89vQhgDbcrsXSN9hrPjDi1SVU7Kld6J/UDksmHhqNse3PUTJcueX4NxNbQQ EkH2xe3jGobPz+dFzzmh7gEBhv4n6RRZwaOifRKDIa8AzRZ8IcgtM0jt5wMpUc2QLFJNFu eq+FcEW9Q720Ti0X11MAdCSdfFKTsdk= Received: from mx-prod-mc-08.mail-002.prod.us-west-2.aws.redhat.com (ec2-35-165-154-97.us-west-2.compute.amazonaws.com [35.165.154.97]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-671-zstRNAcnNBejmI3Xxc-qqw-1; Mon, 12 Jan 2026 12:07:57 -0500 X-MC-Unique: zstRNAcnNBejmI3Xxc-qqw-1 X-Mimecast-MFC-AGG-ID: zstRNAcnNBejmI3Xxc-qqw_1768237676 Received: from mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-08.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 62FF418002C7 for ; Mon, 12 Jan 2026 17:07:56 +0000 (UTC) Received: from fweimer-oldenburg.csb.redhat.com (unknown [10.44.32.58]) by mx-prod-int-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 9D6C130001A2 for ; Mon, 12 Jan 2026 17:07:55 +0000 (UTC) From: Florian Weimer To: libc-alpha@sourceware.org Subject: [PATCH] libio: Fix deadlock between freopen, fflush (NULL) and fclose (bug 24963) Date: Mon, 12 Jan 2026 18:07:53 +0100 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.4 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: metf0FMq22-iM34uVMrmCoSR5rAOByktqqi7wNVjSoc_1768237676 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, RCVD_IN_VALIDITY_RPBL_BLOCKED, RCVD_IN_VALIDITY_SAFE_BLOCKED, SPF_HELO_PASS, SPF_NONE, TXREP, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libc-alpha-bounces~patchwork=sourceware.org@sourceware.org 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: Adhemerval Zanella --- 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(-) base-commit: 78fdb2d6b1c34ea8e779fd48f9436dfbd50b6387 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) { 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 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..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); _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; + _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..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); _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; + _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..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; + /* 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; 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); - _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 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); + _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..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 + . */ + +#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 * +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 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 (fflush_thr); + + return 0; +} + +#include