RFA: close-on-exec internal file descriptors

RFA: close-on-exec internal file descriptors

[Tom Tromey]

A few weeks ago, someone on irc pointed out that gdb leaks file
descriptors to its child processes.  One simple way to see this is to
start gdb with an exec and then shell out:

    opsy. gdb /tmp/r
    [...]
    (gdb) shell ls -l /proc/$$/fd
    total 0
    lrwx------ 1 tromey tromey 64 2008-12-05 17:10 0 -> /dev/pts/1
    lrwx------ 1 tromey tromey 64 2008-12-05 17:10 1 -> /dev/pts/1
    lrwx------ 1 tromey tromey 64 2008-12-05 17:10 2 -> /dev/pts/1
    lr-x------ 1 tromey tromey 64 2008-12-05 17:10 3 -> pipe:[1100229]
    l-wx------ 1 tromey tromey 64 2008-12-05 17:10 4 -> pipe:[1100229]
    lr-x------ 1 tromey tromey 64 2008-12-05 17:10 5 -> /proc/8096/fd

I believe those 'pipe' entries are from the call to pipe in
linux-nat.c:linux_nat_set_async.

This patch fixes this problem by introducing new wrapper functions
which create close-on-exec file descriptors.  Then, these functions
are used everywhere in gdb.  After this patch, these wrapper functions
should be used in all new code as well.

I chose to take advantage of the new glibc flags like O_CLOEXEC when
they are available.  This is friendlier in the Python case -- here,
gdb might have multiple threads, and the glibc flags enable us to
avoid a window where a file descriptor is not marked close-on-exec.
If the new flags or functions are not available, we fall back to
fcntl.

If you're following the binutils list, you may note that I did not
take this approach in libbfd.  I plan to submit a follow-up patch
there, because I now believe this approach to be better.

Built & regtested on x86-64 (compile farm).  I believe that machine
does not have the O_CLOEXEC support.  My machine here does, though.

I didn't write a new test case.  I don't think my build includes
remote-mips.c, so that has not actually been compiled.  Also, I did
not touch gdbserver.

Please review.

thanks,
Tom

2008-11-20  Tom Tromey  <tromey@redhat.com>

	* configure, config.in: Rebuild.
	* configure.ac: Check for fileno, pipe2.
	* xml-tdesc.c (fetch_xml_from_file): Use fopen_cloexec.
	* tracepoint.c (tracepoint_save_command): Use fopen_cloexec.
	* remote.c (remote_file_put): Use fopen_cloexec.
	(remote_file_get): Likewise.
	* remote-mips.c (pmon_start_download): Use fopen_cloexec.
	* defs.h (open_cloexec, fopen_cloexec): Declare.
	(pipe_cloexec): Likewise.
	* ser-tcp.c (net_open): Use socket_cloexec.
	(socket_cloexec): New function.
	* ser-unix.c (hardwire_open): Use open_cloexec.
	* remote-fileio.c (remote_fileio_func_open): Use open_cloexec.
	* cli/cli-dump.c (fopen_with_cleanup): Use fopen_cloexec.
	* ui-file.c (gdb_fopen): Use fopen_cloexec
	* tui/tui-io.c (tui_initialize_io): Use pipe_cloexec.
	* source.c (openp): Use open_cloexec.
	(find_and_open_source): Likewise.
	* solib.c (solib_bfd_open): Use open_cloexec.
	* linux-nat.c (linux_nat_setup_async): Use pipe_cloexec.
	(pid_is_stopped): Use fopen_cloexec.
	(linux_nat_find_memory_regions): Likewise.
	(linux_nat_info_proc_cmd): Likewise.
	(linux_proc_pending_signals): Likewise.
	* corelow.c (core_open): Use open_cloexec.
	* utils.c (FD_CLOEXEC): New define.
	(open_cloexec): New function.
	(fopen_cloexec): Likewise.
	(pipe_cloexec): Likewise.
	(close_on_exec): Likewise.

