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

Jan Kratochvil
2013-08-29T13:04:08Z

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