[patch] Memory leak fixes

[patch] Memory leak fixes

[Ali Lakhia]

[-- Attachment #1: Type: text/plain, Size: 143 bytes --]

Please see fixes for memory leaks.  My testing for these has been
limited to just making sure things work for my day-to-day use. Thanks,

-Ali

[-- Attachment #2: patch2.txt --]
[-- Type: text/plain, Size: 11755 bytes --]

--- gdb-7.1/bfd/elf32-i386.c	2010-09-14 16:59:40.000000000 -0700
+++ gdb-7.1/bfd/elf32-i386.c	2010-06-14 15:04:05.000000000 -0700
@@ -835,6 +835,7 @@
   ret->loc_hash_memory = objalloc_create ();
   if (!ret->loc_hash_table || !ret->loc_hash_memory)
     {
+      objalloc_free (ret->loc_hash_memory);
       free (ret);
       return NULL;
     }
diff -ru gdb-7.1/bfd/elf64-x86-64.c gdb-7.1/bfd/elf64-x86-64.c
--- gdb-7.1/bfd/elf64-x86-64.c	2010-09-14 16:59:40.000000000 -0700
+++ gdb-7.1/bfd/elf64-x86-64.c	2010-06-14 15:01:55.000000000 -0700
@@ -647,6 +647,7 @@
   ret->loc_hash_memory = objalloc_create ();
   if (!ret->loc_hash_table || !ret->loc_hash_memory)
     {
+      objalloc_free (ret->loc_hash_memory);
       free (ret);
       return NULL;
     }
diff -ru gdb-7.1/gdb/dwarf2read.c gdb-7.1/gdb/dwarf2read.c
--- gdb-7.1/gdb/dwarf2read.c	2010-09-14 16:59:40.000000000 -0700
+++ gdb-7.1/gdb/dwarf2read.c	2010-06-15 14:53:54.000000000 -0700
@@ -5462,6 +5462,7 @@
   set_descriptive_type (type, die, cu);
 
   do_cleanups (back_to);
+  xfree (range_types);
 
   /* Install the type in the die. */
   return set_die_type (die, type, cu);
diff -ru gdb-7.1/gdb/elfread.c gdb-7.1/gdb/elfread.c
--- gdb-7.1/gdb/elfread.c	2010-09-14 16:59:40.000000000 -0700
+++ gdb-7.1/gdb/elfread.c	2010-06-15 14:53:54.000000000 -0700
@@ -566,6 +566,7 @@
 	    }
 	}
     }
+  xfree (sectinfo);
 }
 
 struct build_id
--- gdb-7.1/gdb/gdbserver/hostio.c	2010-09-14 16:59:40.000000000 -0700
+++ gdb-7.1/gdb/gdbserver/hostio.c	2010-06-14 12:18:11.000000000 -0700
@@ -367,6 +367,7 @@
       || require_data (p, packet_len - (p - own_buf), &data, &len))
     {
       hostio_packet_error (own_buf);
+      free (data);
       return;
     }
 
--- gdb-7.1/gdb/maint.c	2010-09-14 16:59:40.000000000 -0700
+++ gdb-7.1/gdb/maint.c	2010-06-15 14:53:54.000000000 -0700
@@ -608,8 +608,10 @@
     }
 
   if (!start_ptr || !end_ptr)
