[PATCH 0/4] Fix closing of standard text streams for non-glibc system

[PATCH 0/4] Fix closing of standard text streams for non-glibc system

[Patrick Steinhardt]

Hi,

since commit 52aa1a661 (include/closestream: avoid close more
than once, 2019-06-13), util-linux fails to build on musl libc
based systems. The culprit here is that it introduced assignments
to stderr and stdout, while the C89 standard explicitly notes
that treating stderr and stdout as valid lvalues is not a
requirement for any conforming C implementation. musl libc
implemented these streams as `extern FILE *const`, and as a
result assigning to these variables causes compiler errors.

Attached is a fix for this. Instead of assigning `NULL` to the
streams, util-linux now uses a static variable `streams_closed`.
Unfortunately, this fix necessitated some shifting around as
closestream was previously implemented as header, only, and
implementing static variables inside of a header is not going to
work due to them being static to the single compilation unit,
only. Thus I converted the code to move the implementation into
"lib/closestream.c".

Regards
Patrick

Patrick Steinhardt (4):
  term-utils/ttymsg: fix missing header for ARRAY_SIZE macro
  login-utils/islocal: fix missing header for err macro
  lib/closestream: move implementation into its own compilation unit
  lib/closestream: fix assignment to read-only standard streams

 disk-utils/Makemodule.am                   |  2 +
 include/closestream.h                      | 90 +++-------------------
 lib/Makemodule.am                          |  1 +
 include/closestream.h => lib/closestream.c | 27 ++++---
 login-utils/Makemodule.am                  |  4 +-
 login-utils/islocal.c                      |  1 +
 misc-utils/Makemodule.am                   | 10 ++-
 sys-utils/Makemodule.am                    |  9 ++-
 term-utils/ttymsg.c                        |  1 +
 text-utils/Makemodule.am                   |  6 +-
 10 files changed, 52 insertions(+), 99 deletions(-)
 copy include/closestream.h => lib/closestream.c (75%)

-- 
2.22.1

[PATCH 1/4] term-utils/ttymsg: fix missing header for ARRAY_SIZE macro

[Patrick Steinhardt]

While "term-utils/ttymsg.c" makes use of the `ARRAY_SIZE` macro, it
doesn't include the "c.h" header that declares it. Until now, it has
been transitively included via "closestream.h", but as closestream's
implementation is about to get moved to its own compilation unit in
"lib/", this transitive include is going to be removed.

Explicitly include "c.h" to fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 term-utils/ttymsg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/term-utils/ttymsg.c b/term-utils/ttymsg.c
index 2aab69f10..04c867e95 100644
--- a/term-utils/ttymsg.c
+++ b/term-utils/ttymsg.c
@@ -53,6 +53,7 @@
 #include <string.h>
 #include <stdlib.h>
 
+#include "c.h"
 #include "nls.h"
 #include "closestream.h"
 #include "pathnames.h"
-- 
2.22.1

[PATCH 2/4] login-utils/islocal: fix missing header for err macro

[Patrick Steinhardt]

While "login-utils/islocal.c" makes use of the `err` macro, it doesn't
include the "c.h" header that declares it. Until now, it has been
transitively included via "closestream.h", but as closestream's
implementation is about to get moved to its own compilation unit in
"lib/", this transitive include is going to be removed.

Explicitly include "c.h" to fix this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 login-utils/islocal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/login-utils/islocal.c b/login-utils/islocal.c
index 469bc5629..971e9626f 100644
--- a/login-utils/islocal.c
+++ b/login-utils/islocal.c
@@ -22,6 +22,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 
+#include "c.h"
 #include "closestream.h"
 #include "islocal.h"
 #include "nls.h"
-- 
2.22.1

[PATCH 3/4] lib/closestream: move implementation into its own compilation unit

[Patrick Steinhardt]

