From 3dbe75e4eb0623e54141a53aac7bb71d602e4332 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 29 Jun 2011 15:46:50 -0600 Subject: [PATCH] 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 --- ChangeLog | 8 ++++++++ doc/glibc-functions/pipe2.texi | 4 ++++ doc/posix-functions/pipe.texi | 4 ++++ lib/pipe.c | 11 ++++++++++- lib/pipe2.c | 13 ++++++++++++- 5 files changed, 38 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4c88f8a01..ef0020675 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2011-06-29 Eric Blake + + 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 mmap-anon: do not use regular expressions inadvertently diff --git a/doc/glibc-functions/pipe2.texi b/doc/glibc-functions/pipe2.texi index 5721ae9b9..4bc1cb6eb 100644 --- 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 420e400ac..0f2c0debd 100644 --- 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 eff66cfa5..b68a555ea 100644 --- 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 f50dae665..1590deeae 100644 --- 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; } -- 2.11.0