[patch gdbserver 7.6.1 only] Fix fd leak regression

[patch gdbserver 7.6.1 only] Fix fd leak regression

[Jan Kratochvil]

Hi,

in
	[commit without 7.6.1] [gdbserver patch] [7.6.1] Fix fd leak regression
	https://sourceware.org/ml/gdb-patches/2013-08/msg00845.html
I wrote:
# Although without 7.6.1 - 7.6 does not yet have gdb/common/filestuff.[ch] and
# from various available solutions I think it is just OK to wait for 7.7.

As I found I would like even 7.6.x fixed I wrote this alternative patch.

No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu and in
gdbserver mode.


Thanks,
Jan


gdb/gdbserver/
2013-08-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR server/15604
	* linux-low.c
	(linux_create_inferior) <pid == 0 && !remote_connection_is_stdio ()>:
	Close LISTEN_DESC.
	(lynx_create_inferior) <pid == 0 && !remote_connection_is_stdio ()>:
	Close LISTEN_DESC.
	* remote-utils.c (listen_desc): Remove static qualifier.
	* server.h (listen_desc): New declaration.
	* spu-low.c
	(spu_create_inferior) <pid == 0 && !remote_connection_is_stdio ()>:
	Close LISTEN_DESC.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 523926d..38d1caa 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -668,6 +668,8 @@ linux_create_inferior (char *program, char **allargs)
 	      /* Errors ignored.  */;
 	    }
 	}
+      else
+	close (listen_desc);
 
       execv (program, allargs);
       if (errno == ENOENT)
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index a5f3b6d..78ab6f4 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -218,6 +218,8 @@ lynx_create_inferior (char *program, char **allargs)
       pgrp = getpid();
       setpgid (0, pgrp);
       ioctl (0, TIOCSPGRP, &pgrp);
+      if (!remote_connection_is_stdio ())
+	close (listen_desc);
       lynx_ptrace (PTRACE_TRACEME, null_ptid, 0, 0, 0);
       execv (program, allargs);
       fprintf (stderr, "Cannot exec %s: %s.\n", program, strerror (errno));
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 42c6a54..0738309 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -109,7 +109,7 @@ struct ui_file *gdb_stdlog;
 static int remote_is_stdio = 0;
 
 static gdb_fildes_t remote_desc = INVALID_DESCRIPTOR;
-static gdb_fildes_t listen_desc = INVALID_DESCRIPTOR;
+gdb_fildes_t listen_desc = INVALID_DESCRIPTOR;
 
 /* FIXME headerize? */
 extern int using_threads;
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 139cd49..e5dce4e 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -274,6 +274,7 @@ extern void hostio_last_error_from_errno (char *own_buf);
 /* From remote-utils.c */
 
 extern int remote_debug;