diff --git a/gdb/cli/cli-dump.c b/gdb/cli/cli-dump.c
index d5dbc97..ba78202 100644
--- a/gdb/cli/cli-dump.c
+++ b/gdb/cli/cli-dump.c
@@ -105,7 +105,7 @@ scan_filename_with_cleanup (char **cmd, const char *defname)
 FILE *
 fopen_with_cleanup (const char *filename, const char *mode)
 {
-  FILE *file = fopen (filename, mode);
+  FILE *file = fopen_cloexec (filename, mode);
   if (file == NULL)
     perror_with_name (filename);
   make_cleanup_fclose (file);
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 8725aa6..0b046f6 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -775,7 +775,7 @@ AC_FUNC_VFORK
 AC_CHECK_FUNCS([canonicalize_file_name realpath getrusage getuid \
                 getgid poll pread64 sbrk setpgid setpgrp setsid \
 		sigaction sigprocmask sigsetmask socketpair syscall \
-		ttrace wborder])
+		ttrace wborder pipe2 fileno])
 
 # Check the return and argument types of ptrace.  No canned test for
 # this, so roll our own.
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 35c998c..c03a130 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -300,7 +300,7 @@ core_open (char *filename, int from_tty)
     flags |= O_RDWR;
   else
     flags |= O_RDONLY;
-  scratch_chan = open (filename, flags, 0);
+  scratch_chan = open_cloexec (filename, flags, 0);
   if (scratch_chan < 0)
     perror_with_name (filename);
 
diff --git a/gdb/defs.h b/gdb/defs.h
index b047266..47516b3 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -409,6 +409,14 @@ char *ldirname (const char *filename);
 
 char **gdb_buildargv (const char *);
 
+int open_cloexec (const char *, int, int);
+
+int pipe_cloexec (int[2]);
+
+FILE *fopen_cloexec (const char *, const char *);
+
+void close_on_exec (int);
+
 /* From demangle.c */
 
 extern void set_demangling_style (char *);
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 913bfec..6c45bb7 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1152,7 +1152,7 @@ pid_is_stopped (pid_t pid)
   int retval = 0;
 
   snprintf (buf, sizeof (buf), "/proc/%d/status", (int) pid);
