From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17058 invoked by alias); 29 Aug 2013 13:04:08 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 16996 invoked by uid 89); 29 Aug 2013 13:04:07 -0000 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 Aug 2013 13:04:07 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7TD44tH002626 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 29 Aug 2013 09:04:05 -0400 Received: from host2.jankratochvil.net (ovpn-116-30.ams2.redhat.com [10.36.116.30]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r7TD40NI013495 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Thu, 29 Aug 2013 09:04:02 -0400 Date: Thu, 29 Aug 2013 13:04:00 -0000 From: Jan Kratochvil To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [patch gdbserver 7.6.1 only] Fix fd leak regression Message-ID: <20130829130359.GA31063@host2.jankratochvil.net> References: <20130829111053.GA25662@host2.jankratochvil.net> <521F3B71.1010007@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <521F3B71.1010007@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-08/txt/msg00866.txt.bz2 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 PR server/15604 * linux-low.c (linux_create_inferior) : Close LISTEN_DESC and optionally REMOTE_DESC. (lynx_create_inferior) : 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) : 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 . */ + +#include +#include +#include +#include + +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 . + +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 +}