+extern gdb_fildes_t listen_desc;
 extern int noack_mode;
 extern int transport_is_reliable;
 
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index deaa115..07839ca 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -273,6 +273,8 @@ spu_create_inferior (char *program, char **allargs)
 
   if (pid == 0)
     {
+      if (!remote_connection_is_stdio ())
+	close (listen_desc);
       ptrace (PTRACE_TRACEME, 0, 0, 0);
 
       setpgid (0, 0);

Re: [patch gdbserver 7.6.1 only] Fix fd leak regression

[Pedro Alves]

On 08/29/2013 12:10 PM, Jan Kratochvil wrote:
> Hi,
> 
> in
> 	[commit without 7.6.1] [gdbserver patch] [7.6.1] Fix fd leak regression
> 	https://sourceware.org/ml/gdb-patches/2013-08/msg00845.html
> I wrote:
> # Although without 7.6.1 - 7.6 does not yet have gdb/common/filestuff.[ch] and
> # from various available solutions I think it is just OK to wait for 7.7.
> 
> As I found I would like even 7.6.x fixed I wrote this alternative patch.
> 

(We could also just mark the sockets as SOCK_CLOEXEC, I think?
Though that's "only" since Linux 2.6.27, so I've no objection with
going this route.)

I think we need to close remote_desc too.  With plain "target remote",
the inferior is spawned before gdb connects, but with extended-remote+"run",
that's not the case.

-- 
Pedro Alves

Re: [patch gdbserver 7.6.1 only] Fix fd leak regression

[Jan Kratochvil]

On Thu, 29 Aug 2013 14:15:45 +0200, Pedro Alves wrote:
> (We could also just mark the sockets as SOCK_CLOEXEC, I think?
> Though that's "only" since Linux 2.6.27, so I've no objection with
> going this route.)

* There is no real need for SOCK_CLOEXEC, GDB benefits from it for Python
  threads (possibly calling their own fork+exec) but those do not happen for
  gdbserver.

* It would not work for old/non-Linux OSes which does not matter much but it
  is still rather a disadvantage.

* One has to handle missing SOCK_CLOEXEC #define and also errors on setting
  it, and for two fds.

So +/- I did not consider it easier than this one.


> I think we need to close remote_desc too.  With plain "target remote",
> the inferior is spawned before gdb connects, but with extended-remote+"run",
> that's not the case.

True, thanks for catching it.

I have tested extended-remote really leaked fd and the leak no longer happens.


Jan


gdb/gdbserver/
2013-08-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR server/15604
	* linux-low.c
	(linux_create_inferior) <pid == 0 && !remote_connection_is_stdio ()>:
	Close LISTEN_DESC and optionally REMOTE_DESC.
	(lynx_create_inferior) <pid == 0 && !remote_connection_is_stdio ()>:
	Close LISTEN_DESC and optionally REMOTE_DESC.
	* remote-utils.c (remote_desc, listen_desc): Remove static qualifier.
	* server.h (remote_desc, listen_desc): New declaration.
	* spu-low.c
	(spu_create_inferior) <pid == 0 && !remote_connection_is_stdio ()>:
	Close LISTEN_DESC and optionally REMOTE_DESC.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 12208dc..4e16372 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -602,6 +602,12 @@ linux_create_inferior (char *program, char **allargs)
 	      /* Errors ignored.  */;
 	    }
 	}
+      else
+	{
+	  close (listen_desc);
+	  if (gdb_connected ())
+	    close (remote_desc);
+	}
 
       execv (program, allargs);
       if (errno == ENOENT)
diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
index 4cf8683..46c5fbb 100644
--- a/gdb/gdbserver/lynx-low.c
+++ b/gdb/gdbserver/lynx-low.c
@@ -245,6 +245,12 @@ lynx_create_inferior (char *program, char **allargs)
       pgrp = getpid();
       setpgid (0, pgrp);
       ioctl (0, TIOCSPGRP, &pgrp);
+      if (!remote_connection_is_stdio ())
+	{
+	  close (listen_desc);
+	  if (gdb_connected ())
+	    close (remote_desc);
+	}
       lynx_ptrace (PTRACE_TRACEME, null_ptid, 0, 0, 0);
       execv (program, allargs);
       fprintf (stderr, "Cannot exec %s: %s.\n", program, strerror (errno));
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 5cd6fa1..aa02c09 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -107,8 +107,8 @@ struct ui_file *gdb_stdlog;
 
 static int remote_is_stdio = 0;
 
-static gdb_fildes_t remote_desc = INVALID_DESCRIPTOR;
-static gdb_fildes_t listen_desc = INVALID_DESCRIPTOR;
+gdb_fildes_t remote_desc = INVALID_DESCRIPTOR;
+gdb_fildes_t listen_desc = INVALID_DESCRIPTOR;
 
 /* FIXME headerize? */
 extern int using_threads;
diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
index 1a1d9b2..88eae43 100644
--- a/gdb/gdbserver/server.h
+++ b/gdb/gdbserver/server.h
@@ -281,6 +281,8 @@ extern void hostio_last_error_from_errno (char *own_buf);
 /* From remote-utils.c */
 
 extern int remote_debug;