-    replacement = NULL;
-
+    {
+      xfree (replacement);
+      replacement = NULL;
+    }
 
   /* If they used an alias, we only want to deprecate the alias.
 
diff -ru gdb-7.1/gdb/mi/mi-cmd-disas.c gdb-7.1/gdb/mi/mi-cmd-disas.c
--- gdb-7.1/gdb/mi/mi-cmd-disas.c	2010-09-14 17:03:28.000000000 -0700
+++ gdb-7.1/gdb/mi/mi-cmd-disas.c	2010-06-14 14:43:18.000000000 -0700
@@ -158,4 +158,5 @@
   		   file_string,
 		   mixed_source_and_assembly? DISASSEMBLY_SOURCE : 0,
 		   how_many, low, high);
+  xfree (file_string);
 }
diff -ru gdb-7.1/gdb/mi/mi-cmd-env.c gdb-7.1/gdb/mi/mi-cmd-env.c
--- gdb-7.1/gdb/mi/mi-cmd-env.c	2010-09-14 17:03:28.000000000 -0700
+++ gdb-7.1/gdb/mi/mi-cmd-env.c	2010-06-14 11:57:02.000000000 -0700
@@ -276,4 +276,5 @@
   if (!env)
     env = "";
   orig_path = xstrdup (env);
+  free_environ(environment);
 }
diff -ru gdb-7.1/gdb/printcmd.c gdb-7.1/gdb/printcmd.c
--- gdb-7.1/gdb/printcmd.c	2010-09-14 17:03:28.000000000 -0700
+++ gdb-7.1/gdb/printcmd.c	2010-06-15 14:53:54.000000000 -0700
@@ -608,6 +608,7 @@
     fputs_filtered (">", stream);
 
   do_cleanups (cleanup_chain);
+  xfree (filename);
 }
 
 /* Given an address ADDR return all the elements needed to print the
diff -ru gdb-7.1/gdb/python/py-cmd.c gdb-7.1/gdb/python/py-cmd.c
--- gdb-7.1/gdb/python/py-cmd.c	2010-09-14 17:03:28.000000000 -0700
+++ gdb-7.1/gdb/python/py-cmd.c	2010-06-14 14:06:14.000000000 -0700
@@ -431,7 +431,10 @@
 	  pfx_name[out] = '\0';
 	}
       else if (cmp < 0)
-	  return -1;
+        {
+          xfree (cmd_name);
+          return -1;
+        }
     }
   if (PyObject_HasAttr (self, gdbpy_doc_cst))
     {
diff -ru gdb-7.1/gdb/python/py-type.c gdb-7.1/gdb/python/py-type.c
--- gdb-7.1/gdb/python/py-type.c	2010-09-14 17:03:28.000000000 -0700
+++ gdb-7.1/gdb/python/py-type.c	2010-06-14 14:04:28.000000000 -0700
@@ -580,6 +580,7 @@
 
       type_print (type_object_to_type (self), "", stb, -1);
 
+      xfree (thetype);
       thetype = ui_file_xstrdup (stb, &length);
       do_cleanups (old_chain);
     }
diff -ru gdb-7.1/gdb/python/py-value.c gdb-7.1/gdb/python/py-value.c
--- gdb-7.1/gdb/python/py-value.c	2010-09-14 17:03:28.000000000 -0700
+++ gdb-7.1/gdb/python/py-value.c	2010-06-14 15:28:18.000000000 -0700
@@ -407,6 +407,7 @@
     {
       common_val_print (((value_object *) self)->value, stb, 0,
 			&opts, python_language);
+      xfree (s);
       s = ui_file_xstrdup (stb, NULL);
     }
   GDB_PY_HANDLE_EXCEPTION (except);
diff -ru gdb-7.1/gdb/ser-unix.c gdb-7.1/gdb/ser-unix.c
--- gdb-7.1/gdb/ser-unix.c	2010-09-14 17:03:28.000000000 -0700
+++ gdb-7.1/gdb/ser-unix.c	2010-06-15 14:53:54.000000000 -0700
@@ -183,7 +183,10 @@
   state = (struct hardwire_ttystate *) xmalloc (sizeof *state);
 
   if (get_tty_state (scb, state))
-    return NULL;
+    {
+      xfree (state);
+      return NULL;
+    }
 
   return (serial_ttystate) state;
 }
diff -ru gdb-7.1/gdb/symfile.c gdb-7.1/gdb/symfile.c
--- gdb-7.1/gdb/symfile.c	2010-09-15 14:45:22.000000000 -0700
+++ gdb-7.1/gdb/symfile.c	2010-06-15 14:53:54.000000000 -0700
@@ -2176,6 +2176,7 @@
      frameless.  */
   reinit_frame_cache ();
   do_cleanups (my_cleanups);
+  xfree (sect_opts);
 }
 \f
 
@@ -2338,9 +2339,6 @@
 	  memset (&objfile->msymbol_demangled_hash, 0,
 		  sizeof (objfile->msymbol_demangled_hash));
 
