[PATCH 1/2] libio: Start to return errors when flushing fwrite's buffer [BZ #29459]
DJ Delorie
dj@redhat.com
Wed Jan 8 02:05:23 GMT 2025
Some questions and suggested comment changes, 1-2 logic changes.
Tulio Magno Quites Machado Filho <tuliom@ascii.art.br> writes:
> diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
> index d8d26639d12..fba5ebbecb2 100644
> --- a/libio/bits/types/struct_FILE.h
> +++ b/libio/bits/types/struct_FILE.h
> @@ -94,8 +94,15 @@ struct _IO_FILE_complete
> void *_freeres_buf;
> struct _IO_FILE **_prevchain;
> int _mode;
> +#ifdef __LP64__
> + int _unused3;
> +#endif
> + __uint64_t _total_written;
What initializes this? Is it a per-fwrite count, or a per-stream count?
> +#ifndef __LP64__
> + int _unused3;
> +#endif
> /* Make sure we don't get into trouble again. */
> - char _unused2[15 * sizeof (int) - 5 * sizeof (void *)];
> + char _unused2[12 * sizeof (int) - 5 * sizeof (void *)];
> };
We're adding an int and an int64, but we remove 3 ints from the unused
list. This happens to work when sizeof(int) = sizeof(int32) which I
think is the case for all the targets we support, but this could be
better handled.
> /* These macros are used by bits/stdio.h and internal headers. */
> diff --git a/libio/fileops.c b/libio/fileops.c
> index 759d737ec7e..f7ba519465a 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -1184,6 +1184,7 @@ _IO_new_file_write (FILE *f, const void *data, ssize_t n)
> f->_flags |= _IO_ERR_SEEN;
> break;
> }
> + f->_total_written += count;
> to_do -= count;
> data = (void *) ((char *) data + count);
> }
Ok.
> diff --git a/libio/iofwrite.c b/libio/iofwrite.c
> index af2e2070aff..7aaa9e43c1f 100644
> --- a/libio/iofwrite.c
> +++ b/libio/iofwrite.c
> @@ -36,13 +36,31 @@ _IO_fwrite (const void *buf, size_t size, size_t count, FILE *fp)
> return 0;
> _IO_acquire_lock (fp);
> if (_IO_vtable_offset (fp) != 0 || _IO_fwide (fp, -1) == -1)
> - written = _IO_sputn (fp, (const char *) buf, request);
> + {
> + /* Compute actually written bytes plus pending buffer
> + contents. */
> + uint64_t original_total_written
> + = fp->_total_written + (fp->_IO_write_ptr - fp->_IO_write_base);
> + written = _IO_sputn (fp, (const char *) buf, request);
> + if (written == EOF)
> + {
> + if (fp->_total_written > original_total_written)
> + {
> + written = fp->_total_written - original_total_written;
I don't understand why you're including the
buffered-but-unwritten-to-disk count in the first case, but not in the
second case. Are you assuming the buffered data is written out in
addition to the data passed? If so, a comment to that effect might be
useful.
> + /* If everything was reported as written and somehow an
> + error occurred afterwards, avoid reporting success. */
> + if (written == request)
> + --written;
> + }
> + else
> + /* Only already-pending buffer contents was written. */
> + written = 0;
> + }
> + }
> _IO_release_lock (fp);
> /* We have written all of the input in case the return value indicates
> - this or EOF is returned. The latter is a special case where we
> - simply did not manage to flush the buffer. But the data is in the
> - buffer and therefore written as far as fwrite is concerned. */
> - if (written == request || written == EOF)
> + this. */
> + if (written == request)
> return count;
> else
> return written / size;
Ok.
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index e76e40e5879..46aa31aa793 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -262,6 +262,7 @@ tests := \
> tst-fwrite \
> tst-fwrite-memstrm \
> tst-fwrite-overflow \
> + tst-fwrite-pipe \
> tst-fwrite-ro \
> tst-getline \
> tst-getline-enomem \
> @@ -346,6 +347,7 @@ tests-internal = \
>
> test-srcs = \
> $(xprintf-srcs) \
> + tst-fwrite-bz29459 \
> tst-printf \
> tst-printfsz-islongdouble \
> tst-unbputc \
> @@ -354,6 +356,7 @@ test-srcs = \
> ifeq ($(run-built-tests),yes)
> tests-special += \
> $(foreach f,$(xprintf-stems),$(objpfx)$(f).out) \
> + $(objpfx)tst-fwrite-bz29459.out \
> $(objpfx)tst-printf.out \
> $(objpfx)tst-printfsz-islongdouble.out \
> $(objpfx)tst-setvbuf1-cmp.out \
> @@ -551,6 +554,10 @@ tst-freopen64-6-ENV = \
> MALLOC_TRACE=$(objpfx)tst-freopen64-6.mtrace \
> LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so
>
> +$(objpfx)tst-fwrite-bz29459.out: tst-fwrite-bz29459.sh $(objpfx)tst-fwrite-bz29459
> + $(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \
> + $(evaluate-test)
> +
> $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc
> $(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \
> $(evaluate-test)
Ok.
> diff --git a/stdio-common/tst-fwrite-bz29459.c b/stdio-common/tst-fwrite-bz29459.c
> new file mode 100644
> index 00000000000..89d0e9bf83c
> --- /dev/null
> +++ b/stdio-common/tst-fwrite-bz29459.c
> @@ -0,0 +1,89 @@
> +/* Test fwrite against bug 29459.
> + Copyright (C) 2024 Free Software Foundation, Inc.
Needs 2025 now :-)
> + 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/>. */
> +
> +/* This test is based on the code attached to bug 29459.
> + It depends on stdout being redirected to a specific process via a script
> + with the same name. Because of this, we cannot use the features from
> + test_driver.c. */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <support/check.h>
> +#include <support/xsignal.h>
Ok.
> +/* Usually this test reproduces in a few iterations. However, keep a high
> + number of iterations in order to avoid return false-positives due to an
> + overwhelmed/slow system. */
> +#define ITERATIONS 5000
Ok.
> +/* The goal of this test is to use fwrite () on a redirected and closed
> + stdout. A script will guarantee that stdout is redirected to another
> + process that closes it during the execution. The process reading from
> + the pipe must read at least the first line in order to guarantee that
> + flag _IO_CURRENTLY_PUTTING is set in the write end of the pipe, triggering
> + important parts of the code that flushes lines from fwrite's internal
> + buffer. The underlying write () returns EPIPE, which fwrite () must
> + propagate. */
Ok.
> +int
> +main (void)
> +{
> + int i;
> + size_t rc;
> + /* Ensure the string we send has a new line because we're dealing
> + with a lined-buffered stream. */
> + const char *s = "hello\n";
> + const size_t len = strlen(s);
Ok.
> + /* Ensure that fwrite buffers the output before writing to stdout. */
> + setlinebuf(stdout);
> +
> + /* Ignore SIGPIPE in order to catch the EPIPE returned by the
> + underlying call to write(). */
> + xsignal(SIGPIPE, SIG_IGN);
Ok.
> + for (i = 1; i <= ITERATIONS; i++)
> + {
> + /* Keep writing to stdout. The test succeeds if fwrite () returns an
> + error. */
> + if ((rc = fwrite(s, 1, len, stdout)) < len)
> + {
> + /* An error happened. Check if ferror () does return an error
> + and that it is indeed EPIPE. */
> + TEST_COMPARE (ferror (stdout), 1);
> + TEST_COMPARE (errno, EPIPE);
> + fprintf(stderr, "Success: i=%d. fwrite returned %zu < %zu \
> +and errno=EPIPE\n",
> + i, rc, len);
> + /* The test succeeded! */
> + return 0;
> + }
Ok.
> + else
> + {
> + /* fwrite () was able to write all the contents. Check if no errors
> + have been reported and try again. */
> + TEST_COMPARE (ferror (stdout), 0);
> + TEST_COMPARE (errno, 0);
> + }
> + }
Ok.
> + fprintf(stderr, "Error: fwrite did not return an error\n");
> + return 1;
> +}
Ok.
> diff --git a/stdio-common/tst-fwrite-bz29459.sh b/stdio-common/tst-fwrite-bz29459.sh
> new file mode 100755
> index 00000000000..c63e94bd6bf
> --- /dev/null
> +++ b/stdio-common/tst-fwrite-bz29459.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +# Test fwrite for bug 29459.
> +# Copyright (C) 2024 Free Software Foundation, Inc.
2025 here too.
> +# 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/>.
> +
> +set -e
> +
> +common_objpfx=$1; shift
> +test_program_prefix=$1; shift
> +
> +status=0
> +
> +${test_program_prefix} \
> + ${common_objpfx}stdio-common/tst-fwrite-bz29459 \
> + 2> ${common_objpfx}stdio-common/tst-fwrite-bz29459.out \
> + | head -n1 > /dev/null
> +
> +grep -q Success ${common_objpfx}stdio-common/tst-fwrite-bz29459.out || status=1
> +
> +exit $status
Ok.
> diff --git a/stdio-common/tst-fwrite-pipe.c b/stdio-common/tst-fwrite-pipe.c
> new file mode 100644
> index 00000000000..7f88e962655
> --- /dev/null
> +++ b/stdio-common/tst-fwrite-pipe.c
> @@ -0,0 +1,130 @@
> +/* Test if fwrite returns EPIPE.
> + Copyright (C) 2024 Free Software Foundation, Inc.
Here too.
> + 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 <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <support/check.h>
> +#include <support/xstdio.h>
> +#include <support/xsignal.h>
> +#include <support/xunistd.h>
> +
> +/* Usually this test reproduces in a few iterations. However, keep a high
> + number of iterations in order to avoid return false-positives due to an
> + overwhelmed/slow system. */
> +#define ITERATIONS 5000
Ok.
> +#define BUFFERSIZE 20
> +
> +/* When the underlying write () fails with EPIPE, fwrite () is expected to
> + return an error by returning < size*nmemb and keeping errno=EPIPE. */
fwrite returns less than nmemb, not less than size*nmemb.
> +static int
> +do_test (void)
> +{
> + int fd[2];
> + pid_t p;
> + FILE *f;
> + size_t written;
> + int ret = 1; /* Return failure by default. */
> +
> + /* Try to create a pipe. */
> + xpipe (fd);
> +
> + p = xfork ();
> + if (p == 0)
This is the child side, ok.
> + {
> + char b[BUFFERSIZE];
> + size_t bytes;
> +
> + /* Read at least the first line from the pipe before closing it.
> + This is important because it guarantees the file stream will have
> + flag _IO_CURRENTLY_PUTTING set, which triggers important parts of
> + the code that flushes lines from fwrite's internal buffer. */
> + do {
> + bytes = read (fd[0], b, BUFFERSIZE);
> + } while(memrchr (b, '\n', bytes) == NULL);
This needs a "bytes > 0 &&" in the while, in case of read error.
> +
> + /* Child closes both ends of the pipe in order to trigger an EPIPE
> + error on the parent. */
> + xclose (fd[0]);
> + xclose (fd[1]);
> +
> + return 0;
> + }
Ok.
> + else
> + {
> + /* Ensure the string we send has a new line because we're dealing
> + with a lined-buffered stream. */
> + const char *s = "hello\n";
> + size_t len = strlen (s);
> + int i;
Ok.
> + /* Parent only writes to pipe.
> + Close the unused read end of the pipe. */
> + xclose (fd[0]);
Ok.
> + /* Ignore SIGPIPE in order to catch the EPIPE returned by the
> + underlying call to write(). */
> + xsignal(SIGPIPE, SIG_IGN);
Ok.
> + /* Create a file stream associated with the write end of the pipe. */
> + f = fdopen (fd[1], "w");
> + TEST_VERIFY_EXIT (f != NULL);
> + /* Ensure that fwrite buffers the output before writing to the pipe. */
> + setlinebuf (f);
Ok.
> + /* Ensure errno is not set before starting. */
> + TEST_VERIFY_EXIT (errno == 0);
Or you could have set errno to zero, which is what the standards
typically suggest.
> + for (i = 1; i <= ITERATIONS; i++)
> + {
> + /* Try to write to the pipe. The first calls are expected to
> + suceeded until the child process closes the read end.
> + After that, fwrite () is expected to fail and errno should be
> + set to EPIPE. */
> + written = fwrite (s, 1, len, f);
> +
> + if (written == len)
> + {
> + TEST_VERIFY_EXIT (ferror (f) == 0);
> + TEST_VERIFY_EXIT (errno == 0);
> + }
Ok.
> + else
> + {
> + /* An error happened. Check if ferror () does return an error
> + and that it is indeed EPIPE. */
> + TEST_COMPARE (ferror (f), 1);
> + TEST_COMPARE (errno, EPIPE);
> + /* The test succeeded! Clear the error from the file stream and
> + return success. */
> + clearerr (f);
> + ret = 0;
> + break;
> + }
> + }
Ok.
> + xfclose (f);
> + }
> +
> + if (ret)
> + FAIL_RET ("fwrite should have returned an error, but it didn't.\n");
> +
> + return ret;
> +}
> +
> +#include <support/test-driver.c>
Ok.
More information about the Libc-alpha
mailing list