* [PATCH 1/4] term-utils/ttymsg: fix missing header for ARRAY_SIZE macro
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 ` Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 2/4] login-utils/islocal: fix missing header for err macro Patrick Steinhardt
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Patrick Steinhardt @ 2019-08-14 16:45 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] login-utils/islocal: fix missing header for err macro
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 ` Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 3/4] lib/closestream: move implementation into its own compilation unit Patrick Steinhardt
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Patrick Steinhardt @ 2019-08-14 16:45 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] lib/closestream: move implementation into its own compilation unit
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 ` Patrick Steinhardt
2019-08-14 16:45 ` [PATCH 4/4] lib/closestream: fix assignment to read-only standard streams Patrick Steinhardt
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Patrick Steinhardt @ 2019-08-14 16:45 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] lib/closestream: fix assignment to read-only standard streams
2019-08-14 16:45 [PATCH 0/4] Fix closing of standard text streams for non-glibc system Patrick Steinhardt
` (2 preceding siblings ...)
2019-08-14 16:45 ` [PATCH 3/4] lib/closestream: move implementation into its own compilation unit Patrick Steinhardt
@ 2019-08-14 16:45 ` Patrick Steinhardt
2019-08-19 13:36 ` [PATCH 0/4] Fix closing of standard text streams for non-glibc system Karel Zak
2019-08-22 9:40 ` [PATCH v2] include/closestream: fix assignment to read-only standard streams Patrick Steinhardt
5 siblings, 0 replies; 13+ messages in thread
From: Patrick Steinhardt @ 2019-08-14 16:45 UTC (permalink / raw)
To: util-linux; +Cc: 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system
2019-08-14 16:45 [PATCH 0/4] Fix closing of standard text streams for non-glibc system Patrick Steinhardt
` (3 preceding siblings ...)
2019-08-14 16:45 ` [PATCH 4/4] lib/closestream: fix assignment to read-only standard streams Patrick Steinhardt
@ 2019-08-19 13:36 ` Karel Zak
2019-08-20 13:17 ` Patrick Steinhardt
2019-08-22 9:40 ` [PATCH v2] include/closestream: fix assignment to read-only standard streams Patrick Steinhardt
5 siblings, 1 reply; 13+ messages in thread
From: Karel Zak @ 2019-08-19 13:36 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: util-linux, Florian Weimer
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system
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
0 siblings, 1 reply; 13+ messages in thread
From: Patrick Steinhardt @ 2019-08-20 13:17 UTC (permalink / raw)
To: Karel Zak; +Cc: util-linux, Florian Weimer
[-- 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 --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system
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
0 siblings, 2 replies; 13+ messages in thread
From: Karel Zak @ 2019-08-20 15:04 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: util-linux, Florian Weimer
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system
2019-08-20 15:04 ` Karel Zak
@ 2019-08-20 15:11 ` Florian Weimer
2019-08-23 11:52 ` Karel Zak
1 sibling, 0 replies; 13+ messages in thread
From: Florian Weimer @ 2019-08-20 15:11 UTC (permalink / raw)
To: Karel Zak; +Cc: Patrick Steinhardt, util-linux
* 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/4] Fix closing of standard text streams for non-glibc system
2019-08-20 15:04 ` Karel Zak
2019-08-20 15:11 ` Florian Weimer
@ 2019-08-23 11:52 ` Karel Zak
1 sibling, 0 replies; 13+ messages in thread
From: Karel Zak @ 2019-08-23 11:52 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: util-linux, Florian Weimer, Bernhard Voelker
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] include/closestream: fix assignment to read-only standard streams
2019-08-14 16:45 [PATCH 0/4] Fix closing of standard text streams for non-glibc system Patrick Steinhardt
` (4 preceding siblings ...)
2019-08-19 13:36 ` [PATCH 0/4] Fix closing of standard text streams for non-glibc system Karel Zak
@ 2019-08-22 9:40 ` Patrick Steinhardt
2019-08-23 12:00 ` Karel Zak
2019-09-02 10:01 ` Karel Zak
5 siblings, 2 replies; 13+ messages in thread
From: Patrick Steinhardt @ 2019-08-22 9:40 UTC (permalink / raw)
To: util-linux; +Cc: Patrick Steinhardt, Karel Zak, Florian Weimer
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] include/closestream: fix assignment to read-only standard streams
2019-08-22 9:40 ` [PATCH v2] include/closestream: fix assignment to read-only standard streams Patrick Steinhardt
@ 2019-08-23 12:00 ` Karel Zak
2019-09-02 10:01 ` Karel Zak
1 sibling, 0 replies; 13+ messages in thread
From: Karel Zak @ 2019-08-23 12:00 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: util-linux, Florian Weimer
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] include/closestream: fix assignment to read-only standard streams
2019-08-22 9:40 ` [PATCH v2] include/closestream: fix assignment to read-only standard streams Patrick Steinhardt
2019-08-23 12:00 ` Karel Zak
@ 2019-09-02 10:01 ` Karel Zak
1 sibling, 0 replies; 13+ messages in thread
From: Karel Zak @ 2019-09-02 10:01 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: util-linux, Florian Weimer
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
^ permalink raw reply [flat|nested] 13+ messages in thread