-	  objfile->psymbol_cache = bcache_xmalloc ();
-	  objfile->macro_cache = bcache_xmalloc ();
-	  objfile->filename_cache = bcache_xmalloc ();
 	  /* obstack_init also initializes the obstack so it is
 	     empty.  We could use obstack_specify_allocation but
 	     gdb_obstack.h specifies the alloc/dealloc
diff -ru gdb-7.1/gdb/target.c gdb-7.1/gdb/target.c
--- gdb-7.1/gdb/target.c	2010-09-14 17:06:46.000000000 -0700
+++ gdb-7.1/gdb/target.c	2010-06-15 14:53:54.000000000 -0700
@@ -1907,16 +1907,21 @@
 target_read_stralloc (struct target_ops *ops, enum target_object object,
 		      const char *annex)
 {
-  gdb_byte *buffer;
+  gdb_byte *buffer = NULL;
   LONGEST transferred;
 
   transferred = target_read_alloc_1 (ops, object, annex, &buffer, 1);
 
   if (transferred < 0)
-    return NULL;
-
+    {
+      xfree (buffer);
+      return NULL;
+    }
   if (transferred == 0)
-    return xstrdup ("");
+    {
+      xfree (buffer);
+      return xstrdup ("");
+    }
 
   buffer[transferred] = 0;
   if (strlen (buffer) < transferred)
diff -ru gdb-7.1/gdb/top.c gdb-7.1/gdb/top.c
--- gdb-7.1/gdb/top.c	2010-09-14 17:06:46.000000000 -0700
+++ gdb-7.1/gdb/top.c	2010-09-16 14:21:49.000000000 -0700
@@ -960,7 +960,10 @@
   immediate_quit--;
 
   if (got_eof)
-    return NULL;
+    {
+      xfree (rl);		/* Allocated in readline.  */
+      return NULL;
+    }
 
 #define SERVER_COMMAND_LENGTH 7
   server_command =
diff -ru gdb-7.1/gdb/tracepoint.c gdb-7.1/gdb/tracepoint.c
--- gdb-7.1/gdb/tracepoint.c	2010-09-14 17:06:46.000000000 -0700
+++ gdb-7.1/gdb/tracepoint.c	2010-06-15 14:53:54.000000000 -0700
@@ -1233,7 +1233,11 @@
       return NULL;
     }
   else
-    return *str_list;
+    {
+      char **ret = *str_list;
+      xfree (str_list);
+      return ret;
+    }
 }
 
 /* Render all actions into gdb protocol.  */
@@ -2076,6 +2080,8 @@
     error (_("requires an argument (function, line or *addr) to define a scope"));
 
   sals = decode_line_1 (&args, 1, NULL, 0, &canonical, NULL);
+  xfree (canonical);
+
   if (sals.nelts == 0)
     return;		/* presumably decode_line_1 has already warned */
 
@@ -2772,7 +2778,7 @@
     sprintf (buf, "%s_%d", namebase, try_num++);
 
   /* We have an available name, create the variable.  */
-  tsv = create_trace_state_variable (xstrdup (buf));
+  tsv = create_trace_state_variable (buf);
   tsv->initial_value = utsv->initial_value;
   tsv->builtin = utsv->builtin;
 
diff -ru gdb-7.1/gdb/tui/tui-regs.c gdb-7.1/gdb/tui/tui-regs.c
--- gdb-7.1/gdb/tui/tui-regs.c	2010-09-14 17:10:06.000000000 -0700
+++ gdb-7.1/gdb/tui/tui-regs.c	2010-06-14 14:23:18.000000000 -0700
@@ -257,6 +257,7 @@
 	  TUI_DATA_WIN->generic.content = (void*) NULL;
 	  TUI_DATA_WIN->generic.content_size = 0;
 	  tui_add_content_elements (&TUI_DATA_WIN->generic, nr_regs);
+	  xfree (display_info->regs_content);
 	  display_info->regs_content
             = (tui_win_content) TUI_DATA_WIN->generic.content;
 	  display_info->regs_content_count = nr_regs;
