From patchwork Tue Aug 8 12:53:25 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 73814 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5E6D7385781F for ; Tue, 8 Aug 2023 12:53:51 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5E6D7385781F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1691499231; bh=p2C+ulQRSH3Z1nLk+WQGbf6jVwhlBtwNiVWmtvmL7c8=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=tssfUD11v83h5FHBm735XPwW3moDql9D+Eowj5btXkh18mX3D6RNQRhb2bYDUPJbe niIyWd/NSZi0atLVKXo5MXGM6UAdnVS0c8F2QDG7lAC+qR6BloAb9/pFMj0jXzaBVx lTt6ArN31rv8L2QGXvxg02jWRwjlddyRIwz5EdrA= 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.133.124]) by sourceware.org (Postfix) with ESMTPS id 5AD15385842B for ; Tue, 8 Aug 2023 12:53:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5AD15385842B Received: from mimecast-mx02.redhat.com (66.187.233.73 [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-190-ajyP4rLTOamDg2nXnAvnKQ-1; Tue, 08 Aug 2023 08:53:27 -0400 X-MC-Unique: ajyP4rLTOamDg2nXnAvnKQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 043151C07548 for ; Tue, 8 Aug 2023 12:53:27 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.2.16.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A565C40C6E8A for ; Tue, 8 Aug 2023 12:53:26 +0000 (UTC) To: libc-alpha@sourceware.org Subject: [PATCH 1/2] libio: Introduce _IO_JUMPS_UNVALIDATED Message-ID: X-From-Line: cf56a61681926b77d13046526ff01e705fefbbfe Mon Sep 17 00:00:00 2001 Date: Tue, 08 Aug 2023 14:53:25 +0200 User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Florian Weimer via Libc-alpha From: Florian Weimer Reply-To: Florian Weimer Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" And refactor the macros a bit. Use a safer way to avoid aliasing issues. Reviewed-by: Adhemerval Zanella --- libio/libioP.h | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) base-commit: dcad5c8578130dec7f35fd5b0885304b59f9f543 diff --git a/libio/libioP.h b/libio/libioP.h index 745278e076..0c5276fc4b 100644 --- a/libio/libioP.h +++ b/libio/libioP.h @@ -104,21 +104,41 @@ #define _IO_CHECK_WIDE(THIS) \ (_IO_CAST_FIELD_ACCESS ((THIS), struct _IO_FILE, _wide_data) != NULL) + #if _IO_JUMPS_OFFSET -# define _IO_JUMPS_FUNC(THIS) \ - (IO_validate_vtable \ - (*(struct _IO_jump_t **) ((void *) &_IO_JUMPS_FILE_plus (THIS) \ - + (THIS)->_vtable_offset))) -# define _IO_JUMPS_FUNC_UPDATE(THIS, VTABLE) \ - (*(const struct _IO_jump_t **) ((void *) &_IO_JUMPS_FILE_plus (THIS) \ - + (THIS)->_vtable_offset) = (VTABLE)) # define _IO_vtable_offset(THIS) (THIS)->_vtable_offset #else -# define _IO_JUMPS_FUNC(THIS) (IO_validate_vtable (_IO_JUMPS_FILE_plus (THIS))) -# define _IO_JUMPS_FUNC_UPDATE(THIS, VTABLE) \ - (_IO_JUMPS_FILE_plus (THIS) = (VTABLE)) # define _IO_vtable_offset(THIS) 0 #endif + +/* Pointer to the active vtable pointer in a FILE object. */ +#define _IO_vtable_ptrptr(THIS) \ + ((void *) &_IO_JUMPS_FILE_plus (THIS) + _IO_vtable_offset (THIS)) + +/* Read the vtable pointer in THIS, avoiding an aliasing violation. + This performs no vtable validation, so should only be used for + explicit checks against known vtables. */ +#define _IO_JUMPS_UNVALIDATED(THIS) \ + ({ \ + struct _IO_jump_t *__jump_unvalidated; \ + memcpy (&__jump_unvalidated, _IO_vtable_ptrptr (THIS), \ + sizeof (__jump_unvalidated)); \ + __jump_unvalidated; \ + }) + +/* Like _IO_JUMPS_UNVALIDATED, but with validation. Used to implement + virtual member function calls below. */ +#define _IO_JUMPS_FUNC(THIS) \ + (IO_validate_vtable (_IO_JUMPS_UNVALIDATED (THIS))) + +/* Update the vtable pointer, avoiding an aliasing violation. */ +#define _IO_JUMPS_FUNC_UPDATE(THIS, VTABLE) \ + ({ \ + const struct _IO_jump_t *__jump_copy = (VTABLE); \ + memcpy (_IO_vtable_ptrptr (THIS), &__jump_copy, sizeof (__jump_copy)); \ + (void) 0; \ + }) + #define _IO_WIDE_JUMPS_FUNC(THIS) _IO_WIDE_JUMPS(THIS) #define JUMP_FIELD(TYPE, NAME) TYPE NAME #define JUMP0(FUNC, THIS) (_IO_JUMPS_FUNC(THIS)->FUNC) (THIS) From patchwork Tue Aug 8 12:53:40 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 73815 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E0EFE385B522 for ; Tue, 8 Aug 2023 12:54:07 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E0EFE385B522 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1691499247; bh=w3SdN00f74xErCUjeBxUvFOUiEsv78UYwmSEHJku4m8=; h=To:Subject:In-Reply-To:References:Date:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=T+0cXeJyrbgb7/OIYL3ss/Ws/XvwPGMP3lERPGigN8kXwh4JYCoaM/5ibCQtNL4tB oBqECbU76PS4/KWld3DrkVLwVxwmdgI7dLIOaTmU/MEyZtBwJh0yK4kmxjoAflU0s3 DjuEv7HBuT7mP8C5ui6OLFEj2rgfdr5bzuqTf7qY= 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 ESMTPS id C322C38582BD for ; Tue, 8 Aug 2023 12:53:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C322C38582BD Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-503-xVRiMNVWN320D1gvOwug7A-1; Tue, 08 Aug 2023 08:53:42 -0400 X-MC-Unique: xVRiMNVWN320D1gvOwug7A-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D52DA101AA5F for ; Tue, 8 Aug 2023 12:53:41 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.2.16.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 669342026D4B for ; Tue, 8 Aug 2023 12:53:41 +0000 (UTC) To: libc-alpha@sourceware.org Subject: [PATCH 2/2] libio: Perform write-only fflush as part of freopen (bug 30731) In-Reply-To: Message-ID: <54125d816ab3d073364d760ec5ec4ac0f98d3338.1691499154.git.fweimer@redhat.com> References: X-From-Line: 54125d816ab3d073364d760ec5ec4ac0f98d3338 Mon Sep 17 00:00:00 2001 Date: Tue, 08 Aug 2023 14:53:40 +0200 User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-10.6 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_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Florian Weimer via Libc-alpha From: Florian Weimer Reply-To: Florian Weimer Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" For standard streams that have a file descriptor, the read flush may change the file offset of the underlying file descriptor. This is undeseriable because it makes using freopen after fork unsafe even for read-only files. Tested on i686-linux-gnu and x86_64-linux-gnu. --- libio/Makefile | 2 +- libio/fileops.c | 22 +++++++++ libio/freopen.c | 3 +- libio/freopen64.c | 3 +- libio/libio.h | 4 ++ libio/tst-freopen-no-seek.c | 91 +++++++++++++++++++++++++++++++++++++ 6 files changed, 120 insertions(+), 5 deletions(-) create mode 100644 libio/tst-freopen-no-seek.c diff --git a/libio/Makefile b/libio/Makefile index 287ec11338..0ed1781792 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -83,7 +83,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \ tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof \ tst-sprintf-ub tst-sprintf-chk-ub tst-bz24051 tst-bz24153 \ - tst-wfile-sync tst-bz28828 tst-getdelim + tst-wfile-sync tst-bz28828 tst-getdelim tst-freopen-no-seek tests-internal = tst-vtables tst-vtables-interposed diff --git a/libio/fileops.c b/libio/fileops.c index 1c1113e339..92c4368bb9 100644 --- a/libio/fileops.c +++ b/libio/fileops.c @@ -816,6 +816,28 @@ _IO_new_file_sync (FILE *fp) } libc_hidden_ver (_IO_new_file_sync, _IO_file_sync) +void +__fflush_for_freopen (FILE *fp) +{ + void *vtable = _IO_JUMPS_UNVALIDATED (fp); + if (vtable == &_IO_file_jumps + || vtable == &_IO_file_jumps_mmap + || vtable == &_IO_file_jumps_maybe_mmap + || vtable == &_IO_wfile_jumps + || vtable == &_IO_wfile_jumps_mmap + || vtable == &_IO_wfile_jumps_maybe_mmap) + { + /* For a regular FILE * backed by a file descriptor, only + perform the write flush. A read flush may cause a seek that + only has unwanted side effects because of the immediately + following freopen operation. */ + if (fp->_IO_write_ptr > fp->_IO_write_base) + _IO_do_flush(fp); + } + else + _IO_SYNC (fp); +} + int _IO_file_sync_mmap (FILE *fp) { diff --git a/libio/freopen.c b/libio/freopen.c index 91ff5ec1f9..45e6d2da45 100644 --- a/libio/freopen.c +++ b/libio/freopen.c @@ -42,8 +42,7 @@ freopen (const char *filename, const char *mode, FILE *fp) CHECK_FILE (fp, NULL); _IO_acquire_lock (fp); - /* First flush the stream (failure should be ignored). */ - _IO_SYNC (fp); + __fflush_for_freopen (fp); if (!(fp->_flags & _IO_IS_FILEBUF)) goto end; diff --git a/libio/freopen64.c b/libio/freopen64.c index 9a74f33dd4..c6e6c43d18 100644 --- a/libio/freopen64.c +++ b/libio/freopen64.c @@ -41,8 +41,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp) CHECK_FILE (fp, NULL); _IO_acquire_lock (fp); - /* First flush the stream (failure should be ignored). */ - _IO_SYNC (fp); + __fflush_for_freopen (fp); if (!(fp->_flags & _IO_IS_FILEBUF)) goto end; diff --git a/libio/libio.h b/libio/libio.h index 3d71bc1063..0cc5128e0b 100644 --- a/libio/libio.h +++ b/libio/libio.h @@ -160,6 +160,10 @@ extern wint_t __wunderflow (FILE *); extern wint_t __wuflow (FILE *); extern wint_t __woverflow (FILE *, wint_t); +/* Special fflush operation immediately before freopen FILE * + replacement. Avoids a read flush for descriptor-backed streams. */ +void __fflush_for_freopen (FILE *) attribute_hidden; + #define _IO_getc_unlocked(_fp) __getc_unlocked_body (_fp) #define _IO_peekc_unlocked(_fp) \ (__glibc_unlikely ((_fp)->_IO_read_ptr >= (_fp)->_IO_read_end) \ diff --git a/libio/tst-freopen-no-seek.c b/libio/tst-freopen-no-seek.c new file mode 100644 index 0000000000..00fd37c3c1 --- /dev/null +++ b/libio/tst-freopen-no-seek.c @@ -0,0 +1,91 @@ +/* Test that freopen does not seek unnecessarily (bug 30731). + Copyright (C) 2023 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 +#include +#include + +static void +test (bool do_fdopen, bool do_wide, bool do_freopen64) +{ + char *path; + int fd = create_temp_file ("tst-freopen-no-seek-XXXXXX", &path); + TEST_VERIFY_EXIT (fd >= 0); + + support_write_file_string (path, "freopen test file\n"); + + FILE *fp; + if (do_fdopen) + { + fp = fdopen (fd, "r"); + if (fp == NULL) + FAIL_EXIT1 ("fdopen: %m"); + } + else + { + xclose (fd); + fp = xfopen (path, "r"); + } + free (path); + + /* The fd descriptor shares the same underlying file description as the descriptor underlying fp. In particular, the file offset is shared. */ + fd = dup (fileno (fp)); + if (fd < 0) + FAIL_EXIT1 ("dup: %m"); + + if (do_wide) + TEST_COMPARE (fgetc (fp), 'f'); + else + TEST_COMPARE (fgetc (fp), 'f'); + TEST_COMPARE (xlseek (fd, 0, SEEK_CUR), strlen ("freopen test file\n")); + + if (do_freopen64) + fp = freopen64 ("/dev/null", "r", fp); + else + fp = freopen ("/dev/null", "r", fp); + if (fp == NULL) + FAIL_EXIT1 ("freopen (\"/dev/null\", \"r\"): %m"); + + /* The freopen call should not have changed the seek offset of the + underlying file description because there is nothing to flush as + the stream is opened read-only, and changing the seek offset on a + file descriptor that is closed immediately afterwards is only + visible through unwanted side effects. */ + TEST_COMPARE (xlseek (fd, 0, SEEK_CUR), strlen ("freopen test file\n")); + + xclose (fd); + fclose (fp); +} + +static int +do_test (void) +{ + for (int do_fdopen = 0; do_fdopen < 2; ++do_fdopen) + for (int do_wide = 0; do_wide < 2; ++do_wide) + for (int do_freopen64 = 0; do_freopen64 < 2; ++do_freopen64) + test (do_fdopen, do_wide, do_freopen64); + return 0; +} + +#include