-  status_file = fopen (buf, "r");
+  status_file = fopen_cloexec (buf, "r");
   if (status_file != NULL)
     {
       int have_state = 0;
@@ -3339,7 +3339,7 @@ linux_nat_find_memory_regions (int (*func) (CORE_ADDR,
 
   /* Compose the filename for the /proc memory map, and open it.  */
   sprintf (mapsfilename, "/proc/%lld/maps", pid);
-  if ((mapsfile = fopen (mapsfilename, "r")) == NULL)
+  if ((mapsfile = fopen_cloexec (mapsfilename, "r")) == NULL)
     error (_("Could not open %s."), mapsfilename);
   cleanup = make_cleanup_fclose (mapsfile);
 
@@ -3663,7 +3663,7 @@ linux_nat_info_proc_cmd (char *args, int from_tty)
   if (cmdline_f || all)
     {
       sprintf (fname1, "/proc/%lld/cmdline", pid);
-      if ((procfile = fopen (fname1, "r")) != NULL)
+      if ((procfile = fopen_cloexec (fname1, "r")) != NULL)
 	{
 	  struct cleanup *cleanup = make_cleanup_fclose (procfile);
 	  fgets (buffer, sizeof (buffer), procfile);
@@ -3694,7 +3694,7 @@ linux_nat_info_proc_cmd (char *args, int from_tty)
   if (mappings_f || all)
     {
       sprintf (fname1, "/proc/%lld/maps", pid);
-      if ((procfile = fopen (fname1, "r")) != NULL)
+      if ((procfile = fopen_cloexec (fname1, "r")) != NULL)
 	{
 	  long long addr, endaddr, size, offset, inode;
 	  char permissions[8], device[8], filename[MAXPATHLEN];
@@ -3756,7 +3756,7 @@ linux_nat_info_proc_cmd (char *args, int from_tty)
   if (status_f || all)
     {
       sprintf (fname1, "/proc/%lld/status", pid);
-      if ((procfile = fopen (fname1, "r")) != NULL)
+      if ((procfile = fopen_cloexec (fname1, "r")) != NULL)
 	{
 	  struct cleanup *cleanup = make_cleanup_fclose (procfile);
 	  while (fgets (buffer, sizeof (buffer), procfile) != NULL)
@@ -3769,7 +3769,7 @@ linux_nat_info_proc_cmd (char *args, int from_tty)
   if (stat_f || all)
     {
       sprintf (fname1, "/proc/%lld/stat", pid);
-      if ((procfile = fopen (fname1, "r")) != NULL)
+      if ((procfile = fopen_cloexec (fname1, "r")) != NULL)
 	{
 	  int itmp;
 	  char ctmp;
@@ -3966,7 +3966,7 @@ linux_proc_pending_signals (int pid, sigset_t *pending, sigset_t *blocked, sigse
   sigemptyset (blocked);
   sigemptyset (ignored);
   sprintf (fname, "/proc/%d/status", pid);
-  procfile = fopen (fname, "r");
+  procfile = fopen_cloexec (fname, "r");
   if (procfile == NULL)
     error (_("Could not open %s"), fname);
   cleanup = make_cleanup_fclose (procfile);
@@ -4552,7 +4552,7 @@ linux_nat_get_siginfo (ptid_t ptid)
 static void
 linux_nat_setup_async (void)
 {
-  if (pipe (linux_nat_event_pipe) == -1)
+  if (pipe_cloexec (linux_nat_event_pipe) == -1)
     internal_error (__FILE__, __LINE__,
 		    "creating event pipe failed.");
   fcntl (linux_nat_event_pipe[0], F_SETFL, O_NONBLOCK);
diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c
index 05a78be..65d01fd 100644
--- a/gdb/remote-fileio.c
+++ b/gdb/remote-fileio.c
@@ -655,7 +655,7 @@ remote_fileio_func_open (char *buf)
     }
 
   remote_fio_no_longjmp = 1;
-  fd = open (pathname, flags, mode);
+  fd = open_cloexec (pathname, flags, mode);
   if (fd < 0)
     {
       remote_fileio_return_errno (-1);
diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c
index b4423d5..bfb1da8 100644
--- a/gdb/remote-mips.c
+++ b/gdb/remote-mips.c
@@ -2992,7 +2992,7 @@ pmon_start_download (void)
   if (tftp_in_use)
     {
       /* Create the temporary download file.  */
-      if ((tftp_file = fopen (tftp_localname, "w")) == NULL)
+      if ((tftp_file = fopen_cloexec (tftp_localname, "w")) == NULL)
 	perror_with_name (tftp_localname);
     }
   else
diff --git a/gdb/remote.c b/gdb/remote.c
index 06ab783..b44bdd7 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8418,7 +8418,7 @@ remote_file_put (const char *local_file, const char *remote_file, int from_tty)
   if (!remote_desc)
     error (_("command can only be used with remote target"));
 
-  file = fopen (local_file, "rb");
+  file = fopen_cloexec (local_file, "rb");
   if (file == NULL)
     perror_with_name (local_file);
   back_to = make_cleanup_fclose (file);
@@ -8508,7 +8508,7 @@ remote_file_get (const char *remote_file, const char *local_file, int from_tty)
   if (fd == -1)
     remote_hostio_error (remote_errno);
 
-  file = fopen (local_file, "wb");
+  file = fopen_cloexec (local_file, "wb");
   if (file == NULL)
     perror_with_name (local_file);
   back_to = make_cleanup_fclose (file);
diff --git a/gdb/ser-tcp.c b/gdb/ser-tcp.c
index dcf288d..8f51f45 100644
--- a/gdb/ser-tcp.c
+++ b/gdb/ser-tcp.c
@@ -61,6 +61,20 @@ void _initialize_ser_tcp (void);
 /* how many times per second to poll deprecated_ui_loop_hook */
 #define POLL_INTERVAL 2
 
+/* Like socket, but ensure that the resulting file descriptor is
+   marked close-on-exec, if possible.  */
+static int
+socket_cloexec (int domain, int type, int protocol)
+{
+#ifdef SOCK_CLOEXEC
+  return socket (domain, type | SOCK_CLOEXEC, protocol);
+#else
+  int result = socket (domain, type, protocol);
+  close_on_exec (result);
+  return result;
+#endif
+}
+
 /* Open a tcp socket */
 
 int
@@ -109,9 +123,9 @@ net_open (struct serial *scb, const char *name)
     }
 
   if (use_udp)
-    scb->fd = socket (PF_INET, SOCK_DGRAM, 0);
+    scb->fd = socket_cloexec (PF_INET, SOCK_DGRAM, 0);
   else
-    scb->fd = socket (PF_INET, SOCK_STREAM, 0);
+    scb->fd = socket_cloexec (PF_INET, SOCK_STREAM, 0);
 
   if (scb->fd < 0)
     return -1;
diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
index 679d23e..4e920f2 100644
--- a/gdb/ser-unix.c
+++ b/gdb/ser-unix.c
@@ -108,7 +108,7 @@ void _initialize_ser_hardwire (void);
 static int
 hardwire_open (struct serial *scb, const char *name)
 {
-  scb->fd = open (name, O_RDWR);
+  scb->fd = open_cloexec (name, O_RDWR, 0);
   if (scb->fd < 0)
     return -1;
 
diff --git a/gdb/solib.c b/gdb/solib.c
index d04a907..acc8bdd 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -194,7 +194,7 @@ solib_bfd_open (char *in_pathname)
     }
 
   /* Now see if we can open it.  */
-  found_file = open (temp_pathname, O_RDONLY | O_BINARY, 0);
+  found_file = open_cloexec (temp_pathname, O_RDONLY | O_BINARY, 0);
 
   /* We try to find the library in various ways.  After each attempt
      (except for the one above), either found_file >= 0 and
diff --git a/gdb/source.c b/gdb/source.c
index c41c193..fbab97e 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -708,7 +708,7 @@ openp (const char *path, int opts, const char *string,
 	{
 	  filename = alloca (strlen (string) + 1);
 	  strcpy (filename, string);
-	  fd = open (filename, mode, prot);
+	  fd = open_cloexec (filename, mode, prot);
 	  if (fd >= 0)
 	    goto done;
 	}
@@ -775,7 +775,7 @@ openp (const char *path, int opts, const char *string,
 
       if (is_regular_file (filename))
 	{
-	  fd = open (filename, mode);
+	  fd = open_cloexec (filename, mode, 0);
 	  if (fd >= 0)
 	    break;
 	}
@@ -964,7 +964,7 @@ find_and_open_source (struct objfile *objfile,
           *fullname = rewritten_fullname;
         }
 
-      result = open (*fullname, OPEN_MODE);
+      result = open_cloexec (*fullname, OPEN_MODE, 0);
       if (result >= 0)
 	return result;
       /* Didn't work -- free old one, try again. */
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 5c8c205..a397bb1 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2307,7 +2307,7 @@ tracepoint_save_command (char *args, int from_tty)
 
   pathname = tilde_expand (args);
   cleanup = make_cleanup (xfree, pathname);
-  if (!(fp = fopen (pathname, "w")))
+  if (!(fp = fopen_cloexec (pathname, "w")))
     error (_("Unable to open file '%s' for saving tracepoints (%s)"),
 	   args, safe_strerror (errno));
   make_cleanup_fclose (fp);
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index e25263c..3636f51 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -617,7 +617,7 @@ tui_initialize_io (void)
   /* Temporary solution for readline writing to stdout: redirect
      readline output in a pipe, read that pipe and output the content
      in the curses command window.  */
-  if (pipe (tui_readline_pipe) != 0)
+  if (pipe_cloexec (tui_readline_pipe) != 0)
     {
       fprintf_unfiltered (gdb_stderr, "Cannot create pipe for readline");
       exit (1);
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 9a1d892..ad757be 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -515,7 +515,7 @@ stdio_fileopen (FILE *file)
 struct ui_file *
 gdb_fopen (char *name, char *mode)
 {
-  FILE *f = fopen (name, mode);
+  FILE *f = fopen_cloexec (name, mode);
   if (f == NULL)
     return NULL;
   return stdio_file_new (f, 1);
diff --git a/gdb/utils.c b/gdb/utils.c
index d14009f..2324fb0 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -66,6 +66,11 @@
 
 #include <sys/time.h>
 #include <time.h>
+#include <fcntl.h>
+
+#ifndef FD_CLOEXEC
+#define FD_CLOEXEC 1
+#endif
 
 #if !HAVE_DECL_MALLOC
 extern PTR malloc ();		/* OK: PTR */
@@ -3441,3 +3446,73 @@ gdb_buildargv (const char *s)
     nomem (0);
   return argv;
 }
+
+/* A helper function to mark a file descriptor close-on-exec.  Only
+   call this if you cannot mark the file descriptor close-on-exec at
+   creation.  */
+void
+close_on_exec (int fd)
+{
+#ifdef F_GETFD
+  if (fd >= 0)
+    {
+      int old = fcntl (fd, F_GETFD, 0);
+      if (old >= 0)
+	fcntl (fd, F_SETFD, old | FD_CLOEXEC);
+    }
+#endif
+}
+
+/* Like open, but ensure that the resulting file descriptor is marked
+   close-on-exec, if possible.  */
+int
+open_cloexec (const char *path, int flags, int mode)
+{
+#ifdef O_CLOEXEC
+  return open (path, flags | O_CLOEXEC, mode);
+#else
+  int fd = open (path, flags, mode);
+  close_on_exec (fd);
+  return fd;
+#endif
+}
+
+/* Like fopen, but ensure that the resulting file is marked
+   close-on-exec, if possible.  */
+FILE *
+fopen_cloexec (const char *path, const char *mode)
+{
+#ifdef O_CLOEXEC
+  /* We assume that O_CLOEXEC also implies the availability of the "e"
+     flag to fopen.  */
+  char new_mode[20];
+  strcpy (new_mode, mode);
+  strcat (new_mode, "e");
+  return fopen (path, new_mode);
+#else
+  FILE *result = fopen (path, mode);
+#if defined (HAVE_FILENO)
+  if (result)
+    close_on_exec (fileno (result));
+#endif
+  return result;
+#endif
+}
+
+/* Like pipe, but ensure that the resulting file descriptors are
+   marked close-on-exec, if possible.  */
+int
+pipe_cloexec (int fds[2])
+{
+#if defined (HAVE_PIPE2) && defined (O_CLOEXEC)
+  return pipe2 (fds, O_CLOEXEC);
+#else
+  int result = pipe (fds);
+  if (result != -1)
+    {
+      close_on_exec (fds[0]);
+      close_on_exec (fds[1]);
+    }
+  return result;
+#endif
+}
diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c
index 2e8c1f5..eeb85e0 100644
--- a/gdb/xml-tdesc.c
+++ b/gdb/xml-tdesc.c
@@ -438,11 +438,11 @@ fetch_xml_from_file (const char *filename, void *baton)
       char *fullname = concat (dirname, "/", filename, (char *) NULL);
       if (fullname == NULL)
 	nomem (0);
-      file = fopen (fullname, FOPEN_RT);
+      file = fopen_cloexec (fullname, FOPEN_RT);
       xfree (fullname);
     }
   else
-    file = fopen (filename, FOPEN_RT);
+    file = fopen_cloexec (filename, FOPEN_RT);
 
   if (file == NULL)
     return NULL;

Re: RFA: close-on-exec internal file descriptors

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Date: Fri, 05 Dec 2008 17:38:13 -0700
> 
> This patch fixes this problem by introducing new wrapper functions
> which create close-on-exec file descriptors.  Then, these functions
> are used everywhere in gdb.  After this patch, these wrapper functions
> should be used in all new code as well.

Thanks.

>     opsy. gdb /tmp/r
>     [...]
>     (gdb) shell ls -l /proc/$$/fd
>     total 0
>     lrwx------ 1 tromey tromey 64 2008-12-05 17:10 0 -> /dev/pts/1
>     lrwx------ 1 tromey tromey 64 2008-12-05 17:10 1 -> /dev/pts/1
>     lrwx------ 1 tromey tromey 64 2008-12-05 17:10 2 -> /dev/pts/1
>     lr-x------ 1 tromey tromey 64 2008-12-05 17:10 3 -> pipe:[1100229]
>     l-wx------ 1 tromey tromey 64 2008-12-05 17:10 4 -> pipe:[1100229]
>     lr-x------ 1 tromey tromey 64 2008-12-05 17:10 5 -> /proc/8096/fd
> 
> I believe those 'pipe' entries are from the call to pipe in
> linux-nat.c:linux_nat_set_async.

Are you saying that the problem is specific to Linux native targets?
If so, why the solution invades general source files such as remote.c,
ser-tcp.c, ui-file.c, source.c and even remote-mips.c?

> I chose to take advantage of the new glibc flags like O_CLOEXEC when
> they are available.

Relying on glibc is OK for GNU/Linux, but you seem to be modifying
files that have no relation to the Linux native builds.  Does that
mean the non-glibc builds that don't have the support you are relying
on will still leak descriptors?

> +FILE *
> +fopen_cloexec (const char *path, const char *mode)
> +{
> +#ifdef O_CLOEXEC
> +  /* We assume that O_CLOEXEC also implies the availability of the "e"
> +     flag to fopen.  */
> +  char new_mode[20];
> +  strcpy (new_mode, mode);
> +  strcat (new_mode, "e");
> +  return fopen (path, new_mode);

Can we do something more safe than this arbitrary [20] limitation?

Re: RFA: close-on-exec internal file descriptors

[Daniel Jacobowitz]

On Fri, Dec 05, 2008 at 05:38:13PM -0700, Tom Tromey wrote:
> +/* Like open, but ensure that the resulting file descriptor is marked
> +   close-on-exec, if possible.  */
> +int
> +open_cloexec (const char *path, int flags, int mode)
> +{
> +#ifdef O_CLOEXEC
> +  return open (path, flags | O_CLOEXEC, mode);

If you build with headers that include O_CLOEXEC support, but the
running kernel is too old, will this fail to open entirely?  Or are
unknown open flags ignored?

Also, WDYT about separating the goal from the mechanism?  With this
patch, every bit of gdb knows "we close file descriptors on exec";
if it was gdb_open, it would just be "gdb-specific version of open".

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: close-on-exec internal file descriptors

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Fri, 05 Dec 2008 17:38:13 -0700
> 
> A few weeks ago, someone on irc pointed out that gdb leaks file
> descriptors to its child processes.  One simple way to see this is to
> start gdb with an exec and then shell out:
> 
>     opsy. gdb /tmp/r
>     [...]
>     (gdb) shell ls -l /proc/$$/fd
>     total 0
>     lrwx------ 1 tromey tromey 64 2008-12-05 17:10 0 -> /dev/pts/1
>     lrwx------ 1 tromey tromey 64 2008-12-05 17:10 1 -> /dev/pts/1
>     lrwx------ 1 tromey tromey 64 2008-12-05 17:10 2 -> /dev/pts/1
>     lr-x------ 1 tromey tromey 64 2008-12-05 17:10 3 -> pipe:[1100229]
>     l-wx------ 1 tromey tromey 64 2008-12-05 17:10 4 -> pipe:[1100229]
>     lr-x------ 1 tromey tromey 64 2008-12-05 17:10 5 -> /proc/8096/fd
> 
> I believe those 'pipe' entries are from the call to pipe in
> linux-nat.c:linux_nat_set_async.
> 
> This patch fixes this problem by introducing new wrapper functions
> which create close-on-exec file descriptors.  Then, these functions
> are used everywhere in gdb.  After this patch, these wrapper functions
> should be used in all new code as well.
> 
> I chose to take advantage of the new glibc flags like O_CLOEXEC when
> they are available.  This is friendlier in the Python case -- here,
> gdb might have multiple threads, and the glibc flags enable us to
> avoid a window where a file descriptor is not marked close-on-exec.
> If the new flags or functions are not available, we fall back to
> fcntl.

Sorry, but I don't see the point in having #ifdef O_CLOEXEC code when
there is a perfectly portable way to do this using fcntl.  It leads to
more bits of code that can possibly go untested.

I also think it would actually be better to explicitly close file
descriptors before doing an exec instead of relying on people to use
the proper _cloexec call throughout gdb.

Re: RFA: close-on-exec internal file descriptors

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> I believe those 'pipe' entries are from the call to pipe in
>> linux-nat.c:linux_nat_set_async.

Eli> Are you saying that the problem is specific to Linux native targets?

No.  That was an example.

>> I chose to take advantage of the new glibc flags like O_CLOEXEC when
>> they are available.

Eli> Relying on glibc is OK for GNU/Linux, but you seem to be modifying
Eli> files that have no relation to the Linux native builds.  Does that
Eli> mean the non-glibc builds that don't have the support you are relying
Eli> on will still leak descriptors?

No.  In each case, if O_CLOEXEC is not available, we fall back to the
fcntl-based method.  That is what the calls to "close_on_exec" do in
the patch.

>> +  char new_mode[20];
>> +  strcpy (new_mode, mode);
>> +  strcat (new_mode, "e");
>> +  return fopen (path, new_mode);

Eli> Can we do something more safe than this arbitrary [20] limitation?

I don't think there is any point, because the mode argument to fopen
is never longer than 4 characters or so.  However, if you really want
this, I will change it to a malloc.

Tom

Re: RFA: close-on-exec internal file descriptors

[Tom Tromey]

>>>>> "Daniel" == Daniel Jacobowitz <drow@false.org> writes:

Daniel> If you build with headers that include O_CLOEXEC support, but the
Daniel> running kernel is too old, will this fail to open entirely?  Or are
Daniel> unknown open flags ignored?

I don't know, but I will find out.

Daniel> Also, WDYT about separating the goal from the mechanism?  With this
Daniel> patch, every bit of gdb knows "we close file descriptors on exec";
Daniel> if it was gdb_open, it would just be "gdb-specific version of open".

Sounds good to me, I'll do this.

Tom

Re: RFA: close-on-exec internal file descriptors

[Daniel Jacobowitz]

On Sat, Dec 06, 2008 at 08:58:03AM -0700, Tom Tromey wrote:
> Eli> Can we do something more safe than this arbitrary [20] limitation?
> 
> I don't think there is any point, because the mode argument to fopen
> is never longer than 4 characters or so.  However, if you really want
> this, I will change it to a malloc.

I find that alloca is good for these situations.

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFA: close-on-exec internal file descriptors

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Tom Tromey <tromey@redhat.com>
> Reply-To: Tom Tromey <tromey@redhat.com>
> Date: Sat, 06 Dec 2008 08:58:03 -0700
> 
> Eli> Relying on glibc is OK for GNU/Linux, but you seem to be modifying
> Eli> files that have no relation to the Linux native builds.  Does that
> Eli> mean the non-glibc builds that don't have the support you are relying
> Eli> on will still leak descriptors?
> 
> No.  In each case, if O_CLOEXEC is not available, we fall back to the
> fcntl-based method.  That is what the calls to "close_on_exec" do in
> the patch.

It seemed to me that the patch assumes too much about the various
#define's -- which is OK if you are targeting glibc, but may need
refinement for other libc's.  But perhaps I missed something; I will
take another look.

> >> +  char new_mode[20];
> >> +  strcpy (new_mode, mode);
> >> +  strcat (new_mode, "e");
> >> +  return fopen (path, new_mode);
> 
> Eli> Can we do something more safe than this arbitrary [20] limitation?
> 
> I don't think there is any point, because the mode argument to fopen
> is never longer than 4 characters or so.  However, if you really want
> this, I will change it to a malloc.

I just don't like relying on "is never longer than N" assumptions, and
GNU coding standards frown on arbitrarily dimensioned arrays, so...

Re: RFA: close-on-exec internal file descriptors

[Eli Zaretskii]

> Date: Sat, 6 Dec 2008 11:51:26 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Sat, Dec 06, 2008 at 08:58:03AM -0700, Tom Tromey wrote:
> > Eli> Can we do something more safe than this arbitrary [20] limitation?
> > 
> > I don't think there is any point, because the mode argument to fopen
> > is never longer than 4 characters or so.  However, if you really want
> > this, I will change it to a malloc.
> 
> I find that alloca is good for these situations.

`alloca' is fine with me.

Re: RFA: close-on-exec internal file descriptors

[Tom Tromey]

>>>>> "Mark" == Mark Kettenis <mark.kettenis@xs4all.nl> writes:

Tom> I chose to take advantage of the new glibc flags like O_CLOEXEC when
Tom> they are available.  This is friendlier in the Python case -- here,
Tom> gdb might have multiple threads, and the glibc flags enable us to
TOM> avoid a window where a file descriptor is not marked close-on-exec.

Mark> Sorry, but I don't see the point in having #ifdef O_CLOEXEC code
Mark> when there is a perfectly portable way to do this using fcntl.

It is better for thread safety.  This matters in the Python case.

Mark> It leads to more bits of code that can possibly go untested.

I will try to add a test case.  That will address this.

Mark> I also think it would actually be better to explicitly close file
Mark> descriptors before doing an exec instead of relying on people to use
Mark> the proper _cloexec call throughout gdb.

Why do you think this?

I think that it is difficult to truly ensure reliability with either
approach.  We might miss an open, but so too we might miss a
fork/exec.  The more libraries we use, the more likely this becomes.

But, since gdb and all its dependencies are free software, I think we
might as well try to implement the better approach, whichever that is.
In my view, close-on-exec is preferable.  It better communicates the
intent of the programmer, and in the library case it is an abstraction
barrier.

Tom

Re: RFA: close-on-exec internal file descriptors

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Sat, 06 Dec 2008 15:05:49 -0700
> 
> >>>>> "Mark" == Mark Kettenis <mark.kettenis@xs4all.nl> writes:
> 
> Tom> I chose to take advantage of the new glibc flags like O_CLOEXEC when
> Tom> they are available.  This is friendlier in the Python case -- here,
> Tom> gdb might have multiple threads, and the glibc flags enable us to
> TOM> avoid a window where a file descriptor is not marked close-on-exec.
> 
> Mark> Sorry, but I don't see the point in having #ifdef O_CLOEXEC code
> Mark> when there is a perfectly portable way to do this using fcntl.
> 
> It is better for thread safety.  This matters in the Python case.

Hmm, but that'd mean there will be thread-safety problems on platforms
that don't have O_CLOEXEC (including older Linux systems).  That's not
good :(.

Note that my suggestion to explicitly close file descriptors between
fork() and exec() doesn't have thread-safety problems.

> Mark> I also think it would actually be better to explicitly close file
> Mark> descriptors before doing an exec instead of relying on people to use
> Mark> the proper _cloexec call throughout gdb.
> 
> Why do you think this?
> 
> I think that it is difficult to truly ensure reliability with either
> approach.  We might miss an open, but so too we might miss a
> fork/exec.  The more libraries we use, the more likely this becomes.

I think open is used quite a bit more than fork/exec.  And libraries
that use fork/exec are rare.

> But, since gdb and all its dependencies are free software, I think we
> might as well try to implement the better approach, whichever that is.
> In my view, close-on-exec is preferable.  It better communicates the
> intent of the programmer, and in the library case it is an abstraction
> barrier.

Sorry, but I fail to understand what you mean with "abstraction barrier".