diff -ru gdb-7.1/gdb/tui/tui-winsource.c gdb-7.1/gdb/tui/tui-winsource.c
--- gdb-7.1/gdb/tui/tui-winsource.c	2010-09-14 17:10:06.000000000 -0700
+++ gdb-7.1/gdb/tui/tui-winsource.c	2010-06-14 12:52:41.000000000 -0700
@@ -625,6 +625,7 @@
 	  src_line_buf + (line_width * i);
     }
 
+  xfree (src_line_buf);
   return TUI_SUCCESS;
 }
 
diff -ru gdb-7.1/gdb/typeprint.c gdb-7.1/gdb/typeprint.c
--- gdb-7.1/gdb/typeprint.c	2010-09-14 17:10:06.000000000 -0700
+++ gdb-7.1/gdb/typeprint.c	2010-06-15 14:53:54.000000000 -0700
@@ -97,6 +97,7 @@
   TRY_CATCH (except, RETURN_MASK_ALL)
     {
       type_print (type, "", stb, -1);
+      xfree (s);
       s = ui_file_xstrdup (stb, NULL);
     }
   if (except.reason < 0)
diff -ru gdb-7.1/gdb/varobj.c gdb-7.1/gdb/varobj.c
--- gdb-7.1/gdb/varobj.c	2010-03-03 10:33:04.000000000 -0800
+++ gdb-7.1/gdb/varobj.c	2010-06-15 14:53:54.000000000 -0700
@@ -760,6 +760,7 @@
 		 mycount);
     }
 
+  xfree (result);
   return delcount;
 }
 
@@ -2564,6 +2565,7 @@
   thevalue = ui_file_xstrdup (stb, NULL);
 
   do_cleanups (old_chain);
+  xfree (encoding);
   return thevalue;
 }
 
diff -ru gdb-7.1/gdb/xml-support.c gdb-7.1/gdb/xml-support.c
--- gdb-7.1/gdb/xml-support.c	2009-12-31 23:31:46.000000000 -0800
+++ gdb-7.1/gdb/xml-support.c	2010-06-15 14:53:54.000000000 -0700
@@ -1077,6 +1077,7 @@
 	{
 	  warning (_("Read error from \"%s\""), filename);
 	  do_cleanups (back_to);
+	  xfree (text);
 	  return NULL;
 	}
 
diff -ru gdb-7.1/libiberty/argv.c gdb-7.1/libiberty/argv.c
--- gdb-7.1/libiberty/argv.c	2009-10-08 21:49:48.000000000 -0700
+++ gdb-7.1/libiberty/argv.c	2010-06-14 12:37:06.000000000 -0700
@@ -402,7 +402,7 @@
       size_t len;
       /* A dynamically allocated buffer used to hold options read from a
 	 response file.  */
-      char *buffer;
+      char *buffer = NULL;
       /* Dynamically allocated storage for the options read from the
 	 response file.  */
       char **file_argv;
@@ -423,6 +423,7 @@
 	goto error;
       if (fseek (f, 0L, SEEK_SET) == -1)
 	goto error;
+      xfree (buffer);
       buffer = (char *) xmalloc (pos * sizeof (char) + 1);
       len = fread (buffer, sizeof (char), pos, f);
       if (len != (size_t) pos
--- gdb-7.1/readline/bind.c	2006-05-05 11:26:12.000000000 -0700
+++ gdb-7.1/readline/bind.c	2010-06-14 12:33:46.000000000 -0700
@@ -370,7 +370,10 @@
 
       ic = uc;
       if (ic < 0 || ic >= KEYMAP_SIZE)
-	return -1;
+      {
+        free (keys);
+        return -1;
+      }
 
       if (META_CHAR (ic) && _rl_convert_meta_chars_to_ascii)
 	{
@@ -2068,6 +2071,7 @@
 	    }
 	}
     }