Currently, functionality provided by "closestream.h" is completely
implemented in the header only and does not have a corresponding C file
in "lib/". This makes it impossible to have global static data for use
with the closestream functions, as all static variables would get
declared per compilation unit that includes "closestream.h". As this is
required to make closing standard text streams work on non-glibc
platforms, move the implementation to "lib/closestream.c" and only keep
definitions in the header.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 disk-utils/Makemodule.am                   |  2 +
 include/closestream.h                      | 90 +++-------------------
 lib/Makemodule.am                          |  1 +
 include/closestream.h => lib/closestream.c | 18 +++--
 login-utils/Makemodule.am                  |  4 +-
 misc-utils/Makemodule.am                   | 10 ++-
 sys-utils/Makemodule.am                    |  9 ++-
 text-utils/Makemodule.am                   |  6 +-
 8 files changed, 45 insertions(+), 95 deletions(-)
 copy include/closestream.h => lib/closestream.c (86%)

diff --git a/disk-utils/Makemodule.am b/disk-utils/Makemodule.am
index bea0ed6a6..1676e40fd 100644
--- a/disk-utils/Makemodule.am
+++ b/disk-utils/Makemodule.am
@@ -25,6 +25,7 @@ if BUILD_MKFS
 sbin_PROGRAMS += mkfs
 dist_man_MANS += disk-utils/mkfs.8
 mkfs_SOURCES = disk-utils/mkfs.c
+mkfs_LDADD = $(LDADD) libcommon.la
 endif
 
 
@@ -98,6 +99,7 @@ if BUILD_RAW
 sbin_PROGRAMS += raw
 dist_man_MANS += disk-utils/raw.8
 raw_SOURCES = disk-utils/raw.c
+raw_LDADD = $(LDADD) libcommon.la
 endif
 
 
diff --git a/include/closestream.h b/include/closestream.h
index 83df1ee7d..8c72dde8f 100644
--- a/include/closestream.h
+++ b/include/closestream.h
@@ -1,87 +1,17 @@
+/*
+ * Copyright (C) 2019 Patrick Steinhardt <ps@pks.im>
+ *
+ * This file may be distributed under the terms of the
+ * GNU Lesser General Public License.
+ */
+
 #ifndef UTIL_LINUX_CLOSESTREAM_H
 #define UTIL_LINUX_CLOSESTREAM_H
 
 #include <stdio.h>
-#ifdef HAVE_STDIO_EXT_H
-#include <stdio_ext.h>
-#endif
-#include <unistd.h>
 
-#include "c.h"
-#include "nls.h"
-
-#ifndef CLOSE_EXIT_CODE
-# define CLOSE_EXIT_CODE EXIT_FAILURE
-#endif
-
-static inline int
-close_stream(FILE * stream)
-{
-#ifdef HAVE___FPENDING
-	const int some_pending = (__fpending(stream) != 0);
-#endif
-	const int prev_fail = (ferror(stream) != 0);
-	const int fclose_fail = (fclose(stream) != 0);
-
-	if (prev_fail || (fclose_fail && (
-#ifdef HAVE___FPENDING
-					  some_pending ||
-#endif
-					  errno != EBADF))) {
-		if (!fclose_fail && !(errno == EPIPE))
-			errno = 0;
-		return EOF;
-	}
-	return 0;
-}
-
-/* Meant to be used atexit(close_stdout); */
-static inline void
-close_stdout(void)
-{
-	if (stdout && close_stream(stdout) != 0 && !(errno == EPIPE)) {
-		if (errno)
-			warn(_("write error"));
-		else
-			warnx(_("write error"));
-		_exit(CLOSE_EXIT_CODE);
-	}
-
-	if (stderr && close_stream(stderr) != 0)
-		_exit(CLOSE_EXIT_CODE);
-
-	stdout = NULL;
-	stderr = NULL;
-}
-
-static inline void
-close_stdout_atexit(void)
-{
-	/*
-	 * Note that close stdout at exit disables ASAN to report memory leaks
-	 */
-#if !defined(__SANITIZE_ADDRESS__)
-	atexit(close_stdout);
-#endif
-}
-
-#ifndef HAVE_FSYNC
-static inline int
-fsync(int fd __attribute__((__unused__)))
-{
-	return 0;
-}
-#endif
-
-static inline int
-close_fd(int fd)
-{
-	const int fsync_fail = (fsync(fd) != 0);
-	const int close_fail = (close(fd) != 0);
-
-	if (fsync_fail || close_fail)
-		return EOF;
-	return 0;
-}
+extern int close_stream(FILE *stream);
+extern void close_stdout_atexit(void);
+extern int close_fd(int fd);
 
 #endif /* UTIL_LINUX_CLOSESTREAM_H */
