Re: fix mkostemp, add pipe2-safer

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Eric Blake on 12/6/2009 5:53 PM:
>>   If this is specifically targeted at non-POSIX systems, it would be good to 
>> have
>>   a comment about it.
> 
> Okay, then, a comment describing that dup2 is used as an EBADF filter, and
> not for any side effects on cloexec (since there aren't any side effects),
> would make sense.
> 
>> - Regarding dup_safer_flag and fd_safer_flag: I find it important to also 
>> handle
>>   also O_BINARY and O_TEXT, like we do in the pipe2 function (see the doc in
>>   lib/unistd.in.h). pipe2 supports O_TEXT, but pipe2_safer does not: It
>>   respects the O_TEXT flag in the call to pipe2 but then loses it in the 
>> call to
>>   fd_safer_flag (because dup_cloexec has an O_BINARY flag hardwired).
> 
> Good point.  I see two options:
> 
> Change dup_cloexec to take a flag, so that mingw can handle text vs.
> binary during the dup.  For cygwin, it would have to call setmode() after
> the fact.
> 
> Leave dup_cloexec as is, and make fd_safer_flag and/or dup_safer_flag call
> setmode() after the fact for both cygwin and mingw.

Or a third.  On cygwin, native fcntl(F_DUPFD), dup, and dup2; and our
version of dup_cloexec, already copy the text/binary mode of the source.
So why not make mingw do the same, rather than blindly slamming to O_BINARY.

I wish there were a getmode, rather than having to call setmode() twice
and temporarily changing the state of a text fd.  On cygwin, you can use
fcntl(F_GETFL), but again, mingw is lacking.

Here's what I've tested on mingw and cygwin; now pushed.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksdDVQACgkQ84KuGfSFAYCVcwCgvtX7ddWAua0IJALGGbXPCT7h
Y28An0mLsm6o4gS/tUuTNJfAOq8xKD2G
=5YgE
-----END PGP SIGNATURE-----

From 73da5fb7e129f1fa540e040582cda710b8c2cce4 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Mon, 7 Dec 2009 06:53:59 -0700
Subject: [PATCH] cloexec: preserve text vs. binary across dup_cloexec

On mingw, dup_cloexec mistakenly converted a text fd into a
binary fd.  Cygwin copied the source mode.  Most other platforms
don't distinguish between modes.

* lib/cloexec.c (dup_cloexec) [W32]: Query and use translation
mode.
* modules/dup2-tests (Depends-on): Add binary-io.
* modules/cloexec-tests (Depends-on): Likewise.
* tests/test-dup2.c (setmode, is_mode): New helpers.
(main): Add tests that translation mode is preserved.
* tests/test-cloexec.c (setmode, is_mode, main): Likewise.
Reported by Bruno Haible.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog             |   10 ++++++++++
 lib/cloexec.c         |    9 +++++++--
 modules/cloexec-tests |    1 +
 modules/dup2-tests    |    1 +
 tests/test-cloexec.c  |   31 +++++++++++++++++++++++++++++++
 tests/test-dup2.c     |   26 ++++++++++++++++++++++++++
 6 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 00ac886..3cf1bdb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,15 @@
 2009-12-07  Eric Blake  <address@hidden>

+       cloexec: preserve text vs. binary across dup_cloexec
+       * lib/cloexec.c (dup_cloexec) [W32]: Query and use translation
+       mode.
+       * modules/dup2-tests (Depends-on): Add binary-io.
+       * modules/cloexec-tests (Depends-on): Likewise.
+       * tests/test-dup2.c (setmode, is_mode): New helpers.
+       (main): Add tests that translation mode is preserved.
+       * tests/test-cloexec.c (setmode, is_mode, main): Likewise.
+       Reported by Bruno Haible.
+
        mgetgroups: reduce duplicate listings
        * lib/mgetgroups.c (mgetgroups): Reduce duplicates from the
        resulting array.
diff --git a/lib/cloexec.c b/lib/cloexec.c
index 5479477..57e0be5 100644
--- a/lib/cloexec.c
+++ b/lib/cloexec.c
@@ -64,6 +64,8 @@ set_cloexec_flag (int desc, bool value)

 #else

+  /* Use dup2 to reject invalid file descriptors; the cloexec flag
+     will be unaffected.  */
   if (desc < 0)
     {
       errno = EBADF;
@@ -90,8 +92,10 @@ dup_cloexec (int fd)
   HANDLE curr_process = GetCurrentProcess ();
   HANDLE old_handle = (HANDLE) _get_osfhandle (fd);
   HANDLE new_handle;
+  int mode;

-  if (old_handle == INVALID_HANDLE_VALUE)
+  if (old_handle == INVALID_HANDLE_VALUE
+      || (mode = setmode (fd, O_BINARY)) == -1)
     {
       /* fd is closed, or is open to no handle at all.
          We cannot duplicate fd in this case, because _open_osfhandle
@@ -99,6 +103,7 @@ dup_cloexec (int fd)
       errno = EBADF;
       return -1;
     }
+  setmode (fd, mode);

   if (!DuplicateHandle (curr_process,               /* SourceProcessHandle */
                         old_handle,                 /* SourceHandle */
@@ -113,7 +118,7 @@ dup_cloexec (int fd)
       return -1;
     }

-  nfd = _open_osfhandle ((long) new_handle, O_BINARY | O_NOINHERIT);
+  nfd = _open_osfhandle ((long) new_handle, mode | O_NOINHERIT);
   if (nfd < 0)
     {
       CloseHandle (new_handle);
diff --git a/modules/cloexec-tests b/modules/cloexec-tests
index 38c304c..22792db 100644
--- a/modules/cloexec-tests
+++ b/modules/cloexec-tests
@@ -2,6 +2,7 @@ Files:
 tests/test-cloexec.c

 Depends-on:
+binary-io

 configure.ac:

diff --git a/modules/dup2-tests b/modules/dup2-tests
index 0fb913a..b02e2a2 100644
--- a/modules/dup2-tests
+++ b/modules/dup2-tests
@@ -2,6 +2,7 @@ Files:
 tests/test-dup2.c

 Depends-on:
+binary-io
 cloexec
 open

diff --git a/tests/test-cloexec.c b/tests/test-cloexec.c
index 8df733a..a29d1be 100644
--- a/tests/test-cloexec.c
+++ b/tests/test-cloexec.c
@@ -32,6 +32,8 @@
 # include <windows.h>
 #endif

+#include "binary-io.h"
+
 #define ASSERT(expr) \
   do                                                                         \
     {                                                                        \
@@ -66,6 +68,20 @@ is_inheritable (int fd)
 #endif
 }

+#if !O_BINARY
+# define setmode(f,m) 0
+#endif
+
+/* Return non-zero if FD is open in the given MODE, which is either
+   O_TEXT or O_BINARY.  */
+static int
+is_mode (int fd, int mode)
+{
+  int value = setmode (fd, O_BINARY);
+  setmode (fd, value);
+  return mode == value;
+}
+
 int
 main (void)
 {
@@ -94,6 +110,21 @@ main (void)
   ASSERT (!is_inheritable (fd));
   ASSERT (close (fd2) == 0);

+  /* On systems that distinguish between text and binary mode,
+     dup_cloexec reuses the mode of the source.  */
+  setmode (fd, O_BINARY);
+  ASSERT (is_mode (fd, O_BINARY));
+  fd2 = dup_cloexec (fd);
+  ASSERT (fd < fd2);
+  ASSERT (is_mode (fd2, O_BINARY));
+  ASSERT (close (fd2) == 0);
+  setmode (fd, O_TEXT);
+  ASSERT (is_mode (fd, O_TEXT));
+  fd2 = dup_cloexec (fd);
+  ASSERT (fd < fd2);
+  ASSERT (is_mode (fd2, O_TEXT));
+  ASSERT (close (fd2) == 0);
+
   /* Test error handling.  */
   errno = 0;
   ASSERT (set_cloexec_flag (-1, false) == -1);
diff --git a/tests/test-dup2.c b/tests/test-dup2.c
index 005160f..e6d3c62 100644
--- a/tests/test-dup2.c
+++ b/tests/test-dup2.c
@@ -25,6 +25,7 @@
 #include <stdio.h>
 #include <stdlib.h>

+#include "binary-io.h"
 #include "cloexec.h"

 #if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
@@ -84,6 +85,20 @@ is_inheritable (int fd)
 #endif
 }

+#if !O_BINARY
+# define setmode(f,m) 0
+#endif
+
+/* Return non-zero if FD is open in the given MODE, which is either
+   O_TEXT or O_BINARY.  */
+static int
+is_mode (int fd, int mode)
+{
+  int value = setmode (fd, O_BINARY);
+  setmode (fd, value);
+  return mode == value;
+}
+
 int
 main (void)
 {
@@ -158,6 +173,17 @@ main (void)
   ASSERT (dup2 (fd + 1, fd + 2) == fd + 2);
   ASSERT (is_inheritable (fd + 2));

+  /* On systems that distinguish between text and binary mode, dup2
+     reuses the mode of the source.  */
+  setmode (fd, O_BINARY);
+  ASSERT (is_mode (fd, O_BINARY));
+  ASSERT (dup2 (fd, fd + 1) == fd + 1);
+  ASSERT (is_mode (fd + 1, O_BINARY));
+  setmode (fd, O_TEXT);
+  ASSERT (is_mode (fd, O_TEXT));
+  ASSERT (dup2 (fd, fd + 1) == fd + 1);
+  ASSERT (is_mode (fd + 1, O_TEXT));
+
   /* Clean up.  */
   ASSERT (close (fd + 2) == 0);
   ASSERT (close (fd + 1) == 0);
-- 
1.6.5.rc1