[PATCH v4] libio: Start to return errors when flushing fwrite's buffer [BZ #29459]
DJ Delorie
dj@redhat.com
Tue Jan 14 03:09:26 GMT 2025
Tulio Magno Quites Machado Filho <tuliom@ascii.art.br> writes:
> Changes since v3:
Needs one change to a multi-line fprintf format (noted below) but OK
with that change.
Reviewed-by: DJ Delorie <dj@redhat.com>
> diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
> index 0e8ac64e36..2012d70681 100644
> --- a/libio/bits/types/struct_FILE.h
> +++ b/libio/bits/types/struct_FILE.h
> @@ -97,8 +97,15 @@ struct _IO_FILE_complete
> void *_freeres_buf;
> struct _IO_FILE **_prevchain;
> int _mode;
> +#ifdef __LP64__
> + int _unused3;
> +#endif
> + __uint64_t _total_written;
> +#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 *)];
> };
Ok.
> diff --git a/libio/fileops.c b/libio/fileops.c
> @@ -113,6 +113,7 @@ _IO_new_file_init_internal (struct _IO_FILE_plus *fp)
>
> _IO_link_in (fp);
> fp->file._fileno = -1;
> + fp->file._total_written = 0;
> }
Ok.
> @@ -1185,6 +1186,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 f94493c6ba..7897c4afaf 100644
> --- a/libio/iofwrite.c
> +++ b/libio/iofwrite.c
> @@ -36,13 +36,42 @@ _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);
So let me understand this... original_total_written is how many bytes
made it to write() plus whatever's already queued to go to write(),
i.e. it's the number of bytes that went to write() if the buffer had
been flushed.
After the call, fp->_total_written is the number of bytes that *made it*
to write(), including what might have been in the buffer plus any new
data written. This could be as much as (original count) + (buffered
data) + (new data), or (original_total_written) + (request)
> + if (written == EOF)
This branch only happens when a write() has failed for some reason, and
we don't know how much got written.
> + {
> + /* An error happened and we need to find the appropriate return
> + value. There 3 possible scenarios:
> + 1. If the number of bytes written is between 0..[buffer content],
> + we need to return 0 because none of the bytes from this
> + request have been written;
fp->_total_written will be in the range [original fp->_total_written]
and [original_total_written] here
> + 2. If the number of bytes written is between
> + [buffer content]+1..request-1, that means we managed to write
> + data requested in this fwrite call;
fp->_total_written will be up to original_total_written + request - 1
> + 3. We might have written all the requested data and got an error
> + anyway. We can't return success, which means we still have to
> + return less than request. */
fp->_total_written will be original_total_written + request
> + if (fp->_total_written > original_total_written)
This is cases (2) and (3) above, where some of our data got written.
> + {
> + written = fp->_total_written - original_total_written;
> + /* If everything was reported as written and somehow an
> + error occurred afterwards, avoid reporting success. */
I.e. case (3) above.
> + if (written == request)
> + --written;
> + }
> + else
> + /* Only already-pending buffer contents was written. */
> + written = 0;
This is case (1) above, where none of our own data got written.
> + }
> + }
Ok.
> _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 bbdc1ff709..364ada9a2c 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 \
Ok.
> @@ -357,6 +358,7 @@ tests-internal = \
>
> test-srcs = \
> $(xprintf-srcs) \
> + tst-fwrite-bz29459 \
> tst-printf \
> tst-printfsz-islongdouble \
> tst-unbputc \
Ok.
> @@ -365,6 +367,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 \
Ok.
> @@ -562,6 +565,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)
> +
Ok.
> diff --git a/stdio-common/tst-fwrite-bz29459.c b/stdio-common/tst-fwrite-bz29459.c
> +/* Test fwrite against bug 29459.
> + Copyright (C) 2025 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/>. */
> +
> +/* 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. */
> +
> +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)
rc is unsigned, but fwrite() returns [0..N]. Ok.
> + {
> + /* 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 think we discourage the \ to continue a string construct; preferring
something like this instead:
fprintf(stderr, "Success: i=%d. fwrite returned %zu < %zu "
"and errno=EPIPE\n",
(see support/tst-support_record_failure.c line 120 for an example)
> + 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);
> + }
> + }
> +
> + 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
> +#!/bin/sh
> +# Test fwrite for bug 29459.
> +# Copyright (C) 2025 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/>.
> +
> +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
> +/* Test if fwrite returns EPIPE.
> + Copyright (C) 2025 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 <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <support/check.h>
> +#include <support/xstdio.h>
> +#include <support/xsignal.h>
> +#include <support/xunistd.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
> +
> +#define BUFFERSIZE 20
Ok.
> +/* When the underlying write () fails with EPIPE, fwrite () is expected to
> + return an error by returning < nmemb and keeping errno=EPIPE. */
> +
> +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);
Ok.
> + p = xfork ();
> + if (p == 0)
> + {
> + 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(bytes > 0 && memrchr (b, '\n', bytes) == NULL);
> +
> + /* 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;
> +
> + /* Parent only writes to pipe.
> + Close the unused read end of the pipe. */
> + xclose (fd[0]);
> +
> + /* 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);
> +
> + /* Ensure errno is not set before starting. */
> + errno = 0;
Ok.
> + 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);
> + }
> + 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