portability of fopen and 'e' (O_CLOEXEC) flag

portability of fopen and 'e' (O_CLOEXEC) flag

[Tim Rühsen]

[-- 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 --]

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Bruno Haible]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Eric Blake]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Tim Rühsen]

[-- 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 --]

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Bruno Haible]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Daiki Ueno]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Bruno Haible]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Bruno Haible]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Bruno Haible]

> Here are proposed patches for other modules. Does this look right?

There were no objections. I pushed the changes.

Bruno

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Daiki Ueno]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Daiki Ueno]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Bruno Haible]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Daiki Ueno]

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

Re: portability of fopen and 'e' (O_CLOEXEC) flag

[Bruno Haible]

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