| Message ID | 20241008190203.2856619-1-tuliom@ascii.art.br |
|---|---|
| State | New |
| Headers | show |
| Series | libio: Start to return errors when flushing fwrite's buffer | expand |
* Tulio Magno Quites Machado Filho: > diff --git a/libio/iofwrite.c b/libio/iofwrite.c > index af2e2070aff..8c0323947ae 100644 > --- a/libio/iofwrite.c > +++ b/libio/iofwrite.c > @@ -38,12 +38,25 @@ _IO_fwrite (const void *buf, size_t size, size_t count, FILE *fp) > if (_IO_vtable_offset (fp) != 0 || _IO_fwide (fp, -1) == -1) > written = _IO_sputn (fp, (const char *) buf, request); > _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) > + if (written == request) > + /* We have written all of the input successfully. */ > return count; > + else if (written == EOF) > + { > + /* sputn() has the same semantics as fputs(), returning EOF on error. > + It also means we did not manage to flush the buffer, but the data is > + in the buffer and therefore written, which is a conflicting > + scenario. > + > + Confirm that an irrecoverable error happened and return an error, > + i.e. return less than count. > + Otherwise, return success (aka. count) and let the caller try > + again, which is the behavior that fwrite had for years. */ > + if ((fp->_flags & _IO_ERR_SEEN) && errno != 0 && errno != EAGAIN) > + return 0; > + else > + return count; Is it really appropriate to handle EAGAIN differently here? I don't see anything about that in POSIX. In fact, some EAGAIN errors may not actually be recoverable as required by POSIX. I think the proper error handling behavior for flushes would be like this: * If no bytes can be written successfully, return 0. * If some bytes could be written, but nothing from the fwrite request, adjust the buffer pointers to record the fact that those bytes have been written, and return 0. * If some bytes from the fwrite request have been written, and that amount happens to fall on am item boundary, return the number of items that could be written. * If an item could be written partially, we have a few subcases: - The unwritten tail fits into the buffer, and is not the last item. Preserve the unwritten tail in the buffer and return the number of written items, including the partially written item. - The unwritten tail does not fit into the buffer. This is a catastrophic failure that cannot be represented properly with the fwrite interface. Return the number of completely written items as a fallback. In this case, we should consider if we should translate EAGAIN and EWOULDBLOCK to EMSGSIZE because EAGAIN is specified as “This is a temporary condition and later calls to the same routine may complete normally.”, but this is not true because of the buffer management failure and the loss of record boundary synchronization because normal completion just isn't in the picture anymore. A carefully written application will be able to determine this scenario by examining the file-position indicator, which will will reflect the partially-written - The unwritten tail fits into the buffer, but it is the last item. This is another hard-to-represent corner case. I think we should set the error indicator, but report success because all requested items have been written, but not flushed. This case is not to dissimilar to the scenario where fwrite does not trigger a flush, but system is in a state that all future writes to the underlying descriptor will fail. The idea is that it's possible for an application to clear the error indicator, try again, and preserve item boundaries (if the item size is not greater than the buffer size). This is all a bit theoretical at this point because I don't think the way libio is layered, it can actually handle things in this way. But we should be able to something more useful than just returning 0 here. > diff --git a/stdio-common/tst-fwrite-bz29459.c b/stdio-common/tst-fwrite-bz29459.c > new file mode 100644 > index 00000000000..7ced6d419ca > --- /dev/null > +++ b/stdio-common/tst-fwrite-bz29459.c > @@ -0,0 +1,89 @@ > +/* 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. */ I think we should have a FUSE-based test, so that we can also check that the file-position indicator is updated as required by the specification. This requires a seekable file descriptor, which rules out using a pipe. > + for (i = 1; i <= ITERATIONS; i++) > + { > + /* Keep writing to stdout. Success means that fwrite () returns an > + error. */ “Success” is ambiguous here. I think it's the test objective to trigger an fwrite failure? Thanks, Florian
On Okt 31 2024, Florian Weimer wrote: > * If some bytes from the fwrite request have been written, and that > amount happens to fall on am item boundary, return the number of > items that could be written. > > * If an item could be written partially, we have a few subcases: I don't think fwrite should do anything special wrt. item boundaries. Applications that care about partially written items should not use fwrite with size > 1.
* Andreas Schwab: > On Okt 31 2024, Florian Weimer wrote: > >> * If some bytes from the fwrite request have been written, and that >> amount happens to fall on am item boundary, return the number of >> items that could be written. >> >> * If an item could be written partially, we have a few subcases: > > I don't think fwrite should do anything special wrt. item boundaries. > Applications that care about partially written items should not use > fwrite with size > 1. Considering the interface, I would say that there is an expectation that fwrite maintains item boundaries. Otherwise the caller could do the multiplication. But it's probably not a good use of our resources. We can document that the return value is only used to indicate the error status, and that the concrete item count should not matter. I think that's fine if the application can use ftello to figure out how much was written. Thanks, Florian
On Okt 31 2024, Florian Weimer wrote: > Considering the interface, I would say that there is an expectation that > fwrite maintains item boundaries. That's not how fwrite works. It is defined in terms of fputc, like all other output functions. > Otherwise the caller could do the multiplication. Applications that care, do. > I think that's fine if the application can use ftello to figure out > how much was written. The file position unspecified in case of errors.
I failed to make it explicit in the commit message that this patch is fixing bug 29459. Florian Weimer <fweimer@redhat.com> writes: > * Tulio Magno Quites Machado Filho: > >> diff --git a/libio/iofwrite.c b/libio/iofwrite.c >> index af2e2070aff..8c0323947ae 100644 >> --- a/libio/iofwrite.c >> +++ b/libio/iofwrite.c >> @@ -38,12 +38,25 @@ _IO_fwrite (const void *buf, size_t size, size_t count, FILE *fp) >> if (_IO_vtable_offset (fp) != 0 || _IO_fwide (fp, -1) == -1) >> written = _IO_sputn (fp, (const char *) buf, request); >> _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) >> + if (written == request) >> + /* We have written all of the input successfully. */ >> return count; >> + else if (written == EOF) >> + { >> + /* sputn() has the same semantics as fputs(), returning EOF on error. >> + It also means we did not manage to flush the buffer, but the data is >> + in the buffer and therefore written, which is a conflicting >> + scenario. >> + >> + Confirm that an irrecoverable error happened and return an error, >> + i.e. return less than count. >> + Otherwise, return success (aka. count) and let the caller try >> + again, which is the behavior that fwrite had for years. */ >> + if ((fp->_flags & _IO_ERR_SEEN) && errno != 0 && errno != EAGAIN) >> + return 0; >> + else >> + return count; > > Is it really appropriate to handle EAGAIN differently here? I don't see > anything about that in POSIX. In fact, some EAGAIN errors may not > actually be recoverable as required by POSIX. IMHO, this is the most important question that I'd like to answer with this fix. I'm happy to treat all cases where sputn() returns EOF as errors. But I realized that we might be able to keep the old behavior (i.e. returning success) if a recoverable error is found. I do agree that treating EAGAIN as "recoverable error" is not optimal. With that said, I return this question to you and the community: should we treat recoverable errors differently? > The idea is that it's possible for an application to clear the error > indicator, try again, and preserve item boundaries (if the item size is > not greater than the buffer size). This is all a bit theoretical at > this point because I don't think the way libio is layered, it can > actually handle things in this way. But we should be able to something > more useful than just returning 0 here. I don't think we can do this now, but I agree that we should be able to do better. While I do agree with some of your suggestions here, I believe they're orthogonal to this patch, which is intended to fix bug 29459. I'd appreciate if the fix could be merged without requiring to rewrite libio. > I think we should have a FUSE-based test, so that we can also check that > the file-position indicator is updated as required by the specification. > This requires a seekable file descriptor, which rules out using a pipe. I didn't understand your suggestion. Are you suggesting to replace tst-fwrite-bz29459.c with a FUSE-based one? Keep in mind this is the original reproducer from bug 29459. >> + for (i = 1; i <= ITERATIONS; i++) >> + { >> + /* Keep writing to stdout. Success means that fwrite () returns an >> + error. */ > > “Success” is ambiguous here. I think it's the test objective to trigger > an fwrite failure? I tried to clarify it with that source code comment. Would it help if I replaced the current comment with the following? "The test succeeds if fwrite () returns an error."
* Andreas Schwab: > On Okt 31 2024, Florian Weimer wrote: > >> Considering the interface, I would say that there is an expectation that >> fwrite maintains item boundaries. > > That's not how fwrite works. It is defined in terms of fputc, like all > other output functions. > >> Otherwise the caller could do the multiplication. > > Applications that care, do. > >> I think that's fine if the application can use ftello to figure out >> how much was written. > > The file position unspecified in case of errors. That's a good point. But I think POSIX does not make a distinction for the unspecified file-position indicator whether the item size is 1 or not. If I read it correctly, it's always undefined on error. Thanks, Florian
* Tulio Magno Quites Machado Filho: >> Is it really appropriate to handle EAGAIN differently here? I don't see >> anything about that in POSIX. In fact, some EAGAIN errors may not >> actually be recoverable as required by POSIX. > > IMHO, this is the most important question that I'd like to answer with > this fix. > I'm happy to treat all cases where sputn() returns EOF as errors. > But I realized that we might be able to keep the old behavior > (i.e. returning success) if a recoverable error is found. > > I do agree that treating EAGAIN as "recoverable error" is not optimal. > > With that said, I return this question to you and the community: > should we treat recoverable errors differently? I think Andreas leans towards no. I would like to stress again that straightforward recovery needs the stream to be in a knowable state regarding item boundaries. So you need to know how many items have been written, and somehow deal with the partially written item case. That's not possible to provide in general depending on buffer size. And merely ignoring an error doesn't address it, either. It's also questionable how worthwhile all this effort is in practice. I'm not sure what the Linux error handling story is, even with O_SYNC. It doesn't really matter what we do at the stdio layer if the kernel cannot provide synchronous write error reporting. Or any type of write error causes the file system to be remounted read-only anyway. >> The idea is that it's possible for an application to clear the error >> indicator, try again, and preserve item boundaries (if the item size is >> not greater than the buffer size). This is all a bit theoretical at >> this point because I don't think the way libio is layered, it can >> actually handle things in this way. But we should be able to something >> more useful than just returning 0 here. > > I don't think we can do this now, but I agree that we should be able to > do better. While I do agree with some of your suggestions here, I > believe they're orthogonal to this patch, which is intended to fix bug > 29459. I'd appreciate if the fix could be merged without requiring to > rewrite libio. I came up with this (untested) way to report a correct item count. diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h index d8d26639d1..290f308584 100644 --- a/libio/bits/types/struct_FILE.h +++ b/libio/bits/types/struct_FILE.h @@ -94,8 +94,10 @@ struct _IO_FILE_complete void *_freeres_buf; struct _IO_FILE **_prevchain; int _mode; + int _unused3; + __uint64_t _total_written; /* 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 *)]; }; /* These macros are used by bits/stdio.h and internal headers. */ diff --git a/libio/fileops.c b/libio/fileops.c index 4db4a76f75..8f5b56e3d2 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); } diff --git a/libio/iofwrite.c b/libio/iofwrite.c index af2e2070af..7aaa9e43c1 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; + /* 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; Not sure if that's acceptable, it's kind of a hack. It should play relatively well with vtable interposition stuff. It may report 0 instead of the actually written item count with a SYSWRITE override that does not update _total_written. Threading the byte count through all the existing interfaces is indeed rather hard. >> I think we should have a FUSE-based test, so that we can also check that >> the file-position indicator is updated as required by the specification. >> This requires a seekable file descriptor, which rules out using a pipe. > > I didn't understand your suggestion. Are you suggesting to replace > tst-fwrite-bz29459.c with a FUSE-based one? > Keep in mind this is the original reproducer from bug 29459. We can keep this test, sure. I think we need to verify that we actually implement the POSIX rule that the number of successfully written items is returned. >>> + for (i = 1; i <= ITERATIONS; i++) >>> + { >>> + /* Keep writing to stdout. Success means that fwrite () returns an >>> + error. */ >> >> “Success” is ambiguous here. I think it's the test objective to trigger >> an fwrite failure? > > I tried to clarify it with that source code comment. > Would it help if I replaced the current comment with the following? > "The test succeeds if fwrite () returns an error." Yes, please. Thanks, Florian
diff --git a/libio/iofwrite.c b/libio/iofwrite.c index af2e2070aff..8c0323947ae 100644 --- a/libio/iofwrite.c +++ b/libio/iofwrite.c @@ -38,12 +38,25 @@ _IO_fwrite (const void *buf, size_t size, size_t count, FILE *fp) if (_IO_vtable_offset (fp) != 0 || _IO_fwide (fp, -1) == -1) written = _IO_sputn (fp, (const char *) buf, request); _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) + if (written == request) + /* We have written all of the input successfully. */ return count; + else if (written == EOF) + { + /* sputn() has the same semantics as fputs(), returning EOF on error. + It also means we did not manage to flush the buffer, but the data is + in the buffer and therefore written, which is a conflicting + scenario. + + Confirm that an irrecoverable error happened and return an error, + i.e. return less than count. + Otherwise, return success (aka. count) and let the caller try + again, which is the behavior that fwrite had for years. */ + if ((fp->_flags & _IO_ERR_SEEN) && errno != 0 && errno != EAGAIN) + return 0; + else + return count; + } else return written / size; } diff --git a/stdio-common/Makefile b/stdio-common/Makefile index 88105b3c1b3..bb5a8b0462d 100644 --- a/stdio-common/Makefile +++ b/stdio-common/Makefile @@ -234,6 +234,7 @@ tests := \ tst-fwrite \ tst-fwrite-memstrm \ tst-fwrite-overflow \ + tst-fwrite-pipe \ tst-fwrite-ro \ tst-getline \ tst-getline-enomem \ @@ -316,6 +317,7 @@ tests-internal = \ # tests-internal test-srcs = \ + tst-fwrite-bz29459 \ tst-printf \ tst-printfsz-islongdouble \ tst-unbputc \ @@ -323,6 +325,7 @@ test-srcs = \ ifeq ($(run-built-tests),yes) tests-special += \ + $(objpfx)tst-fwrite-bz29459.out \ $(objpfx)tst-printf.out \ $(objpfx)tst-printfsz-islongdouble.out \ $(objpfx)tst-setvbuf1-cmp.out \ @@ -517,6 +520,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) diff --git a/stdio-common/tst-fwrite-bz29459.c b/stdio-common/tst-fwrite-bz29459.c new file mode 100644 index 00000000000..7ced6d419ca --- /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. + 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> + +/* 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 + +/* 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); + + /* 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); + + for (i = 1; i <= ITERATIONS; i++) + { + /* Keep writing to stdout. Success means that 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; + } + 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; +} 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. +# 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 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. + 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 + +#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. */ + +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) + { + 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); + + /* 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; + } + 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); + + /* 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. */ + TEST_VERIFY_EXIT (errno == 0); + 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; + } + } + + xfclose (f); + } + + if (ret) + FAIL_RET ("fwrite should have returned an error, but it didn't.\n"); + + return ret; +} + +#include <support/test-driver.c>
From: Tulio Magno Quites Machado Filho <tuliom@redhat.com> When an error happens, fwrite is expected to return a value that is less than nmemb. If this error happens while flushing its internal buffer, fwrite is in a complex scenario: all the data might have been written to the buffer, indicating a successful copy, but the buffer is expected to be flushed and it was not. POSIX.1-2024 states the following about errors on fwrite: If an error occurs, the resulting value of the file-position indicator for the stream is unspecified. The fwrite() function shall return the number of elements successfully written, which may be less than nitems if a write error is encountered. With that in mind, this commit modifies fwrite's behavior in case of an error while flushing the buffer in order to return 0 in case an irrecoverable error is found. Contents copied to the buffer are kept there despite the return value because the error is irrecoverable. In case of recoverable errors, fwrite continues to return success (aka. nmemb), keeping the old behavior and allowing the caller to try again. Add 2 tests: 1. tst-fwrite-bz29459: This test is based on the reproducer attached to bug 29459. In order to work, it requires to pipe stdout to another process making it hard to reuse test-driver.c. This code is more specific to the issue reported. 2. tst-fwrite-pipe: Recreates the issue by creating a pipe that is shared with a child process. Reuses test-driver.c. Evaluates a more generic scenario. --- libio/iofwrite.c | 23 +++-- stdio-common/Makefile | 7 ++ stdio-common/tst-fwrite-bz29459.c | 89 ++++++++++++++++++++ stdio-common/tst-fwrite-bz29459.sh | 34 ++++++++ stdio-common/tst-fwrite-pipe.c | 130 +++++++++++++++++++++++++++++ 5 files changed, 278 insertions(+), 5 deletions(-) create mode 100644 stdio-common/tst-fwrite-bz29459.c create mode 100755 stdio-common/tst-fwrite-bz29459.sh create mode 100644 stdio-common/tst-fwrite-pipe.c