+extern gdb_fildes_t remote_desc;
+extern gdb_fildes_t listen_desc;
 extern int noack_mode;
 extern int transport_is_reliable;
 
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index 6e3974a..028829b 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -274,6 +274,12 @@ spu_create_inferior (char *program, char **allargs)
 
   if (pid == 0)
     {
+      if (!remote_connection_is_stdio ())
+	{
+	  close (listen_desc);
+	  if (gdb_connected ())
+	    close (remote_desc);
+	}
       ptrace (PTRACE_TRACEME, 0, 0, 0);
 
       setpgid (0, 0);
diff --git a/gdb/testsuite/gdb.base/fd-leak.c b/gdb/testsuite/gdb.base/fd-leak.c
new file mode 100644
index 0000000..bb88164
--- /dev/null
+++ b/gdb/testsuite/gdb.base/fd-leak.c
@@ -0,0 +1,42 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+#include <stdio.h>
+#include <errno.h>
+#include <string.h>
+
+int
+main (void)
+{
+  int expect_fd = 3, iter;
+
+  for (iter = 0; iter < 20; iter++)
+    {
+      int got = dup (0);
+
+      if (got != expect_fd)
+	{
+	  fprintf (stderr, "In iteration %d expected fd %d but got %d: %s\n",
+	           iter, expect_fd, got, strerror (errno));
+	  return 1;
+	}
+      expect_fd++;
+    }
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/fd-leak.exp b/gdb/testsuite/gdb.base/fd-leak.exp
new file mode 100644
index 0000000..dfc70d7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/fd-leak.exp
@@ -0,0 +1,37 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set test "system fd behavior is known"
+set status [remote_exec target "[standard_output_file $testfile]"]
+if { [lindex $status 0] == 0 } {
+    pass $test
+} else {
+    fail $test
+}
+remote_exec target "ls -l /proc/self/fd/"
+# exec "sh" "-c" "ls -l /proc/self/fd/ >/tmp/fd-leak.debug"
+
+clean_restart "$testfile"
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}

Re: [patch gdbserver 7.6.1 only] Fix fd leak regression

[Pedro Alves]

On 08/29/2013 02:03 PM, Jan Kratochvil wrote:
> On Thu, 29 Aug 2013 14:15:45 +0200, Pedro Alves wrote:
>> (We could also just mark the sockets as SOCK_CLOEXEC, I think?
>> Though that's "only" since Linux 2.6.27, so I've no objection with
>> going this route.)
> 
> * There is no real need for SOCK_CLOEXEC, GDB benefits from it for Python
>   threads (possibly calling their own fork+exec) but those do not happen for
>   gdbserver.

I suspect we'll have to revisit this at some point (years from now),
but yeah, agreed.

> 
> * It would not work for old/non-Linux OSes which does not matter much but it
>   is still rather a disadvantage.

*nod*

> gdb/gdbserver/
> 2013-08-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	PR server/15604
> 	* linux-low.c
> 	(linux_create_inferior) <pid == 0 && !remote_connection_is_stdio ()>:
> 	Close LISTEN_DESC and optionally REMOTE_DESC.
> 	(lynx_create_inferior) <pid == 0 && !remote_connection_is_stdio ()>:
> 	Close LISTEN_DESC and optionally REMOTE_DESC.
> 	* remote-utils.c (remote_desc, listen_desc): Remove static qualifier.
> 	* server.h (remote_desc, listen_desc): New declaration.
> 	* spu-low.c
> 	(spu_create_inferior) <pid == 0 && !remote_connection_is_stdio ()>:
> 	Close LISTEN_DESC and optionally REMOTE_DESC.

Thanks, this looks good to me.

> +set test "system fd behavior is known"
> +set status [remote_exec target "[standard_output_file $testfile]"]
> +if { [lindex $status 0] == 0 } {
> +    pass $test
> +} else {
> +    fail $test
> +}
> +remote_exec target "ls -l /proc/self/fd/"

Before gdbserver's fix, do we get one extra fd from the dejagnu
leak, and another extra from gdbserver's leak?  What if we made
$testfile count open fds, and then compare that between running
under gdb/gdbserver and just under remote_exec ?

-- 
Pedro Alves

[commit 7.6.1 only] [patch gdbserver 7.6.1 only] Fix fd leak regression

[Jan Kratochvil]

On Thu, 29 Aug 2013 16:17:40 +0200, Pedro Alves wrote:
> Thanks, this looks good to me.

Checked in:
	https://sourceware.org/ml/gdb-cvs/2013-08/msg00161.html


> > +set test "system fd behavior is known"
> > +set status [remote_exec target "[standard_output_file $testfile]"]
> > +if { [lindex $status 0] == 0 } {
> > +    pass $test
> > +} else {
> > +    fail $test
> > +}
> > +remote_exec target "ls -l /proc/self/fd/"
> 
> Before gdbserver's fix, do we get one extra fd from the dejagnu
> leak, and another extra from gdbserver's leak?  What if we made
> $testfile count open fds, and then compare that between running
> under gdb/gdbserver and just under remote_exec ?

BTW not sure if it is clear but this gdb/testsuite/ part was sent
accidentally, there is even written no real test and I have not checked in
anything as I got stuck on the DejaGNU bug.

I have sent a bugreport to <bug-dejagnu@gnu.org> and it got processed today
although the mail has not yet appeared in the archive:
	http://lists.gnu.org/archive/html/bug-dejagnu/

I hope DejaGNU gets fixed soon so the testcase can be later written as
UNSUPPORTED (UNTESTED?) with buggy DejaGNU.


Thanks,
Jan

Re: [commit 7.6.1 only] [patch gdbserver 7.6.1 only] Fix fd leak regression

[Pedro Alves]

On 08/29/2013 03:27 PM, Jan Kratochvil wrote:
> On Thu, 29 Aug 2013 16:17:40 +0200, Pedro Alves wrote:

>>> +set test "system fd behavior is known"
>>> +set status [remote_exec target "[standard_output_file $testfile]"]
>>> +if { [lindex $status 0] == 0 } {
>>> +    pass $test
>>> +} else {
>>> +    fail $test
>>> +}
>>> +remote_exec target "ls -l /proc/self/fd/"
>>
>> Before gdbserver's fix, do we get one extra fd from the dejagnu
>> leak, and another extra from gdbserver's leak?  What if we made
>> $testfile count open fds, and then compare that between running
>> under gdb/gdbserver and just under remote_exec ?
> 
> BTW not sure if it is clear 

It wasn't at first, but I figured it out when I noticed it had
no ChangeLog entry.

> but this gdb/testsuite/ part was sent
> accidentally, there is even written no real test and I have not checked in
> anything as I got stuck on the DejaGNU bug.

Understood.  I was suggesting a possible way to not need to
wait for the dejagnu bug to be fixed.  If what I suggested actually
works, I don't see a downside -- we'd just be checking whether
gdbserver itself introduces any leak, which is all we should
care about?

> 
> I have sent a bugreport to <bug-dejagnu@gnu.org> and it got processed today
> although the mail has not yet appeared in the archive:
> 	http://lists.gnu.org/archive/html/bug-dejagnu/
> 
> I hope DejaGNU gets fixed soon so the testcase can be later written as
> UNSUPPORTED (UNTESTED?) with buggy DejaGNU.

Ack.

-- 
Pedro Alves

Re: [patch gdbserver 7.6.1 only] Fix fd leak regression

[Tom Tromey]

>> * There is no real need for SOCK_CLOEXEC, GDB benefits from it for Python
>> threads (possibly calling their own fork+exec) but those do not happen for
>> gdbserver.

Pedro> I suspect we'll have to revisit this at some point (years from now),
Pedro> but yeah, agreed.

gdbserver on trunk can just use the filestuff.c code.

Tom

Re: [patch gdbserver 7.6.1 only] Fix fd leak regression

[Pedro Alves]

On 08/29/2013 03:40 PM, Tom Tromey wrote:
>>> * There is no real need for SOCK_CLOEXEC, GDB benefits from it for Python
>>> threads (possibly calling their own fork+exec) but those do not happen for
>>> gdbserver.
> 
> Pedro> I suspect we'll have to revisit this at some point (years from now),
> Pedro> but yeah, agreed.
> 
> gdbserver on trunk can just use the filestuff.c code.

Yeah, it already does:

 https://sourceware.org/ml/gdb-patches/2013-08/msg00765.html

but it doesn't call socket_mark_cloexec or similars, so we'll
probably still have to revisit this at some point (years
from now).  :-)