+  free (names);
 }
 
 /* Print all of the current functions and their bindings to
diff -ru gdb-7.1/readline/kill.c gdb-7.1/readline/kill.c
--- gdb-7.1/readline/kill.c	2006-05-05 11:26:12.000000000 -0700
+++ gdb-7.1/readline/kill.c	2010-06-14 12:30:56.000000000 -0700
@@ -581,6 +581,7 @@
   arg = history_arg_extract (count, count, entry->line);
   if (!arg || !*arg)
     {
+      free (arg);
       rl_ding ();
       return -1;
     }
diff -ru gdb-7.1/readline/util.c gdb-7.1/readline/util.c
--- gdb-7.1/readline/util.c	2006-05-05 11:26:12.000000000 -0700
+++ gdb-7.1/readline/util.c	2010-06-14 11:33:41.000000000 -0700
@@ -185,6 +185,7 @@
     {
       homedir = tilde_expand ("~");
       _rl_replace_text (homedir, start, end);
+      free (homedir);
       return (0);
     }
   else if (rl_line_buffer[start] != '~')
@@ -215,6 +216,7 @@
       free (temp);
 
       _rl_replace_text (homedir, start, end);
+      free (homedir);
     }
 
   return (0);

Re: [patch] Memory leak fixes

[Joel Brobecker]

Ali,

> Please see fixes for memory leaks.  My testing for these has been
> limited to just making sure things work for my day-to-day use. Thanks,

First and foremost, thanks a lot for your submissions. A couple
of things, however, mostly procedural, I'm afraid:

  1. Your contributions are reaching a point where we can no longer
     take them under the `tiny change' rule.  The rule of thumb is
     around 15 lines, and you are well beyond that.  So, for us to
     continue incorporating your changes, we'll need a copyright
     assignment on file.

     I can get you started on the paperwork, if you would agree to
     assigning your GDB changes to the FSF.

  2. Changes in bfd should be sent as a separate patch to binutils.
     Please see the binutils/MAINTAINERS file for more details.

  3. Changes in GDB should be submitted following a procedure that
     is documented in gdb/CONTRIBUTE.  You're almost complient,
     but you're missing at least the ChangeLog entry.  Also, we
     really appreciate if you can run the testsuite before submitting
     the change.  Running the testsuite before and after your patch
     should not reveal any regression.

-- 
Joel

Re: [patch] Memory leak fixes

[Tom Tromey]

>>>>> "Ali" == Ali Lakhia <lakhia@alumni.utexas.net> writes:

Ali> Please see fixes for memory leaks.  My testing for these has been
Ali> limited to just making sure things work for my day-to-day use. Thanks,

In addition to what Joel said, I have a couple notes on the patch itself.q

Ali> diff -ru gdb-7.1/gdb/dwarf2read.c gdb-7.1/gdb/dwarf2read.c
Ali> --- gdb-7.1/gdb/dwarf2read.c	2010-09-14 16:59:40.000000000 -0700
Ali> +++ gdb-7.1/gdb/dwarf2read.c	2010-06-15 14:53:54.000000000 -0700
Ali> @@ -5462,6 +5462,7 @@
Ali>    set_descriptive_type (type, die, cu);
Ali>    do_cleanups (back_to);

Ali> +  xfree (range_types);

This one seems to be fixed in CVS.
There is a cleanup covering range_types.

Actually, I see this cleanup in 7.1 as well.
How did you find this leak?

Maybe the logic around creating the cleanup is wrong.

Ali> diff -ru gdb-7.1/gdb/printcmd.c gdb-7.1/gdb/printcmd.c
Ali> --- gdb-7.1/gdb/printcmd.c	2010-09-14 17:03:28.000000000 -0700
Ali> +++ gdb-7.1/gdb/printcmd.c	2010-06-15 14:53:54.000000000 -0700
Ali> @@ -608,6 +608,7 @@
Ali>      fputs_filtered (">", stream);
 
Ali>    do_cleanups (cleanup_chain);
Ali> +  xfree (filename);
Ali>  }

This one is also covered by a cleanup.

Ali> diff -ru gdb-7.1/gdb/python/py-type.c gdb-7.1/gdb/python/py-type.c
Ali> --- gdb-7.1/gdb/python/py-type.c	2010-09-14 17:03:28.000000000 -0700
Ali> +++ gdb-7.1/gdb/python/py-type.c	2010-06-14 14:04:28.000000000 -0700
Ali> @@ -580,6 +580,7 @@
 
Ali>        type_print (type_object_to_type (self), "", stb, -1);
 
Ali> +      xfree (thetype);
Ali>        thetype = ui_file_xstrdup (stb, &length);
Ali>        do_cleanups (old_chain);

I didn't check this one, but if you want to insert a free before a call
to do_cleanups, usually that means you should create a cleanup at the
point of allocation.

Tom

Re: [patch] Memory leak fixes

[Ali Lakhia]

On Tue, Oct 5, 2010 at 3:47 PM, Tom Tromey <tromey@redhat.com> wrote:

> Ali> diff -ru gdb-7.1/gdb/dwarf2read.c gdb-7.1/gdb/dwarf2read.c
> Ali> --- gdb-7.1/gdb/dwarf2read.c       2010-09-14 16:59:40.000000000 -0700
> Ali> +++ gdb-7.1/gdb/dwarf2read.c       2010-06-15 14:53:54.000000000 -0700
> Ali> @@ -5462,6 +5462,7 @@
> Ali>    set_descriptive_type (type, die, cu);
> Ali>    do_cleanups (back_to);
>
> Ali> +  xfree (range_types);
>
> This one seems to be fixed in CVS.
> There is a cleanup covering range_types.

Are you referring to the call to make_cleanup() ? That gets called
only if ndim == 0 and I can't really figure out what it is supposed to
do with the arg other than saving it in a cleanup struct and returning
that struct.

> Actually, I see this cleanup in 7.1 as well.
> How did you find this leak?
>
> Maybe the logic around creating the cleanup is wrong.

I found the leak using a static checker. It is known to have false
positives!  :-)

> Ali> diff -ru gdb-7.1/gdb/printcmd.c gdb-7.1/gdb/printcmd.c
> Ali> --- gdb-7.1/gdb/printcmd.c 2010-09-14 17:03:28.000000000 -0700
> Ali> +++ gdb-7.1/gdb/printcmd.c 2010-06-15 14:53:54.000000000 -0700
> Ali> @@ -608,6 +608,7 @@
> Ali>      fputs_filtered (">", stream);
>
> Ali>    do_cleanups (cleanup_chain);
> Ali> +  xfree (filename);
> Ali>  }
>
> This one is also covered by a cleanup.

I don't see any cleanup calls. filename gets allocated by
build_address_symbolic() and nothing is freeing it as far as I can
tell.

> Ali> diff -ru gdb-7.1/gdb/python/py-type.c gdb-7.1/gdb/python/py-type.c
> Ali> --- gdb-7.1/gdb/python/py-type.c   2010-09-14 17:03:28.000000000 -0700
> Ali> +++ gdb-7.1/gdb/python/py-type.c   2010-06-14 14:04:28.000000000 -0700
> Ali> @@ -580,6 +580,7 @@
>
> Ali>        type_print (type_object_to_type (self), "", stb, -1);
>
> Ali> +      xfree (thetype);
> Ali>        thetype = ui_file_xstrdup (stb, &length);
> Ali>        do_cleanups (old_chain);
>
> I didn't check this one, but if you want to insert a free before a call
> to do_cleanups, usually that means you should create a cleanup at the
> point of allocation.

The allocation is happing at the very next line at: ui_file_xstrdup()
and the variable is used after the loop (I'm assuming TRY_CATCH macro
can loop). So, it was safer to free to memory before the leak happens
rather than after allocation,

-Ali

Re: [patch] Memory leak fixes

[Tom Tromey]

>>>>> "Ali" == Ali Lakhia <lakhia@alumni.utexas.net> writes:

Ali> I found the leak using a static checker. It is known to have false
Ali> positives!  :-)

Ok, I suspected that might be the case.

Ali>    do_cleanups (cleanup_chain);
Ali> +  xfree (filename);

Tom> This one is also covered by a cleanup.

Ali> I don't see any cleanup calls. filename gets allocated by
Ali> build_address_symbolic() and nothing is freeing it as far as I can
Ali> tell.

Near the top of the function:

  struct cleanup *cleanup_chain = make_cleanup (free_current_contents, &name);
  make_cleanup (free_current_contents, &filename);

This installs a cleanup that frees `name'.  It is run by the do_cleanups
call at the end of the function.

I think the GDB internals manual has a section on "cleanups" and how
they interact with GDB's exception mechanism.  An analyzer looking for
leaks has to know about this, because it is used pervasively throughout
GDB.

Tom