Fix closing of standard text streams for non-glibc system

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

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.

---
 term-utils/ttymsg.c | 1 +
 1 file changed, 1 insertion(+)

index 2aab69f10..04c867e95 100644
@@ -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

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.

---
 login-utils/islocal.c | 1 +
 1 file changed, 1 insertion(+)

index 469bc5629..971e9626f 100644
@@ -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

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.

---
 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%)

index bea0ed6a6..1676e40fd 100644
@@ -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
 
 

index 862a06c17..056311a40 100644
@@ -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 \

similarity index 86%
copy from include/closestream.h
copy to lib/closestream.c
index 83df1ee7d..d735e4f01 100644
@@ -1,5 +1,9 @@
-#ifndef UTIL_LINUX_CLOSESTREAM_H
-#define UTIL_LINUX_CLOSESTREAM_H
+/*
+ * Copyright (C) 2019 Patrick Steinhardt <[email protected]>
+ *
+ * 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 */

index aafbea307..740cc0721 100644
@@ -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:

index f56a819ac..34ee3e905 100644
@@ -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 \

index 98e2cc29b..1b2277321 100644
@@ -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

index 7478eb427..f77fc2d3e 100644
@@ -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

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.

---
 lib/closestream.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

index d735e4f01..e5d04fe69 100644
@@ -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

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 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.

Karel

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

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

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 :-)

Go ahead.

Karel

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

* Karel Zak:

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

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  <[email protected]>
 http://karelzak.blogspot.com

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.

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

index 83df1ee7d..41afbe208 100644
@@ -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

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

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  <[email protected]>
 http://karelzak.blogspot.com

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

Karel

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com