-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 According to Bruno Haible on 8/20/2009 2:14 AM: > Hi Eric, > >> I went ahead and implemented popen_safer. > > Well done! > >> +/* Get the original definition of popen. It might be defined as a macro. >> */ > > This is not needed. I'm not aware of any platform that defines popen as a > macro:
OK, simplified as follows. >> +#define __need_FILE >> +# include <stdio.h> >> +#undef __need_FILE > > This looks as it could interfere with glibc headers. Can you use a > gnulib-defined > macro name, such as __need_system_stdio_h, instead? This matches what fopen used, but it is dead code now that I applied the simplification of not having to drill through an original popen macro. > Maybe add some comments, what this code is trying to do? Took me a while to > understand it. Also done. > > This is becoming confusing: Why should the 'fopen' test suddenly test > fopen_safer, > not fopen?? fopen_safer is supposed to wrap fopen, so both are being tested. But I agree with your idea to split things, so I will work on that next when I have a chance. >> +#include "cloexec.h" > > I don't see where cloexec.h is defined in your patch 3/3. Provided by making the popen-safer module depend on the cloexec module. - -- Don't work too hard, make some time for fun as well! Eric Blake [email protected] -----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/ iEYEARECAAYFAkqN53UACgkQ84KuGfSFAYCIAwCcD7B2TuDaHD1eWqOXMVTHdlnb LcgAn3TVx9tGfV23DaTKGPUXdCOP1bmO =7r4a -----END PGP SIGNATURE-----
From 1b3a58de6630d88009c1ef06ed154249fb2e9d6b Mon Sep 17 00:00:00 2001 From: Eric Blake <[email protected]> Date: Thu, 20 Aug 2009 18:14:41 -0600 Subject: [PATCH] popen: simplify access to original popen * lib/popen.c (rpl_popen): No need to worry about popen being a macro. Reported by Bruno Haible. Signed-off-by: Eric Blake <[email protected]> --- ChangeLog | 7 +++++++ lib/popen.c | 25 +++++++++++++------------ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/ChangeLog b/ChangeLog index 23a9238..580212c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2009-08-21 Eric Blake <[email protected]> + + popen: simplify access to original popen + * lib/popen.c (rpl_popen): No need to worry about popen being a + macro. + Reported by Bruno Haible. + 2009-08-20 Eric Blake <[email protected]> build: avoid some compiler warnings diff --git a/lib/popen.c b/lib/popen.c index a1f1d45..92a8050 100644 --- a/lib/popen.c +++ b/lib/popen.c @@ -18,17 +18,6 @@ #include <config.h> -/* Get the original definition of popen. It might be defined as a macro. */ -#define __need_FILE -# include <stdio.h> -#undef __need_FILE - -static inline FILE * -orig_popen (const char *filename, const char *mode) -{ - return popen (filename, mode); -} - /* Specification. */ #include <stdio.h> @@ -37,6 +26,8 @@ orig_popen (const char *filename, const char *mode) #include <stdlib.h> #include <unistd.h> +#undef popen + FILE * rpl_popen (const char *filename, const char *mode) { @@ -56,6 +47,15 @@ rpl_popen (const char *filename, const char *mode) int cloexec1 = fcntl (STDOUT_FILENO, F_GETFD); int saved_errno; + /* If either stdin or stdout was closed (that is, fcntl failed), + then we open a dummy close-on-exec fd to occupy that slot. That + way, popen's internal use of pipe() will not contain either fd 0 + or 1, overcoming the fact that the child process blindly calls + close() on the parent's end of the pipe without first checking + whether it is clobbering the fd just placed there via dup2(); the + exec will get rid of the dummy fd's in the child. Fortunately, + closed stderr in the parent does not cause problems in the + child. */ if (cloexec0 < 0) { if (open ("/dev/null", O_RDONLY) != STDIN_FILENO @@ -70,7 +70,8 @@ rpl_popen (const char *filename, const char *mode) fcntl (STDOUT_FILENO, F_GETFD) | FD_CLOEXEC) == -1) abort (); } - result = orig_popen (filename, mode); + result = popen (filename, mode); + /* Now, close any dummy fd's created in the parent. */ saved_errno = errno; if (cloexec0 < 0) close (STDIN_FILENO); -- 1.6.3.3.334.g916e1
