[v2] libio: Fix race in _IO_new_file_init_internal initialization order [BZ #33785]
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
Commit Message
_IO_new_file_init_internal linked the new stream into _IO_list_all
before setting fp->_fileno to -1. A concurrent thread that walks
_IO_list_all (for example via fflush (NULL)) could observe the stream
with an uninitialized _fileno before initialization completed.
Set _fileno = -1 before _IO_link_in so the stream is fully
initialized when it becomes visible in the global list.
This is the residual concurrency defect noted at the end of commit
b657f72fa3 ("libio: Fix deadlock between freopen, fflush (NULL) and
fclose (bug 24963)").
Add libio/tst-file-init-race exercising concurrent fopen/fclose and
fflush (NULL) to detect regressions.
Signed-off-by: Shamil Abdulaev <ashamil435@gmail.com>
---
Changes since v1:
- Apply the same _fileno/_IO_link_in reordering to libio/oldfileops.c
(suggested by Florian Weimer).
- tst-file-init-race: treat fopen/fclose failures as test failures via
FAIL_EXIT1 instead of silently ignoring them (suggested by Florian
Weimer).
libio/Makefile | 3 ++
libio/fileops.c | 2 +-
libio/oldfileops.c | 2 +-
libio/tst-file-init-race.c | 69 ++++++++++++++++++++++++++++++++++++++
4 files changed, 74 insertions(+), 2 deletions(-)
create mode 100644 libio/tst-file-init-race.c
Comments
* Shamil Abdulaev:
> _IO_new_file_init_internal linked the new stream into _IO_list_all
> before setting fp->_fileno to -1. A concurrent thread that walks
> _IO_list_all (for example via fflush (NULL)) could observe the stream
> with an uninitialized _fileno before initialization completed.
>
> Set _fileno = -1 before _IO_link_in so the stream is fully
> initialized when it becomes visible in the global list.
>
> This is the residual concurrency defect noted at the end of commit
> b657f72fa3 ("libio: Fix deadlock between freopen, fflush (NULL) and
> fclose (bug 24963)").
>
> Add libio/tst-file-init-race exercising concurrent fopen/fclose and
> fflush (NULL) to detect regressions.
>
> Signed-off-by: Shamil Abdulaev <ashamil435@gmail.com>
> ---
> Changes since v1:
> - Apply the same _fileno/_IO_link_in reordering to libio/oldfileops.c
> (suggested by Florian Weimer).
> - tst-file-init-race: treat fopen/fclose failures as test failures via
> FAIL_EXIT1 instead of silently ignoring them (suggested by Florian
> Weimer).
This version looks okay to me.
Reviewed-by: Florian Weimer <fweimer@redhat.com>
I'll do a final round of testing and will push it for you afterwards.
Thanks,
Florian
* Florian Weimer:
> * Shamil Abdulaev:
>
>> _IO_new_file_init_internal linked the new stream into _IO_list_all
>> before setting fp->_fileno to -1. A concurrent thread that walks
>> _IO_list_all (for example via fflush (NULL)) could observe the stream
>> with an uninitialized _fileno before initialization completed.
>>
>> Set _fileno = -1 before _IO_link_in so the stream is fully
>> initialized when it becomes visible in the global list.
>>
>> This is the residual concurrency defect noted at the end of commit
>> b657f72fa3 ("libio: Fix deadlock between freopen, fflush (NULL) and
>> fclose (bug 24963)").
>>
>> Add libio/tst-file-init-race exercising concurrent fopen/fclose and
>> fflush (NULL) to detect regressions.
>>
>> Signed-off-by: Shamil Abdulaev <ashamil435@gmail.com>
>> ---
>> Changes since v1:
>> - Apply the same _fileno/_IO_link_in reordering to libio/oldfileops.c
>> (suggested by Florian Weimer).
>> - tst-file-init-race: treat fopen/fclose failures as test failures via
>> FAIL_EXIT1 instead of silently ignoring them (suggested by Florian
>> Weimer).
>
> This version looks okay to me.
>
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
>
> I'll do a final round of testing and will push it for you afterwards.
Sorry, something else has come up. You submitted this under DCO:
> Signed-off-by: Shamil Abdulaev <ashamil435@gmail.com>
And yet the new file you added says:
> + Copyright (C) 2026 Free Software Foundation, Inc.
For DCO submissions, it should be
+ Copyright The GNU Toolchain Authors.
Would you please submit a v3 with this fixed? I don't feel quite
comfortable editing copyright notices before pushing, but I suppose I
could do it for you if explicitly told. 8-)
Thanks,
Florian
@@ -107,6 +107,7 @@ tests = \
tst-fgetc-after-eof \
tst-fgetwc \
tst-fgetws \
+ tst-file-init-race \
tst-fopenloc2 \
tst-fputws \
tst-freopen \
@@ -160,6 +161,8 @@ tests-static += \
$(objpfx)tst-popen-fork: $(shared-thread-library)
+$(objpfx)tst-file-init-race: $(shared-thread-library)
+
tests-internal = tst-vtables tst-vtables-interposed
ifeq (yes,$(build-shared))
@@ -111,8 +111,8 @@ _IO_new_file_init_internal (struct _IO_FILE_plus *fp)
fp->file._offset = _IO_pos_BAD;
fp->file._flags |= CLOSED_FILEBUF_FLAGS;
- _IO_link_in (fp);
fp->file._fileno = -1;
+ _IO_link_in (fp);
}
/* External version of _IO_new_file_init_internal which switches off
@@ -108,8 +108,8 @@ _IO_old_file_init_internal (struct _IO_FILE_plus *fp)
_IO_vtable_offset is used to detect the old binaries. */
fp->file._vtable_offset = ((int) sizeof (struct _IO_FILE)
- (int) sizeof (struct _IO_FILE_complete));
- _IO_link_in (fp);
fp->file._fileno = -1;
+ _IO_link_in (fp);
if (&_IO_stdin_used != NULL || !_IO_legacy_file ((FILE *) fp))
/* The object is dynamically allocated and large enough. Initialize
new file mode 100644
@@ -0,0 +1,69 @@
+/* Test for race during FILE initialization in _IO_new_file_init_internal.
+ 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 <stdio.h>
+#include <stdatomic.h>
+#include <pthread.h>
+#include <time.h>
+
+#include <support/check.h>
+#include <support/xthread.h>
+
+static atomic_bool stop = ATOMIC_VAR_INIT (0);
+
+static void *
+opener_thread (__attribute__ ((unused)) void *arg)
+{
+ while (!atomic_load_explicit (&stop, memory_order_acquire))
+ {
+ FILE *fp = fopen ("/dev/null", "r");
+ if (fp == NULL)
+ FAIL_EXIT1 ("fopen: %m");
+ if (fclose (fp) != 0)
+ FAIL_EXIT1 ("fclose: %m");
+ }
+ return NULL;
+}
+
+static void *
+flusher_thread (__attribute__ ((unused)) void *arg)
+{
+ while (!atomic_load_explicit (&stop, memory_order_acquire))
+ fflush (NULL);
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ pthread_t t1 = xpthread_create (NULL, opener_thread, NULL);
+ pthread_t t2 = xpthread_create (NULL, flusher_thread, NULL);
+
+ struct timespec ts = { .tv_sec = 3, .tv_nsec = 0 };
+ nanosleep (&ts, NULL);
+
+ atomic_store_explicit (&stop, 1, memory_order_release);
+
+ xpthread_join (t1);
+ xpthread_join (t2);
+
+ return 0;
+}
+
+#define TIMEOUT 30
+#include <support/test-driver.c>