bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: popen bugs, popen_safer


From: Bruno Haible
Subject: Re: popen bugs, popen_safer
Date: Thu, 20 Aug 2009 10:14:03 +0200
User-agent: KMail/1.9.9

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:
$ LC_ALL=C fgrep popen */usr/include/stdio.h
aix-4.3.2/usr/include/stdio.h:extern FILE       *popen();
aix-4.3.2/usr/include/stdio.h:extern FILE       *popen(const char *, const char 
*);
aix-5.1.0/usr/include/stdio.h:extern FILE       *popen();
aix-5.1.0/usr/include/stdio.h:extern FILE       *popen(const char *, const char 
*);
aix-5.3.0/usr/include/stdio.h:extern FILE       *popen();
aix-5.3.0/usr/include/stdio.h:extern FILE       *popen(const char *, const char 
*);
beos/usr/include/stdio.h:FILE   *popen(const char *cmd, const char *type);
cygwin/usr/include/stdio.h:FILE *  _EXFUN(popen, (const char *, const char *));
freebsd-5.2.1/usr/include/stdio.h:FILE  *popen(const char *, const char *);
freebsd-6.0/usr/include/stdio.h:FILE    *popen(const char *, const char *);
glibc-2.3.6/usr/include/stdio.h:extern FILE *popen (__const char *__command, 
__const char *__modes);
glibc-2.3.6/usr/include/stdio.h:/* Close a stream opened by popen and return 
the status of its child.
haiku/usr/include/stdio.h:extern FILE           *popen(const char *command, 
const char *mode);
hpux-10.20/usr/include/stdio.h:     extern FILE *popen(const char *, const char 
*);
hpux-10.20/usr/include/stdio.h:     extern FILE *popen();
hpux-11.00/usr/include/stdio.h:     extern __FILE *popen(const char *, const 
char *);
hpux-11.00/usr/include/stdio.h:     extern FILE *popen();
hpux-11.11/usr/include/stdio.h:#    pragma HP_DEFINED_EXTERNAL 
getw,putw,pclose,popen,tempnam
hpux-11.11/usr/include/stdio.h:#    pragma HP_NO_RELOCATION 
getw,putw,pclose,popen,tempnam
hpux-11.11/usr/include/stdio.h:#    pragma HP_LONG_RETURN 
getw,putw,pclose,popen,tempnam
hpux-11.11/usr/include/stdio.h:     extern __FILE *popen(const char *, const 
char *);
hpux-11.11/usr/include/stdio.h:     extern FILE *popen();
hpux-11.23-ia64/usr/include/stdio.h:#      pragma extern getw, putw, pclose, 
popen, tempnam
hpux-11.23-ia64/usr/include/stdio.h:#    pragma HP_DEFINED_EXTERNAL 
getw,putw,pclose,popen,tempnam
hpux-11.23-ia64/usr/include/stdio.h:#    pragma HP_NO_RELOCATION 
getw,putw,pclose,popen,tempnam
hpux-11.23-ia64/usr/include/stdio.h:#    pragma HP_LONG_RETURN 
getw,putw,pclose,popen,tempnam
hpux-11.23-ia64/usr/include/stdio.h:     extern __FILE *popen(const char *, 
const char *);
hpux-11.23-ia64/usr/include/stdio.h:     extern FILE *popen();
interix-3.5/usr/include/stdio.h:FILE* __cdecl popen (const char *, const char 
*);
irix-6.5/usr/include/stdio.h:extern FILE        *popen(const char *, const char 
*);
macosx-10.3/usr/include/stdio.h:FILE    *popen(const char *, const char *);
mingw/usr/include/stdio.h:_CRTIMP FILE* __cdecl _popen (const char*, const 
char*);
mingw/usr/include/stdio.h:_CRTIMP FILE* __cdecl popen (const char*, const 
char*);
mingw/usr/include/stdio.h:_CRTIMP FILE* __cdecl _wpopen (const wchar_t*, const 
wchar_t*);
mingw/usr/include/stdio.h:_CRTIMP FILE* __cdecl wpopen (const wchar_t*, const 
wchar_t*);
mirbsd-10/usr/include/stdio.h:FILE      *popen(const char *, const char *);
netbsd-3.0/usr/include/stdio.h:FILE     *popen(const char *, const char *);
openbsd-3.8/usr/include/stdio.h:FILE    *popen(const char *, const char *);
osf1-4.0d/usr/include/stdio.h:extern FILE       *popen __((const char *, const 
char *));
osf1-5.1a/usr/include/stdio.h:extern FILE       *popen __((const char *, const 
char *));
pips/usr/include/stdio.h:FILE   *popen(const char *, const char *);
pips/usr/include/stdio.h:int popen3(const char *file, const char *cmd, char** 
envp, int fds[3]);
qnx/usr/include/stdio.h:extern _CSTD FILE     *popen( const char *__filename, 
const char *__mode );
solaris-2.10/usr/include/stdio.h:extern FILE    *popen(const char *, const char 
*);
solaris-2.10/usr/include/stdio.h:extern FILE    *popen();

> +#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?

> +  if (cloexec0 < 0)
> +    {
> +      if (open ("/dev/null", O_RDONLY) != STDIN_FILENO
> +       || fcntl (STDIN_FILENO, F_SETFD,
> +                 fcntl (STDIN_FILENO, F_GETFD) | FD_CLOEXEC) == -1)
> +     abort ();
> +    }
> +  if (cloexec1 < 0)
> +    {
> +      if (open ("/dev/null", O_RDONLY) != STDOUT_FILENO
> +       || fcntl (STDOUT_FILENO, F_SETFD,
> +                 fcntl (STDOUT_FILENO, F_GETFD) | FD_CLOEXEC) == -1)
> +     abort ();
> +    }

Maybe add some comments, what this code is trying to do? Took me a while to
understand it.

> diff --git a/tests/test-fopen.c b/tests/test-fopen.c
> index 337a389..b9e3694 100644
> --- a/tests/test-fopen.c
> +++ b/tests/test-fopen.c
> @@ -21,6 +21,10 @@
>  #include <stdio.h>
>  #include <stdlib.h>
> 
> +#if GNULIB_FOPEN_SAFER
> +# include "stdio--.h"
> +#endif
> +
>  #define ASSERT(expr) \
>    do                                                                      \
>      {                                                                        
>      \

This is becoming confusing: Why should the 'fopen' test suddenly test 
fopen_safer,
not fopen??

I would find it better to introduce a new test module fopen-safer-tests, that 
shares
the bulk of the code with fopen-tests. File structure:
  test-fopen.h : all common code, starting with the definition of ASSERT,
  test-fopen.c : just the includes and #include "test-fopen.h"
  test-fopen-safer.c : just the includes, then #include "stdio--.h", then 
#include "test-fopen.h"

See test-snprintf-posix.h, test-snprintf-posix.c, test-vsnprintf-posix.c for
an example of this coding pattern.

Likewise for test-open.c and fcntl--.h.

Likewise for test-popen.c and stdio--.h.

> +#include "cloexec.h"

I don't see where cloexec.h is defined in your patch 3/3.

Bruno




reply via email to

[Prev in Thread] Current Thread [Next in Thread]