[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: popen bugs, popen_safer
From: |
Eric Blake |
Subject: |
Re: popen bugs, popen_safer |
Date: |
Thu, 20 Aug 2009 18:16:53 -0600 |
User-agent: |
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.22) Gecko/20090605 Thunderbird/2.0.0.22 Mnenhy/0.7.6.666 |
-----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 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/
iEYEARECAAYFAkqN53UACgkQ84KuGfSFAYCIAwCcD7B2TuDaHD1eWqOXMVTHdlnb
LcgAn3TVx9tGfV23DaTKGPUXdCOP1bmO
=7r4a
-----END PGP SIGNATURE-----
From 1b3a58de6630d88009c1ef06ed154249fb2e9d6b Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
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 <address@hidden>
---
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 <address@hidden>
+
+ 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 <address@hidden>
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
- Re: O_SAFER (was: fcntl module), (continued)
- Re: fcntl module, Bruno Haible, 2009/08/24
- [PATCH] Move more flags to lib/fcntl.in.h, Paolo Bonzini, 2009/08/20
- Re: [PATCH] Move more flags to lib/fcntl.in.h, Eric Blake, 2009/08/20
- Re: [PATCH] Move more flags to lib/fcntl.in.h, Bruno Haible, 2009/08/22
- Re: O_CLOEXEC support (was: popen bugs, popen_safer), Bruno Haible, 2009/08/22
- Re: O_CLOEXEC support, Paolo Bonzini, 2009/08/22
Re: popen bugs, popen_safer, Bruno Haible, 2009/08/20