-- 
Pedro Alves

Re: [patch gdbserver 7.6.1 only] Fix fd leak regression

[Tom Tromey]

Tom> gdbserver on trunk can just use the filestuff.c code.

Pedro> Yeah, it already does:
Pedro>  https://sourceware.org/ml/gdb-patches/2013-08/msg00765.html
Pedro> but it doesn't call socket_mark_cloexec or similars, so we'll
Pedro> probably still have to revisit this at some point (years
Pedro> from now).  :-)

Yeah, I meant the other filestuff.h code, like gdb_socket_cloexec and
gdb_pipe_cloexec.

Tom

Re: [patch gdbserver 7.6.1 only] Fix fd leak regression

[Tom Tromey]

Tom> Yeah, I meant the other filestuff.h code, like gdb_socket_cloexec and
Tom> gdb_pipe_cloexec.

Like this.  I haven't tested it using native-gdbserver yet.

Tom

commit d190e7c809a8dba4768cb67b9c46610e8e8e1d07
Author: Tom Tromey <tromey@redhat.com>
Date:   Thu Aug 29 10:13:50 2013 -0600

    use _cloexec functions in gdbserver
    
    This changes gdbserver to use the various _cloexec functions defined
    in common/filestuff.c.
    
    Two wrinkles here: first, I couldn't compile the NTO and SPU changes;
    second, the in process agent can't use gdb_socket_cloexec -- too bad,
    since the setting would be useful there.
    
    	* hostio.c (handle_open): Use gdb_open_cloexec.
    	* linux-low.c (elf_64_file_p, linux_create_inferior)
    	(linux_read_memory, linux_read_auxv): Use gdb_open_cloexec.
    	(linux_async): Use gdb_pipe_cloexec.
    	(linux_qxfer_spu, get_phdr_phnum_from_proc_auxv): Use
    	gdb_open_cloexec.
    	* nto-low.c (do_attach): Use gdb_open_cloexec.
    	* remote-utils.c (remote_prepare): Use gdb_socket_cloexec.
    	(remote_open): Use gdb_open_cloexec.
    	* spu-low.c (spu_proc_xfer_spu): Use gdb_open_cloexec.
    	* tracepointc. (gdb_socket_cloexec): Define in IPA mode.
    	(init_named_socket): Use gdb_socket_cloexec.

