Util-Linux Archive on lore.kernel.org
 help / color / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: util-linux@vger.kernel.org
Cc: Patrick Steinhardt <ps@pks.im>, Karel Zak <kzak@redhat.com>,
	Florian Weimer <fweimer@redhat.com>
Subject: [PATCH v2] include/closestream: fix assignment to read-only standard streams
Date: Thu, 22 Aug 2019 11:40:15 +0200
Message-ID: <3e5f8ff67edc6ae34ff67124c956a71dcf49ea98.1566466780.git.ps@pks.im> (raw)
In-Reply-To: <cover.1565800625.git.ps@pks.im>

In order to avoid closing standard streams multiple times, commit
52aa1a661 (include/closestream: avoid close more than once, 2019-06-13)
introduced code to set the standard output and error streams to `NULL`.
As musl libc defines standard streams as constant pointers, the change
causes compiler errors on systems with that libc. According to ISO C89,
being able to assign to the standard text streams is not a requirement
for any C implementation, see footnote 238 in chapter §7.19.5.6:

    The primary use of the freopen function is to change the file
    associated with a standard text stream (stderr, stdin, or stdout),
    as those identifiers need not be modifiable lvalues to which the
    value returned by the fopen function may be assigned.

This commit implements a new function `flush_standard_stream` that tries
to reliably flush standard streams without actually closing them. By not
calling fclose(3P), we can neatly avoid the issue of accessing standard
streams in an unspecified state and thus remove the infringing `NULL`
assignments.

Properly flushing standard streams without fclose(3P) proves to be more
intricate than one may expect, though, as some filesystems like NFS may
defer flushing until they see a close(3P) of the underlying descriptor.
One may call fsync(3P) to remedy that, but this may incur a heavy
performance penalty in some scenarios. To work around the issue and
still get proper errors, we duplicate the stream's file descriptor and
close that one instead, which is sufficient to cause a flush.

Note that both `close_stdout` and `close_stdout_atexit` are misnamed
after this change, as we do not actually close the streams now. In order
to avoid unnecessary code churn, we still retain their current names.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 include/closestream.h | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/include/closestream.h b/include/closestream.h
index 83df1ee7d..41afbe208 100644
--- a/include/closestream.h
+++ b/include/closestream.h
@@ -35,11 +35,37 @@ close_stream(FILE * stream)
 	return 0;
 }
 
+static inline int
+flush_standard_stream(FILE *stream)
+{
+	int fd;
+
+	errno = 0;
+
+	if (ferror(stream) != 0 || fflush(stream) != 0)
+		goto error;
+
+	/*
+	 * Calling fflush is not sufficient on some filesystems
+	 * like e.g. NFS, which may defer the actual flush until
+	 * close. Calling fsync would help solve this, but would
+	 * probably result in a performance hit. Thus, we work
+	 * around this issue by calling close on a dup'd file
+	 * descriptor from the stream.
+	 */
+	if ((fd = fileno(stream)) < 0 || (fd = dup(fd)) < 0 || close(fd) != 0)
+		goto error;
+
+	return 0;
+error:
+	return (errno == EBADF) ? 0 : EOF;
+}
+
 /* Meant to be used atexit(close_stdout); */
 static inline void
 close_stdout(void)
 {
-	if (stdout && close_stream(stdout) != 0 && !(errno == EPIPE)) {
+	if (flush_standard_stream(stdout) != 0 && !(errno == EPIPE)) {
 		if (errno)
 			warn(_("write error"));
 		else
@@ -47,11 +73,8 @@ close_stdout(void)
 		_exit(CLOSE_EXIT_CODE);
 	}
 
-	if (stderr && close_stream(stderr) != 0)
+	if (flush_standard_stream(stderr) != 0)
 		_exit(CLOSE_EXIT_CODE);
-
-	stdout = NULL;
-	stderr = NULL;
 }
 
 static inline void
-- 
2.23.0


  parent reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-14 16:45 [PATCH 0/4] Fix closing of standard text streams for non-glibc system Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 1/4] term-utils/ttymsg: fix missing header for ARRAY_SIZE macro Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 2/4] login-utils/islocal: fix missing header for err macro Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 3/4] lib/closestream: move implementation into its own compilation unit Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 4/4] lib/closestream: fix assignment to read-only standard streams Patrick Steinhardt
2019-08-19 13:36 ` [PATCH 0/4] Fix closing of standard text streams for non-glibc system Karel Zak
2019-08-20 13:17   ` Patrick Steinhardt
2019-08-20 15:04     ` Karel Zak
2019-08-20 15:11       ` Florian Weimer
2019-08-23 11:52       ` Karel Zak
2019-08-22  9:40 ` Patrick Steinhardt [this message]
2019-08-23 12:00   ` [PATCH v2] include/closestream: fix assignment to read-only standard streams Karel Zak
2019-09-02 10:01   ` Karel Zak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3e5f8ff67edc6ae34ff67124c956a71dcf49ea98.1566466780.git.ps@pks.im \
    --to=ps@pks.im \
    --cc=fweimer@redhat.com \
    --cc=kzak@redhat.com \
    --cc=util-linux@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Util-Linux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/util-linux/0 util-linux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 util-linux util-linux/ https://lore.kernel.org/util-linux \
		util-linux@vger.kernel.org
	public-inbox-index util-linux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.util-linux


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git