* portability of fopen and 'e' (O_CLOEXEC) flag @ 2020-05-11 14:49 Tim Rühsen 2020-05-11 16:37 ` Bruno Haible 0 siblings, 1 reply; 14+ messages in thread From: Tim Rühsen @ 2020-05-11 14:49 UTC (permalink / raw) To: bug-gnulib [-- Attachment #1.1: Type: text/plain, Size: 405 bytes --] Hi, i would like to ask for your expert knowledge. How to prevent file descriptor leaks in a multi-threaded application that fork+exec. Quick answer is surely "use O_CLOEXEC" to close those file descriptors on exec. But how does this work with fopen in a portable way ? GNU libc has the 'e' flag for exactly this. How about other non-GNU OSes / alternative C libraries ? Regards, Tim [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-11 14:49 portability of fopen and 'e' (O_CLOEXEC) flag Tim Rühsen @ 2020-05-11 16:37 ` Bruno Haible 2020-05-11 17:41 ` Eric Blake 2020-05-12 8:28 ` Tim Rühsen 0 siblings, 2 replies; 14+ messages in thread From: Bruno Haible @ 2020-05-11 16:37 UTC (permalink / raw) To: bug-gnulib; +Cc: Tim Rühsen Hi Tim, > i would like to ask for your expert knowledge. > > How to prevent file descriptor leaks in a multi-threaded application > that fork+exec. Quick answer is surely "use O_CLOEXEC" to close those > file descriptors on exec. > > But how does this work with fopen in a portable way ? > GNU libc has the 'e' flag for exactly this. Yes [1]. > How about other non-GNU OSes / alternative C libraries ? POSIX [2][3], macOS [4], FreeBSD [5], Solaris [6] don't support this 'e' flag. How about using open() with O_CLOEXEC, and then fdopen()? Bruno [1] http://man7.org/linux/man-pages/man3/fopen.3.html [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html [3] http://man7.org/linux/man-pages/man3/fopen.3p.html [4] https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fopen.3.html [5] https://www.freebsd.org/cgi/man.cgi?query=fopen&sektion=3&apropos=0&manpath=freebsd [6] https://docs.oracle.com/cd/E36784_01/html/E36874/fopen-3c.html ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-11 16:37 ` Bruno Haible @ 2020-05-11 17:41 ` Eric Blake 2020-05-12 8:28 ` Tim Rühsen 1 sibling, 0 replies; 14+ messages in thread From: Eric Blake @ 2020-05-11 17:41 UTC (permalink / raw) To: Bruno Haible, bug-gnulib; +Cc: Tim Rühsen On 5/11/20 11:37 AM, Bruno Haible wrote: > > POSIX [2][3], macOS [4], FreeBSD [5], Solaris [6] don't support this 'e' flag. Although it _is_ slated to be added in a future revision of POSIX: https://www.austingroupbugs.net/view.php?id=411 > > How about using open() with O_CLOEXEC, and then fdopen()? In the meantime, this is indeed more portable. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-11 16:37 ` Bruno Haible 2020-05-11 17:41 ` Eric Blake @ 2020-05-12 8:28 ` Tim Rühsen 2020-05-24 19:22 ` Bruno Haible 1 sibling, 1 reply; 14+ messages in thread From: Tim Rühsen @ 2020-05-12 8:28 UTC (permalink / raw) To: Bruno Haible, bug-gnulib [-- Attachment #1.1: Type: text/plain, Size: 1474 bytes --] Hi Bruno, On 11.05.20 18:37, Bruno Haible wrote: > Hi Tim, > >> i would like to ask for your expert knowledge. >> >> How to prevent file descriptor leaks in a multi-threaded application >> that fork+exec. Quick answer is surely "use O_CLOEXEC" to close those >> file descriptors on exec. >> >> But how does this work with fopen in a portable way ? >> GNU libc has the 'e' flag for exactly this. > > Yes [1]. > >> How about other non-GNU OSes / alternative C libraries ? > > POSIX [2][3], macOS [4], FreeBSD [5], Solaris [6] don't support this 'e' flag. > > How about using open() with O_CLOEXEC, and then fdopen()? Thanks. Sure, this is possible. Doing so at dozens of places (or more) asks for an implementation in gnulib :) Several projects (most library code) could benefit. I currently think about GnuTLS where we have to fix this in one or the other way. Updating gnulib and adding 'e' to the fopen modes would be straight forward. > > Bruno > > [1] http://man7.org/linux/man-pages/man3/fopen.3.html > [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html > [3] http://man7.org/linux/man-pages/man3/fopen.3p.html > [4] https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fopen.3.html > [5] https://www.freebsd.org/cgi/man.cgi?query=fopen&sektion=3&apropos=0&manpath=freebsd > [6] https://docs.oracle.com/cd/E36784_01/html/E36874/fopen-3c.html > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-12 8:28 ` Tim Rühsen @ 2020-05-24 19:22 ` Bruno Haible 2020-05-26 6:01 ` Daiki Ueno 0 siblings, 1 reply; 14+ messages in thread From: Bruno Haible @ 2020-05-24 19:22 UTC (permalink / raw) To: Tim Rühsen; +Cc: bug-gnulib [-- Attachment #1: Type: text/plain, Size: 1179 bytes --] Tim Rühsen wrote on 2020-05-12: > > How about using open() with O_CLOEXEC, and then fdopen()? > > Thanks. Sure, this is possible. Doing so at dozens of places (or more) > asks for an implementation in gnulib :) Additionally, gnulib is the right place to do this because - as Eric said - the 'e' flag is already scheduled for being added to the next POSIX version: https://www.austingroupbugs.net/view.php?id=411 My evaluation of current platform support was incorrect: Current versions of FreeBSD, NetBSD, OpenBSD, Solaris, Cygwin, and even Minix already support it. 2020-05-24 Bruno Haible <bruno@clisp.org> fopen-gnu: Add tests. * tests/test-fopen-gnu.c: New file. * modules/fopen-gnu-tests: New file. fopen-gnu: New module. Suggested by Tim Rühsen <tim.ruehsen@gmx.de> in <https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00119.html>. * lib/fopen.c (rpl_fopen): When the fopen-gnu module is enabled and the mode contains an 'x' or 'e' flag, use open() followed by fdopen(). * m4/fopen.m4 (gl_FUNC_FOPEN_GNU): New macro. * modules/fopen-gnu: New file. * doc/posix-functions/fopen.texi: Document the 'fopen-gnu' module. [-- Attachment #2: 0001-fopen-gnu-New-module.patch --] [-- Type: text/x-patch, Size: 11027 bytes --] From 10cb4be2a2114dd6fff347acc9841d7904636adf Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Sun, 24 May 2020 20:38:53 +0200 Subject: [PATCH 1/2] fopen-gnu: New module. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Suggested by Tim Rühsen <tim.ruehsen@gmx.de> in <https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00119.html>. * lib/fopen.c (rpl_fopen): When the fopen-gnu module is enabled and the mode contains an 'x' or 'e' flag, use open() followed by fdopen(). * m4/fopen.m4 (gl_FUNC_FOPEN_GNU): New macro. * modules/fopen-gnu: New file. * doc/posix-functions/fopen.texi: Document the 'fopen-gnu' module. --- ChangeLog | 11 +++++ doc/posix-functions/fopen.texi | 19 ++++++++- lib/fopen.c | 92 ++++++++++++++++++++++++++++++++++++++++-- m4/fopen.m4 | 87 ++++++++++++++++++++++++++++++++++++++- modules/fopen-gnu | 27 +++++++++++++ 5 files changed, 229 insertions(+), 7 deletions(-) create mode 100644 modules/fopen-gnu diff --git a/ChangeLog b/ChangeLog index ae9fae4..fab0d87 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,16 @@ 2020-05-24 Bruno Haible <bruno@clisp.org> + fopen-gnu: New module. + Suggested by Tim Rühsen <tim.ruehsen@gmx.de> in + <https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00119.html>. + * lib/fopen.c (rpl_fopen): When the fopen-gnu module is enabled and the + mode contains an 'x' or 'e' flag, use open() followed by fdopen(). + * m4/fopen.m4 (gl_FUNC_FOPEN_GNU): New macro. + * modules/fopen-gnu: New file. + * doc/posix-functions/fopen.texi: Document the 'fopen-gnu' module. + +2020-05-24 Bruno Haible <bruno@clisp.org> + open, openat: Really support O_CLOEXEC. * lib/open.c (open): When have_cloexec is still undecided, do pass a O_CLOEXEC flag to orig_open. diff --git a/doc/posix-functions/fopen.texi b/doc/posix-functions/fopen.texi index 308d676..6c562a6 100644 --- a/doc/posix-functions/fopen.texi +++ b/doc/posix-functions/fopen.texi @@ -4,9 +4,9 @@ POSIX specification:@* @url{https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html} -Gnulib module: fopen +Gnulib module: fopen or fopen-gnu -Portability problems fixed by Gnulib: +Portability problems fixed by either Gnulib module @code{fopen} or @code{fopen-gnu}: @itemize @item This function does not fail when the file name argument ends in a slash @@ -21,6 +21,21 @@ On Windows platforms (excluding Cygwin), this function does usually not recognize the @file{/dev/null} filename. @end itemize +Portability problems fixed by Gnulib module @code{fopen-gnu}: +@itemize +@item +This function does not support the mode character +@samp{x} (corresponding to @code{O_EXCL}), introduced in ISO C11, +on some platforms: +FreeBSD 8.2, NetBSD 6.1, OpenBSD 5.6, Minix 3.2, AIX 6.1, HP-UX 11.31, IRIX 6.5, Solaris 11.3, Cygwin 1.7.16 (2012), mingw, MSVC 14. +@item +This function does not support the mode character +@samp{e} (corresponding to @code{O_CLOEXEC}), +introduced into a future POSIX revision through +@url{https://www.austingroupbugs.net/view.php?id=411}, on some platforms: +glibc 2.6, Mac OS X 10.13, FreeBSD 9.0, NetBSD 5.1, OpenBSD 5.6, Minix 3.2, AIX 7.2, HP-UX 11.31, IRIX 6.5, Solaris 11.3, Cygwin 1.7.16 (2012), mingw, MSVC 14. +@end itemize + Portability problems not fixed by Gnulib: @itemize @item diff --git a/lib/fopen.c b/lib/fopen.c index ad6511d..20065e4 100644 --- a/lib/fopen.c +++ b/lib/fopen.c @@ -49,6 +49,12 @@ rpl_fopen (const char *filename, const char *mode) { int open_direction; int open_flags_standard; +#if GNULIB_FOPEN_GNU + int open_flags_gnu; +# define BUF_SIZE 80 + char fdopen_mode_buf[BUF_SIZE + 1]; +#endif + int open_flags; #if defined _WIN32 && ! defined __CYGWIN__ if (strcmp (filename, "/dev/null") == 0) @@ -58,35 +64,88 @@ rpl_fopen (const char *filename, const char *mode) /* Parse the mode. */ open_direction = 0; open_flags_standard = 0; +#if GNULIB_FOPEN_GNU + open_flags_gnu = 0; +#endif { - const char *m; + const char *p = mode; +#if GNULIB_FOPEN_GNU + char *q = fdopen_mode_buf; +#endif - for (m = mode; *m != '\0'; m++) + for (; *p != '\0'; p++) { - switch (*m) + switch (*p) { case 'r': open_direction = O_RDONLY; +#if GNULIB_FOPEN_GNU + if (q < fdopen_mode_buf + BUF_SIZE) + *q++ = *p; +#endif continue; case 'w': open_direction = O_WRONLY; open_flags_standard |= O_CREAT | O_TRUNC; +#if GNULIB_FOPEN_GNU + if (q < fdopen_mode_buf + BUF_SIZE) + *q++ = *p; +#endif continue; case 'a': open_direction = O_WRONLY; open_flags_standard |= O_CREAT | O_APPEND; +#if GNULIB_FOPEN_GNU + if (q < fdopen_mode_buf + BUF_SIZE) + *q++ = *p; +#endif continue; case 'b': +#if GNULIB_FOPEN_GNU + if (q < fdopen_mode_buf + BUF_SIZE) + *q++ = *p; +#endif continue; case '+': open_direction = O_RDWR; +#if GNULIB_FOPEN_GNU + if (q < fdopen_mode_buf + BUF_SIZE) + *q++ = *p; +#endif continue; +#if GNULIB_FOPEN_GNU + case 'x': + open_flags_gnu |= O_EXCL; + continue; + case 'e': + open_flags_gnu |= O_CLOEXEC; + continue; +#endif default: break; } +#if GNULIB_FOPEN_GNU + /* The rest of the mode string can be a platform-dependent extension. + Copy it unmodified. */ + { + size_t len = strlen (p); + if (len > fdopen_mode_buf + BUF_SIZE - q) + len = fdopen_mode_buf + BUF_SIZE - q; + memcpy (q, p, len); + q += len; + } +#endif break; } +#if GNULIB_FOPEN_GNU + *q = '\0'; +#endif } +#if GNULIB_FOPEN_GNU + open_flags = open_flags_standard | open_flags_gnu; +#else + open_flags = open_flags_standard; +#endif #if FOPEN_TRAILING_SLASH_BUG /* Fail if the mode requires write access and the filename ends in a slash, @@ -116,7 +175,7 @@ rpl_fopen (const char *filename, const char *mode) return NULL; } - fd = open (filename, open_direction | open_flags_standard); + fd = open (filename, open_direction | open_flags); if (fd < 0) return NULL; @@ -127,7 +186,11 @@ rpl_fopen (const char *filename, const char *mode) return NULL; } +# if GNULIB_FOPEN_GNU + fp = fdopen (fd, fdopen_mode_buf); +# else fp = fdopen (fd, mode); +# endif if (fp == NULL) { int saved_errno = errno; @@ -139,5 +202,26 @@ rpl_fopen (const char *filename, const char *mode) } #endif +#if GNULIB_FOPEN_GNU + if (open_flags_gnu != 0) + { + int fd; + FILE *fp; + + fd = open (filename, open_direction | open_flags); + if (fd < 0) + return NULL; + + fp = fdopen (fd, fdopen_mode_buf); + if (fp == NULL) + { + int saved_errno = errno; + close (fd); + errno = saved_errno; + } + return fp; + } +#endif + return orig_fopen (filename, mode); } diff --git a/m4/fopen.m4 b/m4/fopen.m4 index 2a4b00d..8eab4a6 100644 --- a/m4/fopen.m4 +++ b/m4/fopen.m4 @@ -1,4 +1,4 @@ -# fopen.m4 serial 10 +# fopen.m4 serial 11 dnl Copyright (C) 2007-2020 Free Software Foundation, Inc. dnl This file is free software; the Free Software Foundation dnl gives unlimited permission to copy and/or distribute it, @@ -58,5 +58,90 @@ changequote([,])dnl esac ]) +AC_DEFUN([gl_FUNC_FOPEN_GNU], +[ + AC_REQUIRE([gl_FUNC_FOPEN]) + AC_CACHE_CHECK([whether fopen supports the mode character 'x'], + [gl_cv_func_fopen_mode_x], + [rm -f conftest.x + AC_RUN_IFELSE( + [AC_LANG_SOURCE([[ +#include <stdio.h> +#include <errno.h> +int main () +{ + FILE *fp; + fp = fopen ("conftest.x", "w"); + fclose (fp); + fp = fopen ("conftest.x", "wx"); + if (fp != NULL) + /* 'x' ignored */ + return 1; + else if (errno == EEXIST) + return 0; + else + /* 'x' rejected */ + return 2; +}]])], + [gl_cv_func_fopen_mode_x=yes], + [gl_cv_func_fopen_mode_x=no], + [case "$host_os" in + # Guess yes on glibc and musl systems. + linux*-gnu* | gnu* | kfreebsd*-gnu | *-musl*) + gl_cv_func_fopen_mode_x="guessing yes" ;; + # If we don't know, obey --enable-cross-guesses. + *) + gl_cv_func_fopen_mode_x="$gl_cross_guess_normal" ;; + esac + ]) + rm -f conftest.x + ]) + AC_CACHE_CHECK([whether fopen supports the mode character 'e'], + [gl_cv_func_fopen_mode_e], + [echo foo > conftest.x + AC_RUN_IFELSE( + [AC_LANG_SOURCE([[ +#include <stdio.h> +#include <errno.h> +#include <fcntl.h> +int main () +{ + FILE *fp = fopen ("conftest.x", "re"); + if (fp != NULL) + { + if (fcntl (fileno (fp), F_GETFD) & FD_CLOEXEC) + return 0; + else + /* 'e' ignored */ + return 1; + } + else + /* 'e' rejected */ + return 2; +}]])], + [gl_cv_func_fopen_mode_e=yes], + [gl_cv_func_fopen_mode_e=no], + [case "$host_os" in + # Guess yes on glibc and musl systems. + linux*-gnu* | gnu* | kfreebsd*-gnu | *-musl*) + gl_cv_func_fopen_mode_e="guessing yes" ;; + # Guess no on native Windows. + mingw*) + gl_cv_func_fopen_mode_e="guessing no" ;; + # If we don't know, obey --enable-cross-guesses. + *) + gl_cv_func_fopen_mode_e="$gl_cross_guess_normal" ;; + esac + ]) + rm -f conftest.x + ]) + case "$gl_cv_func_fopen_mode_x" in + *no) REPLACE_FOPEN=1 ;; + esac + case "$gl_cv_func_fopen_mode_e" in + *no) REPLACE_FOPEN=1 ;; + esac +]) + # Prerequisites of lib/fopen.c. AC_DEFUN([gl_PREREQ_FOPEN], [:]) diff --git a/modules/fopen-gnu b/modules/fopen-gnu new file mode 100644 index 0000000..f0f2054 --- /dev/null +++ b/modules/fopen-gnu @@ -0,0 +1,27 @@ +Description: +fopen() function: open a stream to a file, with GNU extensions. + +Files: + +Depends-on: +fopen +open [test $REPLACE_FOPEN = 1] + +configure.ac: +gl_FUNC_FOPEN_GNU +if test $REPLACE_FOPEN = 1; then + AC_LIBOBJ([fopen]) + gl_PREREQ_FOPEN +fi +gl_MODULE_INDICATOR([fopen-gnu]) + +Makefile.am: + +Include: +<stdio.h> + +License: +LGPLv2+ + +Maintainer: +all -- 2.7.4 [-- Attachment #3: 0002-fopen-gnu-Add-tests.patch --] [-- Type: text/x-patch, Size: 3446 bytes --] From 48888b25625ddcdcc296e4cc1b0cd13e417e54f0 Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Sun, 24 May 2020 20:40:01 +0200 Subject: [PATCH 2/2] fopen-gnu: Add tests. * tests/test-fopen-gnu.c: New file. * modules/fopen-gnu-tests: New file. --- ChangeLog | 4 +++ modules/fopen-gnu-tests | 12 +++++++++ tests/test-fopen-gnu.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 modules/fopen-gnu-tests create mode 100644 tests/test-fopen-gnu.c diff --git a/ChangeLog b/ChangeLog index fab0d87..d7c1e8d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2020-05-24 Bruno Haible <bruno@clisp.org> + fopen-gnu: Add tests. + * tests/test-fopen-gnu.c: New file. + * modules/fopen-gnu-tests: New file. + fopen-gnu: New module. Suggested by Tim Rühsen <tim.ruehsen@gmx.de> in <https://lists.gnu.org/archive/html/bug-gnulib/2020-05/msg00119.html>. diff --git a/modules/fopen-gnu-tests b/modules/fopen-gnu-tests new file mode 100644 index 0000000..9e4c4ca --- /dev/null +++ b/modules/fopen-gnu-tests @@ -0,0 +1,12 @@ +Files: +tests/test-fopen-gnu.c +tests/macros.h + +Depends-on: +fcntl + +configure.ac: + +Makefile.am: +TESTS += test-fopen-gnu +check_PROGRAMS += test-fopen-gnu diff --git a/tests/test-fopen-gnu.c b/tests/test-fopen-gnu.c new file mode 100644 index 0000000..cae4042 --- /dev/null +++ b/tests/test-fopen-gnu.c @@ -0,0 +1,71 @@ +/* Test of opening a file stream. + Copyright (C) 2020 Free Software Foundation, Inc. + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <https://www.gnu.org/licenses/>. */ + +/* Written by Bruno Haible <bruno@clisp.org>, 2020. */ + +#include <config.h> + +/* Specification. */ +#include <stdio.h> + +#include <errno.h> +#include <fcntl.h> +#include <unistd.h> + +#include "macros.h" + +#define BASE "test-fopen-gnu.t" + +int +main (void) +{ + FILE *f; + int fd; + int flags; + + /* Remove anything from prior partial run. */ + unlink (BASE "file"); + + /* Create the file. */ + f = fopen (BASE "file", "w"); + ASSERT (f); + fd = fileno (f); + ASSERT (fd >= 0); + flags = fcntl (fd, F_GETFD); + ASSERT (flags >= 0); + ASSERT ((flags & FD_CLOEXEC) == 0); + ASSERT (fclose (f) == 0); + + /* Create the file and check the 'e' mode. */ + f = fopen (BASE "file", "we"); + ASSERT (f); + fd = fileno (f); + ASSERT (fd >= 0); + flags = fcntl (fd, F_GETFD); + ASSERT (flags >= 0); + ASSERT ((flags & FD_CLOEXEC) != 0); + ASSERT (fclose (f) == 0); + + /* Open the file and check the 'x' mode. */ + f = fopen (BASE "file", "ax"); + ASSERT (f == NULL); + ASSERT (errno == EEXIST); + + /* Cleanup. */ + ASSERT (unlink (BASE "file") == 0); + + return 0; +} -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-24 19:22 ` Bruno Haible @ 2020-05-26 6:01 ` Daiki Ueno 2020-05-26 8:18 ` Bruno Haible 0 siblings, 1 reply; 14+ messages in thread From: Daiki Ueno @ 2020-05-26 6:01 UTC (permalink / raw) To: Bruno Haible; +Cc: Tim Rühsen, bug-gnulib [-- Attachment #1: Type: text/plain, Size: 539 bytes --] Hello Bruno, Bruno Haible <bruno@clisp.org> writes: > Additionally, gnulib is the right place to do this because - as Eric said - > the 'e' flag is already scheduled for being added to the next POSIX version: > https://www.austingroupbugs.net/view.php?id=411 > > My evaluation of current platform support was incorrect: Current versions of > FreeBSD, NetBSD, OpenBSD, Solaris, Cygwin, and even Minix already support it. Thank you for this; would it make sense to use it in the modules that do one-shot fopen/fclose, such as read-file? [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-read-file-make-use-of-fopen-gnu.patch --] [-- Type: text/x-patch, Size: 1861 bytes --] From 5de739d03a7da71127b771cc213c873cd711ce51 Mon Sep 17 00:00:00 2001 From: Daiki Ueno <ueno@gnu.org> Date: Tue, 26 May 2020 07:56:13 +0200 Subject: [PATCH] read-file: make use of fopen-gnu * lib/read-file.c (read_file): Pass an 'e' flag to fopen. (read_binary_file): Likewise. * modules/read-file (Depends-on): Add fopen-gnu. --- ChangeLog | 7 +++++++ lib/read-file.c | 4 ++-- modules/read-file | 1 + 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index b1acb99ca..07d4d5124 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2020-05-26 Daiki Ueno <ueno@gnu.org> + + read-file: make use of fopen-gnu + * lib/read-file.c (read_file): Pass an 'e' flag to fopen. + (read_binary_file): Likewise. + * modules/read-file (Depends-on): Add fopen-gnu. + 2020-05-25 Paul Eggert <eggert@cs.ucla.edu> getentropy, getrandom: new modules diff --git a/lib/read-file.c b/lib/read-file.c index c6f230178..293bc3e8a 100644 --- a/lib/read-file.c +++ b/lib/read-file.c @@ -171,7 +171,7 @@ internal_read_file (const char *filename, size_t *length, const char *mode) char * read_file (const char *filename, size_t *length) { - return internal_read_file (filename, length, "r"); + return internal_read_file (filename, length, "re"); } /* Open (on non-POSIX systems, in binary mode) and read the contents @@ -184,5 +184,5 @@ read_file (const char *filename, size_t *length) char * read_binary_file (const char *filename, size_t *length) { - return internal_read_file (filename, length, "rb"); + return internal_read_file (filename, length, "rbe"); } diff --git a/modules/read-file b/modules/read-file index 506e88f0a..a6e7faf0a 100644 --- a/modules/read-file +++ b/modules/read-file @@ -7,6 +7,7 @@ lib/read-file.c m4/read-file.m4 Depends-on: +fopen-gnu fstat ftello malloc-posix -- 2.26.2 [-- Attachment #3: Type: text/plain, Size: 25 bytes --] Regards, -- Daiki Ueno ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-26 6:01 ` Daiki Ueno @ 2020-05-26 8:18 ` Bruno Haible 2020-05-26 16:09 ` Bruno Haible 0 siblings, 1 reply; 14+ messages in thread From: Bruno Haible @ 2020-05-26 8:18 UTC (permalink / raw) To: Daiki Ueno; +Cc: Tim Rühsen, bug-gnulib Hi Daiki, > Thank you for this; would it make sense to use it in the modules that do > one-shot fopen/fclose, such as read-file? This would improve things for multithreaded programs that call exec or posix_spawn. There are not that many programs of this kind. But on the other hand, multithreading is meant to be a "built-in" functionality nowadays, and the burden of fopen-gnu is zero on the majority of the modern platforms and small on the other platforms. => I've applied your patch. Bruno ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-26 8:18 ` Bruno Haible @ 2020-05-26 16:09 ` Bruno Haible 2020-05-27 18:39 ` Bruno Haible 0 siblings, 1 reply; 14+ messages in thread From: Bruno Haible @ 2020-05-26 16:09 UTC (permalink / raw) To: Daiki Ueno; +Cc: Tim Rühsen, bug-gnulib [-- Attachment #1: Type: text/plain, Size: 345 bytes --] Hi Daiki, > > Thank you for this; would it make sense to use it in the modules that do > > one-shot fopen/fclose, such as read-file? Here are proposed patches for other modules. Does this look right? Note: The patch to sethostname.c is only relevant for Minix. Minix does not have multithreading now, but a future version likely will. Bruno [-- Attachment #2: 0001-bitset-Make-more-robust-in-multithreaded-application.patch --] [-- Type: text/x-patch, Size: 1902 bytes --] From acb3be326fbbeaf82af33fbd7bb7b3cb180d2616 Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Tue, 26 May 2020 17:51:03 +0200 Subject: [PATCH 1/8] bitset: Make more robust in multithreaded applications. * lib/bitset/stats.c (bitset_stats_read, bitset_stats_write): Pass an 'e' flag to fopen. * modules/bitset (Depends-on): Add fopen-gnu. --- ChangeLog | 7 +++++++ lib/bitset/stats.c | 4 ++-- modules/bitset | 1 + 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 07d4d51..ca9fbb3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2020-05-26 Bruno Haible <bruno@clisp.org> + + bitset: Make more robust in multithreaded applications. + * lib/bitset/stats.c (bitset_stats_read, bitset_stats_write): Pass an + 'e' flag to fopen. + * modules/bitset (Depends-on): Add fopen-gnu. + 2020-05-26 Daiki Ueno <ueno@gnu.org> read-file: make use of fopen-gnu diff --git a/lib/bitset/stats.c b/lib/bitset/stats.c index 10aa5d7..5bd44c0 100644 --- a/lib/bitset/stats.c +++ b/lib/bitset/stats.c @@ -245,7 +245,7 @@ bitset_stats_read (const char *file_name) if (!file_name) file_name = BITSET_STATS_FILE; - FILE *file = fopen (file_name, "r"); + FILE *file = fopen (file_name, "re"); if (file) { if (fread (&bitset_stats_info_data, sizeof (bitset_stats_info_data), @@ -273,7 +273,7 @@ bitset_stats_write (const char *file_name) if (!file_name) file_name = BITSET_STATS_FILE; - FILE *file = fopen (file_name, "w"); + FILE *file = fopen (file_name, "we"); if (file) { if (fwrite (&bitset_stats_info_data, sizeof (bitset_stats_info_data), diff --git a/modules/bitset b/modules/bitset index ec7f34b..20c6806 100644 --- a/modules/bitset +++ b/modules/bitset @@ -19,6 +19,7 @@ lib/bitset/vector.h Depends-on: attribute c99 +fopen-gnu gettext-h obstack xalloc -- 2.7.4 [-- Attachment #3: 0002-exclude-Make-more-robust-in-multithreaded-applicatio.patch --] [-- Type: text/x-patch, Size: 1658 bytes --] From 67d0dc62291be365765dac3fb9e8c006d8427097 Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Tue, 26 May 2020 17:52:23 +0200 Subject: [PATCH 2/8] exclude: Make more robust in multithreaded applications. * lib/exclude.c (add_exclude_file): Pass an 'e' flag to fopen. * modules/exclude (Depends-on): Add fopen-gnu. --- ChangeLog | 6 ++++++ lib/exclude.c | 2 +- modules/exclude | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index ca9fbb3..e766479 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2020-05-26 Bruno Haible <bruno@clisp.org> + exclude: Make more robust in multithreaded applications. + * lib/exclude.c (add_exclude_file): Pass an 'e' flag to fopen. + * modules/exclude (Depends-on): Add fopen-gnu. + +2020-05-26 Bruno Haible <bruno@clisp.org> + bitset: Make more robust in multithreaded applications. * lib/bitset/stats.c (bitset_stats_read, bitset_stats_write): Pass an 'e' flag to fopen. diff --git a/lib/exclude.c b/lib/exclude.c index c63c004..2b57a9b 100644 --- a/lib/exclude.c +++ b/lib/exclude.c @@ -683,7 +683,7 @@ add_exclude_file (void (*add_func) (struct exclude *, char const *, int), if (use_stdin) in = stdin; - else if (! (in = fopen (file_name, "r"))) + else if (! (in = fopen (file_name, "re"))) return -1; rc = add_exclude_fp (call_addfn, ex, in, options, line_end, &add_func); diff --git a/modules/exclude b/modules/exclude index 5ff0539..8fe2708 100644 --- a/modules/exclude +++ b/modules/exclude @@ -8,6 +8,7 @@ lib/exclude.c Depends-on: filename fnmatch +fopen-gnu hash mbscasecmp mbuiter -- 2.7.4 [-- Attachment #4: 0003-getloadavg-Make-more-robust-in-multithreaded-applica.patch --] [-- Type: text/x-patch, Size: 1651 bytes --] From 350c6f8dba8cc2a6266cb8a350d01c49805af9db Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Tue, 26 May 2020 17:53:47 +0200 Subject: [PATCH 3/8] getloadavg: Make more robust in multithreaded applications. * lib/getloadavg.c (getloadavg): Pass an 'e' flag to fopen. * modules/getloadavg (Depends-on): Add fopen-gnu. --- ChangeLog | 6 ++++++ lib/getloadavg.c | 2 +- modules/getloadavg | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index e766479..7dabc5e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2020-05-26 Bruno Haible <bruno@clisp.org> + getloadavg: Make more robust in multithreaded applications. + * lib/getloadavg.c (getloadavg): Pass an 'e' flag to fopen. + * modules/getloadavg (Depends-on): Add fopen-gnu. + +2020-05-26 Bruno Haible <bruno@clisp.org> + exclude: Make more robust in multithreaded applications. * lib/exclude.c (add_exclude_file): Pass an 'e' flag to fopen. * modules/exclude (Depends-on): Add fopen-gnu. diff --git a/lib/getloadavg.c b/lib/getloadavg.c index ebb6f5d..7e11c32 100644 --- a/lib/getloadavg.c +++ b/lib/getloadavg.c @@ -569,7 +569,7 @@ getloadavg (double loadavg[], int nelem) int count; FILE *fp; - fp = fopen (NETBSD_LDAV_FILE, "r"); + fp = fopen (NETBSD_LDAV_FILE, "re"); if (fp == NULL) return -1; count = fscanf (fp, "%lu %lu %lu %lu\n", diff --git a/modules/getloadavg b/modules/getloadavg index 1b0f581..e14ddc8 100644 --- a/modules/getloadavg +++ b/modules/getloadavg @@ -7,6 +7,7 @@ m4/getloadavg.m4 Depends-on: extensions +fopen-gnu intprops stdbool stdlib -- 2.7.4 [-- Attachment #5: 0004-getpass-Make-more-robust-in-multithreaded-applicatio.patch --] [-- Type: text/x-patch, Size: 1639 bytes --] From 54e5d001797c755994968a34259bae745ee515e6 Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Tue, 26 May 2020 17:56:31 +0200 Subject: [PATCH 4/8] getpass: Make more robust in multithreaded applications. * lib/getpass.c (getpass): Pass an 'e' flag to fopen. * modules/getpass (Depends-on): Add fopen-gnu. --- ChangeLog | 6 ++++++ lib/getpass.c | 2 +- modules/getpass | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 7dabc5e..93d8b5a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2020-05-26 Bruno Haible <bruno@clisp.org> + getpass: Make more robust in multithreaded applications. + * lib/getpass.c (getpass): Pass an 'e' flag to fopen. + * modules/getpass (Depends-on): Add fopen-gnu. + +2020-05-26 Bruno Haible <bruno@clisp.org> + getloadavg: Make more robust in multithreaded applications. * lib/getloadavg.c (getloadavg): Pass an 'e' flag to fopen. * modules/getloadavg (Depends-on): Add fopen-gnu. diff --git a/lib/getpass.c b/lib/getpass.c index af8d72e..3b0552e 100644 --- a/lib/getpass.c +++ b/lib/getpass.c @@ -96,7 +96,7 @@ getpass (const char *prompt) /* Try to write to and read from the terminal if we can. If we can't open the terminal, use stderr and stdin. */ - tty = fopen ("/dev/tty", "w+"); + tty = fopen ("/dev/tty", "w+e"); if (tty == NULL) { in = stdin; diff --git a/modules/getpass b/modules/getpass index 7e73451..74d781a 100644 --- a/modules/getpass +++ b/modules/getpass @@ -9,6 +9,7 @@ m4/getpass.m4 Depends-on: unistd extensions +fopen-gnu fseeko getline stdbool -- 2.7.4 [-- Attachment #6: 0005-readutmp-Make-more-robust-in-multithreaded-applicati.patch --] [-- Type: text/x-patch, Size: 1613 bytes --] From 56bcba9b1013176d52f1b25a3085695a3bd8d9b8 Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Tue, 26 May 2020 17:57:58 +0200 Subject: [PATCH 5/8] readutmp: Make more robust in multithreaded applications. * lib/readutmp.c (read_utmp): Pass an 'e' flag to fopen. * modules/readutmp (Depends-on): Add fopen-gnu. --- ChangeLog | 6 ++++++ lib/readutmp.c | 2 +- modules/readutmp | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 93d8b5a..893d080 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2020-05-26 Bruno Haible <bruno@clisp.org> + readutmp: Make more robust in multithreaded applications. + * lib/readutmp.c (read_utmp): Pass an 'e' flag to fopen. + * modules/readutmp (Depends-on): Add fopen-gnu. + +2020-05-26 Bruno Haible <bruno@clisp.org> + getpass: Make more robust in multithreaded applications. * lib/getpass.c (getpass): Pass an 'e' flag to fopen. * modules/getpass (Depends-on): Add fopen-gnu. diff --git a/lib/readutmp.c b/lib/readutmp.c index 308390d..793d480 100644 --- a/lib/readutmp.c +++ b/lib/readutmp.c @@ -132,7 +132,7 @@ read_utmp (char const *file, size_t *n_entries, STRUCT_UTMP **utmp_buf, size_t n_alloc = 0; STRUCT_UTMP *utmp = NULL; int saved_errno; - FILE *f = fopen (file, "r"); + FILE *f = fopen (file, "re"); if (! f) return -1; diff --git a/modules/readutmp b/modules/readutmp index 51f6290..e88897c 100644 --- a/modules/readutmp +++ b/modules/readutmp @@ -11,6 +11,7 @@ extensions xalloc stdbool stdint +fopen-gnu configure.ac: gl_READUTMP -- 2.7.4 [-- Attachment #7: 0006-sethostname-Make-more-robust-in-multithreaded-applic.patch --] [-- Type: text/x-patch, Size: 1918 bytes --] From acdcd9f872f5338f901a9019627953dcb5e8d95f Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Tue, 26 May 2020 18:00:04 +0200 Subject: [PATCH 6/8] sethostname: Make more robust in multithreaded applications. * lib/sethostname.c (sethostname): Pass an 'e' flag to fopen. * modules/sethostname (Depends-on): Add fopen-gnu. --- ChangeLog | 6 ++++++ lib/sethostname.c | 2 +- modules/sethostname | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 893d080..162b861 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2020-05-26 Bruno Haible <bruno@clisp.org> + sethostname: Make more robust in multithreaded applications. + * lib/sethostname.c (sethostname): Pass an 'e' flag to fopen. + * modules/sethostname (Depends-on): Add fopen-gnu. + +2020-05-26 Bruno Haible <bruno@clisp.org> + readutmp: Make more robust in multithreaded applications. * lib/readutmp.c (read_utmp): Pass an 'e' flag to fopen. * modules/readutmp (Depends-on): Add fopen-gnu. diff --git a/lib/sethostname.c b/lib/sethostname.c index 87b3af9..1be69be 100644 --- a/lib/sethostname.c +++ b/lib/sethostname.c @@ -52,7 +52,7 @@ sethostname (const char *name, size_t len) these are appropriate for us to set, even if they may match the situation, during failed open/write/close operations, so we leave errno alone and rely on what the system sets up. */ - hostf = fopen ("/etc/hostname.file", "w"); + hostf = fopen ("/etc/hostname.file", "we"); if (hostf == NULL) r = -1; else diff --git a/modules/sethostname b/modules/sethostname index f3c0d13..92b4a2b 100644 --- a/modules/sethostname +++ b/modules/sethostname @@ -9,6 +9,7 @@ m4/gethostname.m4 Depends-on: unistd errno [test $HAVE_SETHOSTNAME = 0] +fopen-gnu [test $HAVE_SETHOSTNAME = 0] configure.ac: gl_FUNC_SETHOSTNAME -- 2.7.4 [-- Attachment #8: 0007-mountlist-Make-more-robust-in-multithreaded-applicat.patch --] [-- Type: text/x-patch, Size: 2788 bytes --] From 7fac4fbbd7203d2fcf3be257872e3c7508b86402 Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Tue, 26 May 2020 18:04:26 +0200 Subject: [PATCH 7/8] mountlist: Make more robust in multithreaded applications. * lib/mountlist.c (setmntent, read_file_system_list): Pass an 'e' flag to fopen. * modules/mountlist (Depends-on): Add fopen-gnu. --- ChangeLog | 7 +++++++ lib/mountlist.c | 10 +++++----- modules/mountlist | 1 + 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index 162b861..b30f4b1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,12 @@ 2020-05-26 Bruno Haible <bruno@clisp.org> + mountlist: Make more robust in multithreaded applications. + * lib/mountlist.c (setmntent, read_file_system_list): Pass an 'e' flag + to fopen. + * modules/mountlist (Depends-on): Add fopen-gnu. + +2020-05-26 Bruno Haible <bruno@clisp.org> + sethostname: Make more robust in multithreaded applications. * lib/sethostname.c (sethostname): Pass an 'e' flag to fopen. * modules/sethostname (Depends-on): Add fopen-gnu. diff --git a/lib/mountlist.c b/lib/mountlist.c index 7abe024..4cb19c8 100644 --- a/lib/mountlist.c +++ b/lib/mountlist.c @@ -125,7 +125,7 @@ #ifdef MOUNTED_GETMNTENT1 # if !HAVE_SETMNTENT /* Android <= 4.4 */ -# define setmntent(fp,mode) fopen (fp, mode) +# define setmntent(fp,mode) fopen (fp, mode "e") # endif # if !HAVE_ENDMNTENT /* Android <= 4.4 */ # define endmntent(fp) fclose (fp) @@ -460,7 +460,7 @@ read_file_system_list (bool need_fs_type) (and that code is in previous versions of this function), however libmount depends on libselinux which pulls in many dependencies. */ char const *mountinfo = "/proc/self/mountinfo"; - fp = fopen (mountinfo, "r"); + fp = fopen (mountinfo, "re"); if (fp != NULL) { char *line = NULL; @@ -794,7 +794,7 @@ read_file_system_list (bool need_fs_type) char *table = "/etc/mnttab"; FILE *fp; - fp = fopen (table, "r"); + fp = fopen (table, "re"); if (fp == NULL) return NULL; @@ -852,7 +852,7 @@ read_file_system_list (bool need_fs_type) by the kernel. */ errno = 0; - fp = fopen (table, "r"); + fp = fopen (table, "re"); if (fp == NULL) ret = errno; else @@ -924,7 +924,7 @@ read_file_system_list (bool need_fs_type) # endif errno = 0; - fp = fopen (table, "r"); + fp = fopen (table, "re"); if (fp == NULL) ret = errno; else diff --git a/modules/mountlist b/modules/mountlist index 11cf809..5bb45ed 100644 --- a/modules/mountlist +++ b/modules/mountlist @@ -8,6 +8,7 @@ m4/fstypename.m4 m4/mountlist.m4 Depends-on: +fopen-gnu getline stdbool stdint -- 2.7.4 [-- Attachment #9: 0008-javacomp-Make-more-robust-in-multithreaded-applicati.patch --] [-- Type: text/x-patch, Size: 1676 bytes --] From 0f0f1ad56a75eb04e091ba84a6707b4431eeb68e Mon Sep 17 00:00:00 2001 From: Bruno Haible <bruno@clisp.org> Date: Tue, 26 May 2020 18:05:34 +0200 Subject: [PATCH 8/8] javacomp: Make more robust in multithreaded applications. * lib/javacomp.c (write_temp_file): Pass an 'e' flag to fopen_temp. * modules/javacomp (Depends-on): Add fopen-gnu. --- ChangeLog | 6 ++++++ lib/javacomp.c | 2 +- modules/javacomp | 1 + 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index b30f4b1..b95b945 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2020-05-26 Bruno Haible <bruno@clisp.org> + javacomp: Make more robust in multithreaded applications. + * lib/javacomp.c (write_temp_file): Pass an 'e' flag to fopen_temp. + * modules/javacomp (Depends-on): Add fopen-gnu. + +2020-05-26 Bruno Haible <bruno@clisp.org> + mountlist: Make more robust in multithreaded applications. * lib/mountlist.c (setmntent, read_file_system_list): Pass an 'e' flag to fopen. diff --git a/lib/javacomp.c b/lib/javacomp.c index 9239616..ac56196 100644 --- a/lib/javacomp.c +++ b/lib/javacomp.c @@ -573,7 +573,7 @@ write_temp_file (struct temp_dir *tmpdir, const char *file_name, FILE *fp; register_temp_file (tmpdir, file_name); - fp = fopen_temp (file_name, "w"); + fp = fopen_temp (file_name, "we"); if (fp == NULL) { error (0, errno, _("failed to create \"%s\""), file_name); diff --git a/modules/javacomp b/modules/javacomp index 4f49bd9..fccdaac 100644 --- a/modules/javacomp +++ b/modules/javacomp @@ -22,6 +22,7 @@ xmalloca getline xconcat-filename fwriteerror +fopen-gnu clean-temp stat error -- 2.7.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-26 16:09 ` Bruno Haible @ 2020-05-27 18:39 ` Bruno Haible 2020-05-28 10:00 ` Daiki Ueno 0 siblings, 1 reply; 14+ messages in thread From: Bruno Haible @ 2020-05-27 18:39 UTC (permalink / raw) To: Daiki Ueno; +Cc: Tim Rühsen, bug-gnulib > Here are proposed patches for other modules. Does this look right? There were no objections. I pushed the changes. Bruno ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-27 18:39 ` Bruno Haible @ 2020-05-28 10:00 ` Daiki Ueno 2020-05-28 10:04 ` Daiki Ueno 0 siblings, 1 reply; 14+ messages in thread From: Daiki Ueno @ 2020-05-28 10:00 UTC (permalink / raw) To: Bruno Haible; +Cc: Tim Rühsen, bug-gnulib [-- Attachment #1: Type: text/plain, Size: 490 bytes --] Bruno Haible <bruno@clisp.org> writes: >> Here are proposed patches for other modules. Does this look right? > > There were no objections. I pushed the changes. Thank you for this. I have rebased GnuTLS on top of it, but noticed a strange test failures on Windows CI, which involve reading binary files (OCSP response): https://gitlab.com/gnutls/gnutls/-/jobs/569815031 It seems that the fopen module ignores a 'b' flag. The attached patch fixes the failures. Regards, -- Daiki Ueno [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-fopen-gnu-make-b-flag-can-be-used-with-e-on-Windows.patch --] [-- Type: text/x-patch, Size: 3437 bytes --] From 17fbb2560a05e3006125f8793c8e814ef5baa847 Mon Sep 17 00:00:00 2001 From: Daiki Ueno <ueno@gnu.org> Date: Thu, 28 May 2020 11:40:49 +0200 Subject: [PATCH] fopen-gnu: make 'b' flag can be used with 'e' on Windows * lib/fopen.c (rpl_fopen): Pass O_BINARY to open, if a 'b' flag is specified on Windows. * tests/test-fopen-gnu.c (DATA): New define. (main): Add test for reading binary files with an 'e' flag. --- ChangeLog | 8 ++++++++ lib/fopen.c | 9 +++++++-- tests/test-fopen-gnu.c | 17 +++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index c17b76b72..ea2716b2f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2020-05-28 Daiki Ueno <ueno@gnu.org> + + fopen-gnu: make 'b' flag can be used with 'e' on Windows + * lib/fopen.c (rpl_fopen): Pass O_BINARY to open, if a 'b' flag is + specified on Windows. + * tests/test-fopen-gnu.c (DATA): New define. + (main): Add test for reading binary files with an 'e' flag. + 2020-05-27 Bruno Haible <bruno@clisp.org> Don't assume that UNICODE is not defined. diff --git a/lib/fopen.c b/lib/fopen.c index 20065e4c6..f60c51f95 100644 --- a/lib/fopen.c +++ b/lib/fopen.c @@ -51,6 +51,7 @@ rpl_fopen (const char *filename, const char *mode) int open_flags_standard; #if GNULIB_FOPEN_GNU int open_flags_gnu; + int open_flags_other; # define BUF_SIZE 80 char fdopen_mode_buf[BUF_SIZE + 1]; #endif @@ -66,6 +67,7 @@ rpl_fopen (const char *filename, const char *mode) open_flags_standard = 0; #if GNULIB_FOPEN_GNU open_flags_gnu = 0; + open_flags_other = 0; #endif { const char *p = mode; @@ -101,6 +103,9 @@ rpl_fopen (const char *filename, const char *mode) #endif continue; case 'b': +#if defined _WIN32 && ! defined __CYGWIN__ + open_flags_other |= O_BINARY; +#endif #if GNULIB_FOPEN_GNU if (q < fdopen_mode_buf + BUF_SIZE) *q++ = *p; @@ -142,9 +147,9 @@ rpl_fopen (const char *filename, const char *mode) #endif } #if GNULIB_FOPEN_GNU - open_flags = open_flags_standard | open_flags_gnu; + open_flags = open_flags_standard | open_flags_other | open_flags_gnu; #else - open_flags = open_flags_standard; + open_flags = open_flags_standard | open_flags_other; #endif #if FOPEN_TRAILING_SLASH_BUG diff --git a/tests/test-fopen-gnu.c b/tests/test-fopen-gnu.c index cae40421a..eeb1712c7 100644 --- a/tests/test-fopen-gnu.c +++ b/tests/test-fopen-gnu.c @@ -29,15 +29,20 @@ #define BASE "test-fopen-gnu.t" +/* 0x1a is an EOF on Windows. */ +#define DATA "abc\x1adef" + int main (void) { FILE *f; int fd; int flags; + char buf[16]; /* Remove anything from prior partial run. */ unlink (BASE "file"); + unlink (BASE "binary"); /* Create the file. */ f = fopen (BASE "file", "w"); @@ -64,8 +69,20 @@ main (void) ASSERT (f == NULL); ASSERT (errno == EEXIST); + /* Open a binary file and check that the 'e' mode doesn't interfere. */ + f = fopen (BASE "binary", "wbe"); + ASSERT (f); + ASSERT (fwrite (DATA, 1, sizeof(DATA)-1, f) == sizeof(DATA)-1); + ASSERT (fclose (f) == 0); + + f = fopen (BASE "binary", "rbe"); + ASSERT (f); + ASSERT (fread (buf, 1, sizeof(buf), f) == sizeof(DATA)-1); + ASSERT (fclose (f) == 0); + /* Cleanup. */ ASSERT (unlink (BASE "file") == 0); + ASSERT (unlink (BASE "binary") == 0); return 0; } -- 2.26.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-28 10:00 ` Daiki Ueno @ 2020-05-28 10:04 ` Daiki Ueno 2020-05-28 11:54 ` Bruno Haible 0 siblings, 1 reply; 14+ messages in thread From: Daiki Ueno @ 2020-05-28 10:04 UTC (permalink / raw) To: Bruno Haible; +Cc: Tim Rühsen, bug-gnulib [-- Attachment #1: Type: text/plain, Size: 630 bytes --] Daiki Ueno <ueno@gnu.org> writes: > Bruno Haible <bruno@clisp.org> writes: > >>> Here are proposed patches for other modules. Does this look right? >> >> There were no objections. I pushed the changes. > > Thank you for this. I have rebased GnuTLS on top of it, but noticed a > strange test failures on Windows CI, which involve reading binary files > (OCSP response): > https://gitlab.com/gnutls/gnutls/-/jobs/569815031 > > It seems that the fopen module ignores a 'b' flag. The attached patch > fixes the failures. Sorry, attached an old patch; this would be simpler (and also supports other platforms that need O_BINARY). [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-fopen-gnu-make-b-flag-can-be-used-with-e-on-Windows.patch --] [-- Type: text/x-patch, Size: 2740 bytes --] From 81695244eb467603009a2777c3a8438f1a707954 Mon Sep 17 00:00:00 2001 From: Daiki Ueno <ueno@gnu.org> Date: Thu, 28 May 2020 11:40:49 +0200 Subject: [PATCH] fopen-gnu: make 'b' flag can be used with 'e' on Windows * lib/fopen.c (rpl_fopen): Pass O_BINARY to open, if a 'b' flag is specified on Windows. * tests/test-fopen-gnu.c (DATA): New define. (main): Add test for reading binary files with an 'e' flag. --- ChangeLog | 8 ++++++++ lib/fopen.c | 4 ++++ tests/test-fopen-gnu.c | 17 +++++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/ChangeLog b/ChangeLog index c17b76b72..ea2716b2f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2020-05-28 Daiki Ueno <ueno@gnu.org> + + fopen-gnu: make 'b' flag can be used with 'e' on Windows + * lib/fopen.c (rpl_fopen): Pass O_BINARY to open, if a 'b' flag is + specified on Windows. + * tests/test-fopen-gnu.c (DATA): New define. + (main): Add test for reading binary files with an 'e' flag. + 2020-05-27 Bruno Haible <bruno@clisp.org> Don't assume that UNICODE is not defined. diff --git a/lib/fopen.c b/lib/fopen.c index 20065e4c6..47d7f194d 100644 --- a/lib/fopen.c +++ b/lib/fopen.c @@ -101,6 +101,10 @@ rpl_fopen (const char *filename, const char *mode) #endif continue; case 'b': + /* While it is non-standard, O_BINARY is guaranteed by + gnulib <fcntl.h>. We can also assume that orig_fopen + supports the 'b' flag. */ + open_flags_standard |= O_BINARY; #if GNULIB_FOPEN_GNU if (q < fdopen_mode_buf + BUF_SIZE) *q++ = *p; diff --git a/tests/test-fopen-gnu.c b/tests/test-fopen-gnu.c index cae40421a..eeb1712c7 100644 --- a/tests/test-fopen-gnu.c +++ b/tests/test-fopen-gnu.c @@ -29,15 +29,20 @@ #define BASE "test-fopen-gnu.t" +/* 0x1a is an EOF on Windows. */ +#define DATA "abc\x1adef" + int main (void) { FILE *f; int fd; int flags; + char buf[16]; /* Remove anything from prior partial run. */ unlink (BASE "file"); + unlink (BASE "binary"); /* Create the file. */ f = fopen (BASE "file", "w"); @@ -64,8 +69,20 @@ main (void) ASSERT (f == NULL); ASSERT (errno == EEXIST); + /* Open a binary file and check that the 'e' mode doesn't interfere. */ + f = fopen (BASE "binary", "wbe"); + ASSERT (f); + ASSERT (fwrite (DATA, 1, sizeof(DATA)-1, f) == sizeof(DATA)-1); + ASSERT (fclose (f) == 0); + + f = fopen (BASE "binary", "rbe"); + ASSERT (f); + ASSERT (fread (buf, 1, sizeof(buf), f) == sizeof(DATA)-1); + ASSERT (fclose (f) == 0); + /* Cleanup. */ ASSERT (unlink (BASE "file") == 0); + ASSERT (unlink (BASE "binary") == 0); return 0; } -- 2.26.2 [-- Attachment #3: Type: text/plain, Size: 28 bytes --] Regards, -- Daiki Ueno ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-28 10:04 ` Daiki Ueno @ 2020-05-28 11:54 ` Bruno Haible 2020-05-29 3:04 ` Daiki Ueno 0 siblings, 1 reply; 14+ messages in thread From: Bruno Haible @ 2020-05-28 11:54 UTC (permalink / raw) To: Daiki Ueno; +Cc: Tim Rühsen, bug-gnulib Hi Daiki, Thank you for noticing this, and the rapid fix. > Sorry, attached an old patch; this would be simpler (and also supports > other platforms that need O_BINARY). > + ASSERT (fwrite (DATA, 1, sizeof(DATA)-1, f) == sizeof(DATA)-1); ... > + ASSERT (fread (buf, 1, sizeof(buf), f) == sizeof(DATA)-1); GNU coding style wants a space between 'sizeof' and the opening parenthesis. Other than that, your patch is perfect. Thanks again! Bruno ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-28 11:54 ` Bruno Haible @ 2020-05-29 3:04 ` Daiki Ueno 2020-05-29 6:25 ` Bruno Haible 0 siblings, 1 reply; 14+ messages in thread From: Daiki Ueno @ 2020-05-29 3:04 UTC (permalink / raw) To: Bruno Haible; +Cc: Tim Rühsen, bug-gnulib [-- Attachment #1: Type: text/plain, Size: 491 bytes --] Bruno Haible <bruno@clisp.org> writes: >> + ASSERT (fwrite (DATA, 1, sizeof(DATA)-1, f) == sizeof(DATA)-1); > ... >> + ASSERT (fread (buf, 1, sizeof(buf), f) == sizeof(DATA)-1); > > GNU coding style wants a space between 'sizeof' and the opening parenthesis. > Other than that, your patch is perfect. Thank you for the review; fixed and pushed. Afterwards, I realized a compilation error in the test code on FreeBSD, so I've added the follow-up patch attached. Regards, -- Daiki Ueno [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-fopen-gnu-tests-fix-x-escape-usage.patch --] [-- Type: text/x-patch, Size: 1079 bytes --] From 9326739489050009e3b8834838abc02203c592e5 Mon Sep 17 00:00:00 2001 From: Daiki Ueno <ueno@gnu.org> Date: Fri, 29 May 2020 04:54:31 +0200 Subject: [PATCH] fopen-gnu-tests: fix "\x" escape usage * tests/test-fopen-gnu.c (DATA): Use safer escape sequence. --- ChangeLog | 5 +++++ tests/test-fopen-gnu.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index c39bfd372..77c637414 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2020-05-29 Daiki Ueno <ueno@gnu.org> + + fopen-gnu-tests: fix "\x" escape usage + * tests/test-fopen-gnu.c (DATA): Use safer escape sequence. + 2020-05-28 Bruno Haible <bruno@clisp.org> Avoid dynamic loading of Windows API functions when possible. diff --git a/tests/test-fopen-gnu.c b/tests/test-fopen-gnu.c index 4d98dcd85..7de45ab8d 100644 --- a/tests/test-fopen-gnu.c +++ b/tests/test-fopen-gnu.c @@ -30,7 +30,7 @@ #define BASE "test-fopen-gnu.t" /* 0x1a is an EOF on Windows. */ -#define DATA "abc\x1adef" +#define DATA "abc\x1axyz" int main (void) -- 2.26.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: portability of fopen and 'e' (O_CLOEXEC) flag 2020-05-29 3:04 ` Daiki Ueno @ 2020-05-29 6:25 ` Bruno Haible 0 siblings, 0 replies; 14+ messages in thread From: Bruno Haible @ 2020-05-29 6:25 UTC (permalink / raw) To: Daiki Ueno; +Cc: Tim Rühsen, bug-gnulib Daiki Ueno wrote: > Afterwards, I realized a > compilation error in the test code on FreeBSD, so I've added the > follow-up patch attached. Indeed, octal and hexadecimal escape sequences are not limited to 3 or 2 digits. ISO C 11 section 6.4.4.4 says: "The hexadecimal digits that follow the backslash and the letter x in a hexadecimal escape sequence are taken to be part of the construction ..." "Each octal or hexadecimal escape sequence is the longest sequence of characters that can constitute the escape sequence." I had forgotten about it too. > "abc\x1adef" One way to write it would be "abc\x1a" "def". Or "abc\032def". Bruno ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-05-29 6:27 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-11 14:49 portability of fopen and 'e' (O_CLOEXEC) flag Tim Rühsen 2020-05-11 16:37 ` Bruno Haible 2020-05-11 17:41 ` Eric Blake 2020-05-12 8:28 ` Tim Rühsen 2020-05-24 19:22 ` Bruno Haible 2020-05-26 6:01 ` Daiki Ueno 2020-05-26 8:18 ` Bruno Haible 2020-05-26 16:09 ` Bruno Haible 2020-05-27 18:39 ` Bruno Haible 2020-05-28 10:00 ` Daiki Ueno 2020-05-28 10:04 ` Daiki Ueno 2020-05-28 11:54 ` Bruno Haible 2020-05-29 3:04 ` Daiki Ueno 2020-05-29 6:25 ` Bruno Haible
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).