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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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
+}