diff --git a/lib/Makemodule.am b/lib/Makemodule.am
index 862a06c17..056311a40 100644
--- a/lib/Makemodule.am
+++ b/lib/Makemodule.am
@@ -4,6 +4,7 @@ libcommon_la_CFLAGS = $(AM_CFLAGS)
 libcommon_la_SOURCES = \
 	lib/blkdev.c \
 	lib/canonicalize.c \
+	lib/closestream.c \
 	lib/crc32.c \
 	lib/crc32c.c \
 	lib/env.c \
diff --git a/include/closestream.h b/lib/closestream.c
similarity index 86%
copy from include/closestream.h
copy to lib/closestream.c
index 83df1ee7d..d735e4f01 100644
--- a/include/closestream.h
+++ b/lib/closestream.c
@@ -1,5 +1,9 @@
-#ifndef UTIL_LINUX_CLOSESTREAM_H
-#define UTIL_LINUX_CLOSESTREAM_H
+/*
+ * Copyright (C) 2019 Patrick Steinhardt <ps@pks.im>
+ *
+ * This file may be distributed under the terms of the
+ * GNU Lesser General Public License.
+ */
 
 #include <stdio.h>
 #ifdef HAVE_STDIO_EXT_H
@@ -7,6 +11,7 @@
 #endif
 #include <unistd.h>
 
+#include "closestream.h"
 #include "c.h"
 #include "nls.h"
 
@@ -14,7 +19,7 @@
 # define CLOSE_EXIT_CODE EXIT_FAILURE
 #endif
 
-static inline int
+int
 close_stream(FILE * stream)
 {
 #ifdef HAVE___FPENDING
@@ -54,8 +59,7 @@ close_stdout(void)
 	stderr = NULL;
 }
 