diff --git a/gdb/gdbserver/hostio.c b/gdb/gdbserver/hostio.c
index a74c2f8..edef7bc 100644
--- a/gdb/gdbserver/hostio.c
+++ b/gdb/gdbserver/hostio.c
@@ -20,6 +20,7 @@
 
 #include "server.h"
 #include "gdb/fileio.h"
+#include "filestuff.h"
 
 #include <fcntl.h>
 #include <limits.h>
@@ -295,7 +296,7 @@ handle_open (char *own_buf)
 
   /* We do not need to convert MODE, since the fileio protocol
      uses the standard values.  */
-  fd = open (filename, flags, mode);
+  fd = gdb_open_cloexec (filename, flags, mode);
 
   if (fd == -1)
     {
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 7db1fc8..3fa3bf1 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -315,7 +315,7 @@ elf_64_file_p (const char *file, unsigned int *machine)
   Elf64_Ehdr header;
   int fd;
 
-  fd = open (file, O_RDONLY);
+  fd = gdb_open_cloexec (file, O_RDONLY, 0);
   if (fd < 0)
     return -1;
 
@@ -596,7 +596,7 @@ linux_create_inferior (char *program, char **allargs)
       if (remote_connection_is_stdio ())
 	{
 	  close (0);
-	  open ("/dev/null", O_RDONLY);
+	  gdb_open_cloexec ("/dev/null", O_RDONLY, 0);
 	  dup2 (2, 1);
 	  if (write (2, "stdin/stdout redirected\n",
 		     sizeof ("stdin/stdout redirected\n") - 1) < 0)
@@ -4426,7 +4426,7 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
       /* We could keep this file open and cache it - possibly one per
 	 thread.  That requires some juggling, but is even faster.  */
       sprintf (filename, "/proc/%d/mem", pid);
-      fd = open (filename, O_RDONLY | O_LARGEFILE);
+      fd = gdb_open_cloexec (filename, O_RDONLY | O_LARGEFILE, 0);
       if (fd == -1)
 	goto no_proc;
 
@@ -4626,7 +4626,7 @@ linux_read_auxv (CORE_ADDR offset, unsigned char *myaddr, unsigned int len)
 
   xsnprintf (filename, sizeof filename, "/proc/%d/auxv", pid);
 
-  fd = open (filename, O_RDONLY);
+  fd = gdb_open_cloexec (filename, O_RDONLY, 0);
   if (fd < 0)
     return -1;
 
@@ -4859,7 +4859,7 @@ linux_async (int enable)
 
       if (enable)
 	{
-	  if (pipe (linux_event_pipe) == -1)
+	  if (gdb_pipe_cloexec (linux_event_pipe) == -1)
 	    fatal ("creating event pipe failed.");
 
 	  fcntl (linux_event_pipe[0], F_SETFL, O_NONBLOCK);
@@ -5000,7 +5000,7 @@ linux_qxfer_spu (const char *annex, unsigned char *readbuf,
     }
 
   sprintf (buf, "/proc/%ld/fd/%s", pid, annex);
-  fd = open (buf, writebuf? O_WRONLY : O_RDONLY);
+  fd = gdb_open_cloexec (buf, writebuf? O_WRONLY : O_RDONLY, 0);
   if (fd <= 0)
     return -1;
 
@@ -5224,7 +5224,7 @@ get_phdr_phnum_from_proc_auxv (const int pid, const int is_elf64,
 
   xsnprintf (filename, sizeof filename, "/proc/%d/auxv", pid);
 
-  fd = open (filename, O_RDONLY);
+  fd = gdb_open_cloexec (filename, O_RDONLY, 0);
   if (fd < 0)
     return 1;
 
diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
index 3670133..91fffbc 100644
--- a/gdb/gdbserver/nto-low.c
+++ b/gdb/gdbserver/nto-low.c
@@ -21,6 +21,7 @@
 #include "server.h"
 #include "gdbthread.h"
 #include "nto-low.h"
+#include "filestuff.h"
 
 #include <limits.h>
 #include <fcntl.h>
@@ -178,7 +179,8 @@ do_attach (pid_t pid)
       init_nto_inferior (&nto_inferior);
     }
   xsnprintf (nto_inferior.nto_procfs_path, PATH_MAX - 1, "/proc/%d/as", pid);
-  nto_inferior.ctl_fd = open (nto_inferior.nto_procfs_path, O_RDWR);
+  nto_inferior.ctl_fd = gdb_open_cloexec (nto_inferior.nto_procfs_path,
+					  O_RDWR, 0);
   if (nto_inferior.ctl_fd == -1)
     {
       TRACE ("Failed to open %s\n", nto_inferior.nto_procfs_path);
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 5cd6fa1..25f5302 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -68,6 +68,8 @@
 #include <sys/iomgr.h>
 #endif /* __QNX__ */
 
+#include "filestuff.h"
+
 #ifndef HAVE_SOCKLEN_T
 typedef int socklen_t;
 #endif
@@ -262,7 +264,7 @@ remote_prepare (char *name)
     }
 #endif
 
-  listen_desc = socket (PF_INET, SOCK_STREAM, IPPROTO_TCP);
+  listen_desc = gdb_socket_cloexec (PF_INET, SOCK_STREAM, IPPROTO_TCP);
   if (listen_desc == -1)
     perror_with_name ("Can't open socket");
 
@@ -316,7 +318,7 @@ remote_open (char *name)
 
       if (stat (name, &statbuf) == 0
 	  && (S_ISCHR (statbuf.st_mode) || S_ISFIFO (statbuf.st_mode)))
-	remote_desc = open (name, O_RDWR);
+	remote_desc = gdb_open_cloexec (name, O_RDWR, 0);
       else
 	{
 	  errno = EINVAL;
diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
index e604b9f..63a2efc 100644
--- a/gdb/gdbserver/spu-low.c
+++ b/gdb/gdbserver/spu-low.c
@@ -239,7 +239,7 @@ spu_proc_xfer_spu (const char *annex, unsigned char *readbuf,
     return 0;
 
   sprintf (buf, "/proc/%ld/fd/%s", ptid_get_lwp (current_ptid), annex);
-  fd = open (buf, writebuf? O_WRONLY : O_RDONLY);
+  fd = gdb_open_cloexec (buf, writebuf? O_WRONLY : O_RDONLY, 0);
   if (fd <= 0)
     return -1;
 
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 5c0dec7..4c4a751 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -19,6 +19,7 @@
 #include "server.h"
 #include "gdbthread.h"
 #include "agent.h"
+#include "filestuff.h"
 
 #include <ctype.h>
 #include <fcntl.h>
@@ -147,6 +148,9 @@ trace_vdebug (const char *fmt, ...)
 # define ust_loaded gdb_agent_ust_loaded
 # define helper_thread_id gdb_agent_helper_thread_id
 # define cmd_buf gdb_agent_cmd_buf
+
+/* We don't want to use this one in IPA.  */
+# define gdb_socket_cloexec socket
 #endif
 
 #ifndef IN_PROCESS_AGENT
@@ -6787,7 +6791,7 @@ init_named_socket (const char *name)
   int result, fd;
   struct sockaddr_un addr;
 
-  result = fd = socket (PF_UNIX, SOCK_STREAM, 0);
+  result = fd = gdb_socket_cloexec (PF_UNIX, SOCK_STREAM, 0);
   if (result == -1)
     {
       warning ("socket creation failed: %s", strerror (errno));

Re: [patch gdbserver 7.6.1 only] Fix fd leak regression

[Pedro Alves]

On 08/29/2013 06:22 PM, Tom Tromey wrote:
> Tom> Yeah, I meant the other filestuff.h code, like gdb_socket_cloexec and
> Tom> gdb_pipe_cloexec.
> 

>     second, the in process agent can't use gdb_socket_cloexec 

But, why ?

> @@ -596,7 +596,7 @@ linux_create_inferior (char *program, char **allargs)
>        if (remote_connection_is_stdio ())
>  	{
>  	  close (0);
> -	  open ("/dev/null", O_RDONLY);
> +	  gdb_open_cloexec ("/dev/null", O_RDONLY, 0);

This is the child opening its stdin.  Doesn't look
correct to close it on the subsequent exec.

>  
>  #include <ctype.h>
>  #include <fcntl.h>
> @@ -147,6 +148,9 @@ trace_vdebug (const char *fmt, ...)
>  # define ust_loaded gdb_agent_ust_loaded
>  # define helper_thread_id gdb_agent_helper_thread_id
>  # define cmd_buf gdb_agent_cmd_buf
> +
> +/* We don't want to use this one in IPA.  */

"because ..." ?

> +# define gdb_socket_cloexec socket
>  #endif

-- 
Pedro Alves

Re: [patch gdbserver 7.6.1 only] Fix fd leak regression

[Tom Tromey]

Tom> second, the in process agent can't use gdb_socket_cloexec 

Pedro> But, why ?

filestuff.o isn't available there.  I guess it could be?
I will give it a whirl.

>> -	  open ("/dev/null", O_RDONLY);
>> +	  gdb_open_cloexec ("/dev/null", O_RDONLY, 0);

Pedro> This is the child opening its stdin.  Doesn't look
Pedro> correct to close it on the subsequent exec.

Hahaha, barf.

Tom