From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1059 invoked by alias); 28 Oct 2008 20:00:37 -0000 Received: (qmail 1049 invoked by uid 22791); 28 Oct 2008 20:00:36 -0000 X-Spam-Check-By: sourceware.org Received: from mx2.redhat.com (HELO mx2.redhat.com) (66.187.237.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 28 Oct 2008 20:00:01 +0000 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id m9SJxwdD023806 for ; Tue, 28 Oct 2008 15:59:58 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx2.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m9SJxv6Q002352; Tue, 28 Oct 2008 15:59:58 -0400 Received: from opsy.redhat.com (vpn-12-121.rdu.redhat.com [10.11.12.121]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id m9SJxuRj027757; Tue, 28 Oct 2008 15:59:57 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id E7FD63785B9; Tue, 28 Oct 2008 13:59:55 -0600 (MDT) To: gdb-patches@sourceware.org Subject: RFA: open-related cleanup handling From: Tom Tromey Reply-To: tromey@redhat.com X-Attribution: Tom Date: Tue, 28 Oct 2008 20:21:00 -0000 Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 X-SW-Source: 2008-10/txt/msg00683.txt.bz2 Another in a series of possible file descriptor leak fixes. This fixes most leaks associated with open, and also opendir. I say most because there is one more patch that is a bit more odd that I wanted to submit separately. This one should not be too controversial. I found the problems in question by looking at all calls to open or opendir, and then all calls to all functions returning the result of open or opendir. I also looked at all calls to socket, accept, and pipe, but none of these showed bugs. (I did not look at gdbserver at all, FWIW.) Also note the changes to symtab_to_fullname and psymtab_to_fullname. These incorrectly test for an error return from find_and_open_source. Built and regtested on x86-64 (compile farm), though again, this is likely not a comprehensive test. Ok? Tom 2008-10-28 Tom Tromey * source.c (symtab_to_fullname): Test 'r >= 0'. (psymtab_to_fullname): Likewise. (get_filename_and_charpos): Make a cleanup. (forward_search_command): Likewise. (reverse_search_command): Likewise. * exec.c (exec_file_attach): Close scratch_chan on failure. * procfs.c (load_syscalls): Make a cleanup. (open_procinfo_files): fd==0 is ok. * nto-procfs.c (procfs_open): Make a cleanup. (procfs_pidlist): Likewise. (do_closedir_cleanup): New function. diff --git a/gdb/exec.c b/gdb/exec.c index 3b9ecfa..92450e6 100644 --- a/gdb/exec.c +++ b/gdb/exec.c @@ -217,8 +217,11 @@ exec_file_attach (char *filename, int from_tty) scratch_chan); if (!exec_bfd) - error (_("\"%s\": could not open as an executable file: %s"), - scratch_pathname, bfd_errmsg (bfd_get_error ())); + { + close (scratch_chan); + error (_("\"%s\": could not open as an executable file: %s"), + scratch_pathname, bfd_errmsg (bfd_get_error ())); + } /* At this point, scratch_pathname and exec_bfd->name both point to the same malloc'd string. However exec_close() will attempt to free it diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c index 7450edc..f69aaf2 100644 --- a/gdb/nto-procfs.c +++ b/gdb/nto-procfs.c @@ -125,6 +125,7 @@ procfs_open (char *arg, int from_tty) char buffer[50]; int fd, total_size; procfs_sysinfo *sysinfo; + struct cleanup *cleanups; nto_is_nto_target = procfs_is_nto_target; @@ -169,13 +170,13 @@ procfs_open (char *arg, int from_tty) safe_strerror (errno)); error (_("Invalid procfs arg")); } + cleanups = make_cleanup_close (fd); sysinfo = (void *) buffer; if (devctl (fd, DCMD_PROC_SYSINFO, sysinfo, sizeof buffer, 0) != EOK) { printf_filtered ("Error getting size: %d (%s)\n", errno, safe_strerror (errno)); - close (fd); error (_("Devctl failed.")); } else @@ -186,7 +187,6 @@ procfs_open (char *arg, int from_tty) { printf_filtered ("Memory error: %d (%s)\n", errno, safe_strerror (errno)); - close (fd); error (_("alloca failed.")); } else @@ -195,7 +195,6 @@ procfs_open (char *arg, int from_tty) { printf_filtered ("Error getting sysinfo: %d (%s)\n", errno, safe_strerror (errno)); - close (fd); error (_("Devctl failed.")); } else @@ -203,14 +202,11 @@ procfs_open (char *arg, int from_tty) if (sysinfo->type != nto_map_arch_to_cputype (gdbarch_bfd_arch_info (current_gdbarch)->arch_name)) - { - close (fd); - error (_("Invalid target CPU.")); - } + error (_("Invalid target CPU.")); } } } - close (fd); + do_cleanups (cleanups); printf_filtered ("Debugging using %s\n", nto_procfs_path); } @@ -259,12 +255,17 @@ procfs_find_new_threads (void) return; } +static void +do_closedir_cleanup (void *dir) +{ + closedir (dir); +} + void procfs_pidlist (char *args, int from_tty) { DIR *dp = NULL; struct dirent *dirp = NULL; - int fd = -1; char buf[512]; procfs_info *pidinfo = NULL; procfs_debuginfo *info = NULL; @@ -272,6 +273,7 @@ procfs_pidlist (char *args, int from_tty) pid_t num_threads = 0; pid_t pid; char name[512]; + struct cleanup *cleanups; dp = opendir (nto_procfs_path); if (dp == NULL) @@ -281,18 +283,23 @@ procfs_pidlist (char *args, int from_tty) return; } + cleanups = make_cleanup (do_closedir_cleanup, dp); + /* Start scan at first pid. */ rewinddir (dp); do { + int fd; + struct cleanup *inner_cleanup; + /* Get the right pid and procfs path for the pid. */ do { dirp = readdir (dp); if (dirp == NULL) { - closedir (dp); + do_cleanups (cleanups); return; } snprintf (buf, 511, "%s/%s/as", nto_procfs_path, dirp->d_name); @@ -306,9 +313,10 @@ procfs_pidlist (char *args, int from_tty) { fprintf_unfiltered (gdb_stderr, "failed to open %s - %d (%s)\n", buf, errno, safe_strerror (errno)); - closedir (dp); + do_cleanups (cleanups); return; } + inner_cleanup = make_cleanup_close (fd); pidinfo = (procfs_info *) buf; if (devctl (fd, DCMD_PROC_INFO, pidinfo, sizeof (buf), 0) != EOK) @@ -336,12 +344,12 @@ procfs_pidlist (char *args, int from_tty) if (status->tid != 0) printf_filtered ("%s - %d/%d\n", name, pid, status->tid); } - close (fd); + + do_cleanups (inner_cleanup); } while (dirp != NULL); - close (fd); - closedir (dp); + do_cleanups (cleanups); return; } diff --git a/gdb/source.c b/gdb/source.c index 3ef557c..c41c193 100644 --- a/gdb/source.c +++ b/gdb/source.c @@ -1064,7 +1064,7 @@ symtab_to_fullname (struct symtab *s) r = find_and_open_source (s->objfile, s->filename, s->dirname, &s->fullname); - if (r) + if (r >= 0) { close (r); return s->fullname; @@ -1093,7 +1093,7 @@ psymtab_to_fullname (struct partial_symtab *ps) r = find_and_open_source (ps->objfile, ps->filename, ps->dirname, &ps->fullname); - if (r) + if (r >= 0) { close (r); return ps->fullname; @@ -1251,6 +1251,7 @@ static int get_filename_and_charpos (struct symtab *s, char **fullname) { int desc, linenums_changed = 0; + struct cleanup *cleanups; desc = open_source_file (s); if (desc < 0) @@ -1259,13 +1260,14 @@ get_filename_and_charpos (struct symtab *s, char **fullname) *fullname = NULL; return 0; } + cleanups = make_cleanup_close (desc); if (fullname) *fullname = s->fullname; if (s->line_charpos == 0) linenums_changed = 1; if (linenums_changed) find_source_lines (s, desc); - close (desc); + do_cleanups (cleanups); return linenums_changed; } @@ -1540,6 +1542,7 @@ forward_search_command (char *regex, int from_tty) FILE *stream; int line; char *msg; + struct cleanup *cleanups; line = last_line_listed + 1; @@ -1553,24 +1556,21 @@ forward_search_command (char *regex, int from_tty) desc = open_source_file (current_source_symtab); if (desc < 0) perror_with_name (current_source_symtab->filename); + cleanups = make_cleanup_close (desc); if (current_source_symtab->line_charpos == 0) find_source_lines (current_source_symtab, desc); if (line < 1 || line > current_source_symtab->nlines) - { - close (desc); - error (_("Expression not found")); - } + error (_("Expression not found")); if (lseek (desc, current_source_symtab->line_charpos[line - 1], 0) < 0) - { - close (desc); - perror_with_name (current_source_symtab->filename); - } + perror_with_name (current_source_symtab->filename); + discard_cleanups (cleanups); stream = fdopen (desc, FDOPEN_MODE); clearerr (stream); + cleanups = make_cleanup_fclose (stream); while (1) { static char *buf = NULL; @@ -1622,7 +1622,7 @@ forward_search_command (char *regex, int from_tty) } printf_filtered (_("Expression not found\n")); - fclose (stream); + do_cleanups (cleanups); } static void @@ -1633,6 +1633,7 @@ reverse_search_command (char *regex, int from_tty) FILE *stream; int line; char *msg; + struct cleanup *cleanups; line = last_line_listed - 1; @@ -1646,24 +1647,21 @@ reverse_search_command (char *regex, int from_tty) desc = open_source_file (current_source_symtab); if (desc < 0) perror_with_name (current_source_symtab->filename); + cleanups = make_cleanup_close (desc); if (current_source_symtab->line_charpos == 0) find_source_lines (current_source_symtab, desc); if (line < 1 || line > current_source_symtab->nlines) - { - close (desc); - error (_("Expression not found")); - } + error (_("Expression not found")); if (lseek (desc, current_source_symtab->line_charpos[line - 1], 0) < 0) - { - close (desc); - perror_with_name (current_source_symtab->filename); - } + perror_with_name (current_source_symtab->filename); + discard_cleanups (cleanups); stream = fdopen (desc, FDOPEN_MODE); clearerr (stream); + cleanups = make_cleanup_fclose (stream); while (line > 1) { /* FIXME!!! We walk right off the end of buf if we get a long line!!! */ @@ -1709,7 +1707,7 @@ reverse_search_command (char *regex, int from_tty) } printf_filtered (_("Expression not found\n")); - fclose (stream); + do_cleanups (cleanups); return; }