pipe, pipe2: don't corrupt fd on error

I noticed a potential subtle double-close bug in libvirt.  There,
a common idiom is to initialize an int fd[2]={-1,-1}, then have
multiple error paths goto common cleanup code.  In the cleanup
code, the fds are closed if they are not already -1; this works
if the error label is reached before the pipe call, or after
pipe succeeds, but if it was the pipe call itself that jumped
to the error label, then it is relying on failed pipe() not
altering the values already in fd array prior to the failure.
Our pipe2 replacement violated this assumption, and could leave
a non-negative value in the array, which in turn would let
libvirt close an already-closed fd, possibly nuking an unrelated
fd opened by another thread that happened to get the same value.

As a result, I raised a POSIX issue regarding the behavior of
pipe on failure: http://austingroupbugs.net/view.php?id=467

Using that test program, I learned that most systems leave fd
unchanged on error, but that mingw always assigns -1 into the
array.  This fixes the mingw pipe() replacement, as well as
the gnulib pipe2() replacement.

I don't know of any race-free way to work around a cygwin crash:
http://cygwin.com/ml/cygwin/2011-06/msg00328.html - we could
always open() and then close() two fds to guess whether two
spare fd still remain before calling pipe(), but that is racy.

* lib/pipe.c (pipe): Leave fd unchanged on error.
* lib/pipe2.c (pipe2): Likewise.
* doc/posix-functions/pipe.texi (pipe): Document cygwin issue.
* doc/glibc-functions/pipe2.texi (pipe2): Likewise.

Signed-off-by: Eric Blake <eblake@redhat.com>

diff --git a/ChangeLog b/ChangeLog
index 4c88f8a01b78c1d2f3a5411afa29bb9469ad935e..ef0020675c750311bcffc181d780c51a0f77d51b 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2011-06-29  Eric Blake  <eblake@redhat.com>
+
+       pipe, pipe2: don't corrupt fd on error
+       * lib/pipe.c (pipe): Leave fd unchanged on error.
+       * lib/pipe2.c (pipe2): Likewise.
+       * doc/posix-functions/pipe.texi (pipe): Document cygwin issue.
+       * doc/glibc-functions/pipe2.texi (pipe2): Likewise.
+
 2011-06-27  Paolo Bonzini  <bonzini@gnu.org>
 
        mmap-anon: do not use regular expressions inadvertently

diff --git a/doc/glibc-functions/pipe2.texi b/doc/glibc-functions/pipe2.texi
index 5721ae9b98473c3cf1304f7f6377089057623401..4bc1cb6eb6a57b0fa3c21c7b8257f91dfaa324e6 100644 (file)
--- a/doc/glibc-functions/pipe2.texi
+++ b/doc/glibc-functions/pipe2.texi
@@ -14,6 +14,10 @@ IRIX 6.5, OSF/1 5.1, Solaris 11 2010-11, Cygwin 1.7.1, mingw, Interix 3.5, BeOS.
 
 Portability problems not fixed by Gnulib:
 @itemize
+@item
+This function crashes rather than failing with @code{EMFILE} if no
+resources are left on some platforms:
+Cygwin 1.7.9.
 @end itemize
 
 Note: This function portably supports the @code{O_NONBLOCK} flag only if the

diff --git a/doc/posix-functions/pipe.texi b/doc/posix-functions/pipe.texi
index 420e400ac8c02238b8b173a74fc1c07b22c908ac..0f2c0debd7bd9af61a087563aff234b268191058 100644 (file)
--- a/doc/posix-functions/pipe.texi
+++ b/doc/posix-functions/pipe.texi
@@ -15,4 +15,8 @@ mingw.
 
 Portability problems not fixed by Gnulib:
 @itemize
+@item
+This function crashes rather than failing with @code{EMFILE} if no
+resources are left on some platforms:
+Cygwin 1.7.9.
 @end itemize

diff --git a/lib/pipe.c b/lib/pipe.c
index eff66cfa5de9954cc059ebb1ce3bbdb0840df458..b68a555ea517f6abe81e9fc64803ecd62a08c411 100644 (file)
--- a/lib/pipe.c
+++ b/lib/pipe.c
@@ -32,7 +32,16 @@
 int
 pipe (int fd[2])
 {
-  return _pipe (fd, 4096, _O_BINARY);
+  /* Mingw changes fd to {-1,-1} on failure, but this violates
+     http://austingroupbugs.net/view.php?id=467 */
+  int tmp[2];
+  int result = _pipe (tmp, 4096, _O_BINARY);
+  if (!result)
+    {
+      fd[0] = tmp[0];
+      fd[1] = tmp[1];
+    }
+  return result;
 }
 
 #else

diff --git a/lib/pipe2.c b/lib/pipe2.c
index f50dae6650a5ce2ff54ca4856b7ea6693b7242bd..1590deeae365c960dfd5a66e32899cf5b0f61031 100644 (file)
--- a/lib/pipe2.c
+++ b/lib/pipe2.c
@@ -40,6 +40,11 @@
 int
 pipe2 (int fd[2], int flags)
 {
+  /* Mingw _pipe() corrupts fd on failure; also, if we succeed at
+     creating the pipe but later fail at changing fcntl, we want
+     to leave fd unchanged: http://austingroupbugs.net/view.php?id=467  */
+  int tmp[2] = { fd[0], fd[1] };
+
 #if HAVE_PIPE2
 # undef pipe2
   /* Try the system call first, if it exists.  (We may be running with a glibc
@@ -71,7 +76,11 @@ pipe2 (int fd[2], int flags)
 /* Native Woe32 API.  */
 
   if (_pipe (fd, 4096, flags & ~O_NONBLOCK) < 0)
-    return -1;
+    {
+      fd[0] = tmp[0];
+      fd[1] = tmp[1];
+      return -1;
+    }
 
   /* O_NONBLOCK handling.
      On native Windows platforms, O_NONBLOCK is defined by gnulib.  Use the
@@ -145,6 +154,8 @@ pipe2 (int fd[2], int flags)
     int saved_errno = errno;
     close (fd[0]);
     close (fd[1]);
+    fd[0] = tmp[0];
+    fd[1] = tmp[1];
     errno = saved_errno;
     return -1;
   }