-----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 e...@byu.net
-----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 <e...@byu.net>
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 <e...@byu.net>
---
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 <e...@byu.net>
+ 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