-static inline void
-close_stdout_atexit(void)
+void close_stdout_atexit(void)
 {
 	/*
 	 * Note that close stdout at exit disables ASAN to report memory leaks
@@ -73,7 +77,7 @@ fsync(int fd __attribute__((__unused__)))
 }
 #endif
 
-static inline int
+int
 close_fd(int fd)
 {
 	const int fsync_fail = (fsync(fd) != 0);
@@ -83,5 +87,3 @@ close_fd(int fd)
 		return EOF;
 	return 0;
 }
-
-#endif /* UTIL_LINUX_CLOSESTREAM_H */
diff --git a/login-utils/Makemodule.am b/login-utils/Makemodule.am
index aafbea307..740cc0721 100644
--- a/login-utils/Makemodule.am
+++ b/login-utils/Makemodule.am
@@ -122,7 +122,7 @@ chfn_SOURCES = \
 	$(chfn_chsh_sources)
 chfn_CFLAGS = $(chfn_chsh_cflags)
 chfn_LDFLAGS = $(chfn_chsh_ldflags)
-chfn_LDADD = $(LDADD) $(chfn_chsh_ldadd)
+chfn_LDADD = $(LDADD) libcommon.la $(chfn_chsh_ldadd)
 
 chsh_SOURCES = login-utils/chsh.c $(chfn_chsh_sources)
 chsh_CFLAGS = $(chfn_chsh_cflags)
@@ -226,11 +226,13 @@ check_PROGRAMS += \
 
 test_islocal_SOURCES = login-utils/islocal.c
 test_islocal_CPPFLAGS = -DTEST_PROGRAM $(AM_CPPFLAGS)
+test_islocal_LDADD = $(LDADD) libcommon.la
 
 test_logindefs_SOURCES = \
 	login-utils/logindefs.c \
 	login-utils/logindefs.h
 test_logindefs_CPPFLAGS = -DTEST_PROGRAM $(AM_CPPFLAGS)
+test_logindefs_LDADD = $(LDADD) libcommon.la
 
 
 install-exec-hook:
diff --git a/misc-utils/Makemodule.am b/misc-utils/Makemodule.am
index f56a819ac..34ee3e905 100644
--- a/misc-utils/Makemodule.am
+++ b/misc-utils/Makemodule.am
@@ -26,7 +26,7 @@ if BUILD_LOGGER
 usrbin_exec_PROGRAMS += logger
 dist_man_MANS += misc-utils/logger.1
 logger_SOURCES = misc-utils/logger.c lib/strutils.c lib/strv.c
-logger_LDADD = $(LDADD)
+logger_LDADD = $(LDADD) libcommon.la
 logger_CFLAGS = $(AM_CFLAGS)
 if HAVE_SYSTEMD
 logger_LDADD += $(SYSTEMD_LIBS) $(SYSTEMD_DAEMON_LIBS) $(SYSTEMD_JOURNAL_LIBS)
@@ -44,6 +44,7 @@ if BUILD_LOOK
 usrbin_exec_PROGRAMS += look
 dist_man_MANS += misc-utils/look.1
 look_SOURCES = misc-utils/look.c
+look_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_MCOOKIE
@@ -57,6 +58,7 @@ if BUILD_NAMEI
 usrbin_exec_PROGRAMS += namei
 dist_man_MANS += misc-utils/namei.1
 namei_SOURCES = misc-utils/namei.c lib/strutils.c lib/idcache.c
+namei_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_WHEREIS
@@ -94,7 +96,7 @@ if BUILD_UUIDGEN
 usrbin_exec_PROGRAMS += uuidgen
 dist_man_MANS += misc-utils/uuidgen.1
 uuidgen_SOURCES = misc-utils/uuidgen.c
-uuidgen_LDADD = $(LDADD) libuuid.la
+uuidgen_LDADD = $(LDADD) libcommon.la libuuid.la
 uuidgen_CFLAGS = $(AM_CFLAGS) -I$(ul_libuuid_incdir)
 endif
 
@@ -153,7 +155,7 @@ endif # BUILD_BLKID
 if BUILD_FINDFS
 sbin_PROGRAMS += findfs
 dist_man_MANS += misc-utils/findfs.8
-findfs_LDADD = $(LDADD) libblkid.la
+findfs_LDADD = $(LDADD) libblkid.la libcommon.la
 findfs_SOURCES = misc-utils/findfs.c
 findfs_CFLAGS = $(AM_CFLAGS) -I$(ul_libblkid_incdir)
 endif
@@ -197,12 +199,14 @@ if BUILD_RENAME
 usrbin_exec_PROGRAMS += rename
 dist_man_MANS += misc-utils/rename.1
 rename_SOURCES = misc-utils/rename.c
+rename_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_GETOPT
 usrbin_exec_PROGRAMS += getopt
 dist_man_MANS += misc-utils/getopt.1
 getopt_SOURCES = misc-utils/getopt.c
+getopt_LDADD = $(LDADD) libcommon.la
 getoptexampledir = $(docdir)/getopt/
 dist_getoptexample_SCRIPTS = \
 	misc-utils/getopt-parse.bash \
diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
index 98e2cc29b..1b2277321 100644
--- a/sys-utils/Makemodule.am
+++ b/sys-utils/Makemodule.am
@@ -64,6 +64,7 @@ if BUILD_RENICE
 usrbin_exec_PROGRAMS += renice
 dist_man_MANS += sys-utils/renice.1
 renice_SOURCES = sys-utils/renice.c
+renice_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_RFKILL
@@ -78,12 +79,14 @@ if BUILD_SETSID
 usrbin_exec_PROGRAMS += setsid
 dist_man_MANS += sys-utils/setsid.1
 setsid_SOURCES = sys-utils/setsid.c
+setsid_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_READPROFILE
 usrsbin_exec_PROGRAMS += readprofile
 dist_man_MANS += sys-utils/readprofile.8
 readprofile_SOURCES = sys-utils/readprofile.c
+readprofile_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_TUNELP
@@ -132,6 +135,7 @@ if BUILD_FSFREEZE
 sbin_PROGRAMS += fsfreeze
 dist_man_MANS += sys-utils/fsfreeze.8
 fsfreeze_SOURCES = sys-utils/fsfreeze.c
+fsfreeze_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_BLKDISCARD
@@ -167,6 +171,7 @@ if BUILD_SETARCH
 usrbin_exec_PROGRAMS += setarch
 dist_man_MANS += sys-utils/setarch.8
 setarch_SOURCES = sys-utils/setarch.c
+setarch_LDADD = $(LDADD) libcommon.la
 
 SETARCH_LINKS = uname26 linux32 linux64
 
@@ -388,7 +393,7 @@ endif
 
 if BUILD_MOUNTPOINT
 bin_PROGRAMS += mountpoint
-mountpoint_LDADD = $(LDADD) libmount.la
+mountpoint_LDADD = $(LDADD) libcommon.la libmount.la
 mountpoint_CFLAGS = $(AM_CFLAGS) -I$(ul_libmount_incdir)
 dist_man_MANS += sys-utils/mountpoint.1
 mountpoint_SOURCES = sys-utils/mountpoint.c
@@ -405,12 +410,14 @@ if BUILD_PIVOT_ROOT
 sbin_PROGRAMS += pivot_root
 dist_man_MANS += sys-utils/pivot_root.8
 pivot_root_SOURCES = sys-utils/pivot_root.c
+pivot_root_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_SWITCH_ROOT
 sbin_PROGRAMS += switch_root
 dist_man_MANS += sys-utils/switch_root.8
 switch_root_SOURCES = sys-utils/switch_root.c
+switch_root_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_UNSHARE
diff --git a/text-utils/Makemodule.am b/text-utils/Makemodule.am
index 7478eb427..f77fc2d3e 100644
--- a/text-utils/Makemodule.am
+++ b/text-utils/Makemodule.am
@@ -9,6 +9,7 @@ if BUILD_COLCRT
 usrbin_exec_PROGRAMS += colcrt
 dist_man_MANS += text-utils/colcrt.1
 colcrt_SOURCES = text-utils/colcrt.c
+colcrt_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_COLRM
@@ -42,6 +43,7 @@ if BUILD_REV
 usrbin_exec_PROGRAMS += rev
 dist_man_MANS += text-utils/rev.1
 rev_SOURCES = text-utils/rev.c
+rev_LDADD = $(LDADD) libcommon.la
 endif
 
 if BUILD_LINE
@@ -64,7 +66,7 @@ usrbin_exec_PROGRAMS += ul
 dist_man_MANS += text-utils/ul.1
 ul_SOURCES = text-utils/ul.c
 ul_CFLAGS = $(AM_CFLAGS)
-ul_LDADD = $(LDADD)
+ul_LDADD = $(LDADD) libcommon.la
 if HAVE_TINFO
 ul_LDADD += $(TINFO_LIBS)
 ul_LDADD += $(TINFO_CFLAGS)
@@ -80,7 +82,7 @@ bin_PROGRAMS += more
 dist_man_MANS += text-utils/more.1
 more_SOURCES = text-utils/more.c
 more_CFLAGS = $(AM_CFLAGS) $(BSD_WARN_CFLAGS)
-more_LDADD = $(LDADD)
+more_LDADD = $(LDADD) libcommon.la
 if HAVE_TINFO
 more_LDADD += $(TINFO_LIBS)
 more_LDADD += $(TINFO_CFLAGS)
-- 
2.22.1

[PATCH 4/4] lib/closestream: fix assignment to read-only standard streams

[Patrick Steinhardt]

In order to avoid closing standard streams multiple times, commit
52aa1a661 (include/closestream: avoid close more than once, 2019-06-13)
started to set the standard output and error streams to `NULL`.
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.

Fix the issue by instead using a local static variable `streams_closed`
that gets set to `1` when the standard streams have been closed. This
unbreaks compilation on musl libc systems.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 lib/closestream.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/closestream.c b/lib/closestream.c
index d735e4f01..e5d04fe69 100644
--- a/lib/closestream.c
+++ b/lib/closestream.c
@@ -19,6 +19,8 @@
 # define CLOSE_EXIT_CODE EXIT_FAILURE
 #endif
 
+static int streams_closed = 0;
+
 int
 close_stream(FILE * stream)
 {
@@ -44,7 +46,7 @@ close_stream(FILE * stream)
 static inline void
 close_stdout(void)
 {
-	if (stdout && close_stream(stdout) != 0 && !(errno == EPIPE)) {
+	if (!streams_closed && close_stream(stdout) != 0 && !(errno == EPIPE)) {
 		if (errno)
 			warn(_("write error"));
 		else
@@ -52,11 +54,10 @@ close_stdout(void)
 		_exit(CLOSE_EXIT_CODE);
 	}
 
-	if (stderr && close_stream(stderr) != 0)
+	if (!streams_closed && close_stream(stderr) != 0)
 		_exit(CLOSE_EXIT_CODE);
 
-	stdout = NULL;
-	stderr = NULL;
+	streams_closed = 1;
 }
 
 void close_stdout_atexit(void)
-- 
2.22.1

Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system

[Karel Zak]

On Wed, Aug 14, 2019 at 06:45:03PM +0200, Patrick Steinhardt wrote:
> since commit 52aa1a661 (include/closestream: avoid close more
> than once, 2019-06-13), util-linux fails to build on musl libc
> based systems. The culprit here is that it introduced assignments
> to stderr and stdout, while the C89 standard explicitly notes
> that treating stderr and stdout as valid lvalues is not a
> requirement for any conforming C implementation. musl libc
> implemented these streams as `extern FILE *const`, and as a
> result assigning to these variables causes compiler errors.

The question is if close() for stdout and stderr is the right way to
go. 

In an ideal world it would be enough to use ferror()+fflush(),
unfortunately for example NFS has never reached an ideal world and it
requires fclose()... See

 https://lists.gnu.org/r/bug-gnulib/2019-04/msg00061.html

Florian (added to CC), also suggested to use dup3() for the
descriptors and then fclose() for the new handle. It sounds like a
pretty elegant solution how to avoid all the issues with NULL and it's
also robust enough if you accidentally call close_stream() more than
once.

See

 https://bugzilla.redhat.com/show_bug.cgi?id=1732450#c4

Maybe we can improve include/closestream.h to use dup3(), than it would
be possible keep all in the header file as inline functions. 

Comments?

> Attached is a fix for this. Instead of assigning `NULL` to the
> streams, util-linux now uses a static variable `streams_closed`.

I don't think we need to check if we already performed this operation
as it's always called only once by atexit() and with dup3() it will be
robust enough.

> Unfortunately, this fix necessitated some shifting around as
> closestream was previously implemented as header, only, and
> implementing static variables inside of a header is not going to
> work due to them being static to the single compilation unit,
> only. Thus I converted the code to move the implementation into
> "lib/closestream.c".

 Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]

On Mon, Aug 19, 2019 at 03:36:19PM +0200, Karel Zak wrote:
> On Wed, Aug 14, 2019 at 06:45:03PM +0200, Patrick Steinhardt wrote:
> > since commit 52aa1a661 (include/closestream: avoid close more
> > than once, 2019-06-13), util-linux fails to build on musl libc
> > based systems. The culprit here is that it introduced assignments
> > to stderr and stdout, while the C89 standard explicitly notes
> > that treating stderr and stdout as valid lvalues is not a
> > requirement for any conforming C implementation. musl libc
> > implemented these streams as `extern FILE *const`, and as a
> > result assigning to these variables causes compiler errors.
> 
> The question is if close() for stdout and stderr is the right way to
> go. 
> 
> In an ideal world it would be enough to use ferror()+fflush(),
> unfortunately for example NFS has never reached an ideal world and it
> requires fclose()... See
> 
>  https://lists.gnu.org/r/bug-gnulib/2019-04/msg00061.html
> 
> Florian (added to CC), also suggested to use dup3() for the
> descriptors and then fclose() for the new handle. It sounds like a
> pretty elegant solution how to avoid all the issues with NULL and it's
> also robust enough if you accidentally call close_stream() more than
> once.
> 
> See
> 
>  https://bugzilla.redhat.com/show_bug.cgi?id=1732450#c4
> 
> Maybe we can improve include/closestream.h to use dup3(), than it would
> be possible keep all in the header file as inline functions. 
> 
> Comments?

I honestly don't get why we'd need to use dup3(2), though.
Couldn't the same be achieved by using dup2(3P) followed by
close(3P), instead? Especially considering that the former one
isn't specified by POSIX, either, but the latter one is.

That being said, if we agree on a proper fix here then I'd be
happy to post a v2.

Regards
Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system

[Karel Zak]

On Tue, Aug 20, 2019 at 03:17:42PM +0200, Patrick Steinhardt wrote:
> On Mon, Aug 19, 2019 at 03:36:19PM +0200, Karel Zak wrote:
> > On Wed, Aug 14, 2019 at 06:45:03PM +0200, Patrick Steinhardt wrote:
> > > since commit 52aa1a661 (include/closestream: avoid close more
> > > than once, 2019-06-13), util-linux fails to build on musl libc
> > > based systems. The culprit here is that it introduced assignments
> > > to stderr and stdout, while the C89 standard explicitly notes
> > > that treating stderr and stdout as valid lvalues is not a
> > > requirement for any conforming C implementation. musl libc
> > > implemented these streams as `extern FILE *const`, and as a
> > > result assigning to these variables causes compiler errors.
> > 
> > The question is if close() for stdout and stderr is the right way to
> > go. 
> > 
> > In an ideal world it would be enough to use ferror()+fflush(),
> > unfortunately for example NFS has never reached an ideal world and it
> > requires fclose()... See
> > 
> >  https://lists.gnu.org/r/bug-gnulib/2019-04/msg00061.html
> > 
> > Florian (added to CC), also suggested to use dup3() for the
> > descriptors and then fclose() for the new handle. It sounds like a
> > pretty elegant solution how to avoid all the issues with NULL and it's
> > also robust enough if you accidentally call close_stream() more than
> > once.
> > 
> > See
> > 
> >  https://bugzilla.redhat.com/show_bug.cgi?id=1732450#c4
> > 
> > Maybe we can improve include/closestream.h to use dup3(), than it would
> > be possible keep all in the header file as inline functions. 
> > 
> > Comments?
> 
> I honestly don't get why we'd need to use dup3(2), though.
> Couldn't the same be achieved by using dup2(3P) followed by
> close(3P), instead? Especially considering that the former one
> isn't specified by POSIX, either, but the latter one is.

Well, I guess Florian has used dup3() as example and I don't think we
have to care about the flags (O_CLOEXEC)  as we close the descriptor
in the same function. The important thing is to have descriptor which we
can close to force filesystems to sync stuff :-)

> That being said, if we agree on a proper fix here then I'd be
> happy to post a v2.

Go ahead.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system

[Florian Weimer]

* Karel Zak:

> On Tue, Aug 20, 2019 at 03:17:42PM +0200, Patrick Steinhardt wrote:
>> On Mon, Aug 19, 2019 at 03:36:19PM +0200, Karel Zak wrote:
>> > On Wed, Aug 14, 2019 at 06:45:03PM +0200, Patrick Steinhardt wrote:
>> > > since commit 52aa1a661 (include/closestream: avoid close more
>> > > than once, 2019-06-13), util-linux fails to build on musl libc
>> > > based systems. The culprit here is that it introduced assignments
>> > > to stderr and stdout, while the C89 standard explicitly notes
>> > > that treating stderr and stdout as valid lvalues is not a
>> > > requirement for any conforming C implementation. musl libc
>> > > implemented these streams as `extern FILE *const`, and as a
>> > > result assigning to these variables causes compiler errors.
>> > 
>> > The question is if close() for stdout and stderr is the right way to
>> > go. 
>> > 
>> > In an ideal world it would be enough to use ferror()+fflush(),
>> > unfortunately for example NFS has never reached an ideal world and it
>> > requires fclose()... See
>> > 
>> >  https://lists.gnu.org/r/bug-gnulib/2019-04/msg00061.html
>> > 
>> > Florian (added to CC), also suggested to use dup3() for the
>> > descriptors and then fclose() for the new handle. It sounds like a
>> > pretty elegant solution how to avoid all the issues with NULL and it's
>> > also robust enough if you accidentally call close_stream() more than
>> > once.
>> > 
>> > See
>> > 
>> >  https://bugzilla.redhat.com/show_bug.cgi?id=1732450#c4
>> > 
>> > Maybe we can improve include/closestream.h to use dup3(), than it would
>> > be possible keep all in the header file as inline functions. 
>> > 
>> > Comments?
>> 
>> I honestly don't get why we'd need to use dup3(2), though.
>> Couldn't the same be achieved by using dup2(3P) followed by
>> close(3P), instead? Especially considering that the former one
>> isn't specified by POSIX, either, but the latter one is.
>
> Well, I guess Florian has used dup3() as example and I don't think we
> have to care about the flags (O_CLOEXEC)  as we close the descriptor
> in the same function. The important thing is to have descriptor which we
> can close to force filesystems to sync stuff :-)

