RFA: open-related cleanup handling

RFA: open-related cleanup handling

[Tom Tromey]

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  <tromey@redhat.com>

	* 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;
 }
 

Re: RFA: open-related cleanup handling

[Pedro Alves]

On Tuesday 28 October 2008 19:59:55, Tom Tromey wrote:

> Ok?

Looks OK to me.

> @@ -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);

I noticed that discarding a _close cleanup is currently leaking the
xmalloc'ed int used to hold the file descriptor.  Fixing it should be a
matter of doing something similar to make_cleanup_restore_integer
(using make_my_cleanup2) from inside make_cleanup_close.
Would you like to take care of that while you have your hangs
dirty doing these cleaning ups?

-- 
Pedro Alves

Re: RFA: open-related cleanup handling

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I noticed that discarding a _close cleanup is currently leaking the
Pedro> xmalloc'ed int used to hold the file descriptor.  Fixing it should be a
Pedro> matter of doing something similar to make_cleanup_restore_integer
Pedro> (using make_my_cleanup2) from inside make_cleanup_close.
Pedro> Would you like to take care of that while you have your hangs
Pedro> dirty doing these cleaning ups?

No problem.

How about this?  I used make_cleanup_dtor -- same difference though.

Built & regtested on x86-64 (compile farm).

What do you think of making make_my_cleanup and make_my_cleanup2 static?
They aren't used outside of utils.c.

Tom

2008-10-30  Tom Tromey  <tromey@redhat.com>

	* utils.c (make_cleanup_close): Use make_cleanup_dtor.
	(do_close_cleanup): Don't free 'fd'.

diff --git a/gdb/utils.c b/gdb/utils.c
index f9a5f19..26d7933 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -244,7 +244,6 @@ do_close_cleanup (void *arg)
 {
   int *fd = arg;
   close (*fd);
-  xfree (fd);
 }
 
 struct cleanup *
@@ -252,7 +251,7 @@ make_cleanup_close (int fd)
 {
   int *saved_fd = xmalloc (sizeof (fd));
   *saved_fd = fd;
-  return make_cleanup (do_close_cleanup, saved_fd);
+  return make_cleanup_dtor (do_close_cleanup, saved_fd, xfree);
 }
 
 /* Helper function which does the work for make_cleanup_fclose.  */

Re: RFA: open-related cleanup handling

[Pedro Alves]

On Thursday 30 October 2008 19:41:58, Tom Tromey wrote:
> How about this?  I used make_cleanup_dtor -- same difference though.

Great.  Thanks, this is OK.

> Built & regtested on x86-64 (compile farm).
> 
> What do you think of making make_my_cleanup and make_my_cleanup2 static?
> They aren't used outside of utils.c.

Fine with me.

-- 
Pedro Alves