Even in that case, dup3 would still be relevant to multi-threaded
programs.  I don't know about the context, it may not be relevant to
util-linux.

Thanks,
Florian

[PATCH v2] include/closestream: fix assignment to read-only standard streams

[Patrick Steinhardt]

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

Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system

[Karel Zak]

On Tue, Aug 20, 2019 at 05:04:39PM +0200, Karel Zak wrote:
> On Tue, Aug 20, 2019 at 03:17:42PM +0200, Patrick Steinhardt wrote:
> > That being said, if we agree on a proper fix here then I'd be
> > happy to post a v2.
> 
> Go ahead.

Note that Bernhard Voelker sent me (off list, but thanks;-) another to
link the original coreutils discussion:

 https://lists.gnu.org/archive/html/bug-gnulib/2019-05/msg00156.html

where is something about close() and dup(). My note:

It seems there is no ideal solution which is portable and reliable
on all platforms (kernels).  

The current solution with std{out,err} close is also not elegant as the 
close makes it useless with things like ASAN or another built-in debuggers,
and it's incompatible with some libc.

The question is what does it mean for util-linux. I don't think we
need 100% reliability on all platforms; the primary target is Linux,
commonly used filesystems, standard use-cases and code (=!behavior)
portability -- everything else is bonus. From this point of view
dup()+close() sounds like better than the current solution.

From my point of view all this game with the streams is just a way how
to detect some basic users' mistakes. I don't think we can detect
serious I/O errors without fsync(), and close() itself does not
guarantee anything. So, it does not make sense trying to make it
super durable, reliable and portable to all operation systems if at
the end you rely on poor close() ...

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH v2] include/closestream: fix assignment to read-only standard streams

[Karel Zak]

On Thu, Aug 22, 2019 at 11:40:15AM +0200, Patrick Steinhardt wrote:
> 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.

Yes, it would be nice to rename it later to something like
"flush_stdstreams" or "verify_stdstreams" or so...

>  include/closestream.h | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)

Looks good. I'll apply the patch later (let's wait for another reviews).

And it seems we can remove 

    #if !defined(__SANITIZE_ADDRESS__)

from the code now.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH v2] include/closestream: fix assignment to read-only standard streams

[Karel Zak]

On Thu, Aug 22, 2019 at 11:40:15AM +0200, Patrick Steinhardt wrote:
> 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.

Applied, thanks. We'll see... :-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com