[patch/Darwin] Fix cleanup leak in machoread.c:macho_symfile_read

[Joel Brobecker <brobecker at adacore dot com>]

Hello,

This is a followup on...

    http://www.sourceware.org/ml/gdb-patches/2013-06/msg00014.html

... where a patch from Tom making a correct fix for a cleanup leak
in macho_symfile_read (symbol_table)...

          symbol_table = (asymbol **) xmalloc (storage_needed);
          make_cleanup (xfree, symbol_table);

... exposed a series of latent bugs... The triggering patch was
temporarily reverted, and this patch fixes the leak again.

Unfortunately, fixing the leak alone triggers a crash which occurs
while loading the symbols from an executable:

    % gdb
    (gdb) file g_exe
    [SIGSEGV]

The crash is caused by the fact that performing the cleanup
right after the call to macho_symtab_read, as currently done,
is too early.

Indeed, references to this symbol_table get saved in the oso_vector
global during the call to macho_symtab_read via calls to
macho_register_oso, and those references then get accessed
later on, when processing all the OSOs that got pushed (see
call to macho_symfile_read_all_oso).

This patch prevents this by using one single cleanup queue for
the entire function, rather than having additional separate
cleanup queues (Eg: for the handling of the minimal symbols),
thus preventing the premature free'ing of the minimal_symbols
array.

Secondly, this patch takes this opportunity for avoiding the use
of the oso_vector global, thus making it simpler to track its
lifetime.

gdb/ChangeLog:

        * machoread.c (oso_vector): Delete this global.
        (macho_register_oso): Add new parameter "oso_vector_ptr".
        Use it instead of the "oso_vector" global.
        (macho_symtab_read, macho_symfile_read_all_oso): Likewise.
        (macho_symfile_read): Use a local oso_vector, to be free'ed
        at the end of this function, in place of the old "oso_vector"
        global.  Update various function calls accordingly.  Use one
        single cleanup chain for the entire function.

Tested on x86_64-darwin.  Applying this patch also uncovers
another latent bug, tentatively fixed by:

    [RFA/DWARF] do not use dwarf2_per_objfile in dwarf2_per_objfile_free.
    http://www.sourceware.org/ml/gdb-patches/2013-06/msg00231.html

Without it, we'll get a crashing while unloading the symbols
before loading new symbols. For instance:

    (gdb) file g1
    (gdb) file g2
    [SIGSEGV]

I will therefore wait for approval of the RFA/DWARF before applying
this patch.

Thanks,
-- 
Joel

---
 gdb/machoread.c |   50 ++++++++++++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/gdb/machoread.c b/gdb/machoread.c
index 7a4f0c3..8d45f6f 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -64,10 +64,8 @@ typedef struct oso_el
 }
 oso_el;
 
-/* Vector of object files to be read after the executable.  This is one
-   global variable but it's life-time is the one of macho_symfile_read.  */
+/* Vector of object files to be read after the executable.  */
 DEF_VEC_O (oso_el);
-static VEC (oso_el) *oso_vector;
 
 static void
 macho_new_init (struct objfile *objfile)
@@ -83,7 +81,8 @@ macho_symfile_init (struct objfile *objfile)
 /*  Add a new OSO to the vector of OSO to load.  */
 
 static void
-macho_register_oso (struct objfile *objfile,
+macho_register_oso (VEC (oso_el) **oso_vector_ptr,
+		    struct objfile *objfile,
                     asymbol **oso_sym, asymbol **end_sym,
                     unsigned int nbr_syms)
 {
@@ -94,7 +93,7 @@ macho_register_oso (struct objfile *objfile,
   el.oso_sym = oso_sym;
   el.end_sym = end_sym;
   el.nbr_syms = nbr_syms;
-  VEC_safe_push (oso_el, oso_vector, &el);
+  VEC_safe_push (oso_el, *oso_vector_ptr, &el);
 }
 
 /* Add symbol SYM to the minimal symbol table of OBJFILE.  */
@@ -175,7 +174,8 @@ macho_symtab_add_minsym (struct objfile *objfile, const asymbol *sym)
 
 static void
 macho_symtab_read (struct objfile *objfile,
-		   long number_of_symbols, asymbol **symbol_table)
+		   long number_of_symbols, asymbol **symbol_table,
+		   VEC (oso_el) **oso_vector_ptr)
 {
   long i;
   const asymbol *dir_so = NULL;
@@ -299,7 +299,8 @@ macho_symtab_read (struct objfile *objfile,
                 {
                   /* End of file.  */
                   if (state == S_DWARF_FILE)
-                    macho_register_oso (objfile, oso_file, symbol_table + i,
+                    macho_register_oso (oso_vector_ptr, objfile,
+					oso_file, symbol_table + i,
                                         nbr_syms);
                   state = S_NO_SO;
                 }
@@ -642,19 +643,20 @@ macho_add_oso_symfile (oso_el *oso, bfd *abfd,
   do_cleanups (cleanup);
 }
 
-/* Read symbols from the vector of oso files.  */
+/* Read symbols from the vector of oso files.
+
+   Note that this function sorts OSO_VECTOR_PTR.  */
 
 static void
-macho_symfile_read_all_oso (struct objfile *main_objfile, int symfile_flags)
+macho_symfile_read_all_oso (VEC (oso_el) **oso_vector_ptr,
+			    struct objfile *main_objfile,
+			    int symfile_flags)
 {
   int ix;
-  VEC (oso_el) *vec;
+  VEC (oso_el) *vec = *oso_vector_ptr;
   oso_el *oso;
   struct cleanup *cleanup = make_cleanup (null_cleanup, NULL);
 
-  vec = oso_vector;
-  oso_vector = NULL;
-
   /* Sort oso by name so that files from libraries are gathered.  */
   qsort (VEC_address (oso_el, vec), VEC_length (oso_el, vec),
          sizeof (oso_el), oso_el_compare_name);
@@ -773,7 +775,6 @@ macho_symfile_read_all_oso (struct objfile *main_objfile, int symfile_flags)
         }
     }
 
-  VEC_free (oso_el, vec);
   do_cleanups (cleanup);
 }
 
@@ -850,6 +851,8 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
   CORE_ADDR offset;
   long storage_needed;
   bfd *dsym_bfd;
+  VEC (oso_el) *oso_vector = NULL;
+  struct cleanup *old_chain = make_cleanup (VEC_cleanup (oso_el), &oso_vector);
 
   /* Get symbols from the symbol table only if the file is an executable.
      The symbol table of object files is not relocated and is expected to
@@ -867,13 +870,12 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
 	{
 	  asymbol **symbol_table;
 	  long symcount;
-          struct cleanup *back_to;
 
 	  symbol_table = (asymbol **) xmalloc (storage_needed);
 	  make_cleanup (xfree, symbol_table);
 
           init_minimal_symbol_collection ();
-          back_to = make_cleanup_discard_minimal_symbols ();
+          make_cleanup_discard_minimal_symbols ();
 
 	  symcount = bfd_canonicalize_symtab (objfile->obfd, symbol_table);
 
@@ -882,10 +884,9 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
 		   bfd_get_filename (objfile->obfd),
 		   bfd_errmsg (bfd_get_error ()));
 
-	  macho_symtab_read (objfile, symcount, symbol_table);
+	  macho_symtab_read (objfile, symcount, symbol_table, &oso_vector);
 
           install_minimal_symbols (objfile);
-          do_cleanups (back_to);
 	}
 
       /* Try to read .eh_frame / .debug_frame.  */
@@ -901,15 +902,10 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
 	  int ix;
 	  oso_el *oso;
           struct bfd_section *asect, *dsect;
-	  struct cleanup *cleanup;
 
 	  if (mach_o_debug_level > 0)
 	    printf_unfiltered (_("dsym file found\n"));
 
-	  /* Remove oso.  They won't be used.  */
-	  VEC_free (oso_el, oso_vector);
-	  oso_vector = NULL;
-
           /* Set dsym section size.  */
           for (asect = objfile->obfd->sections, dsect = dsym_bfd->sections;
                asect && dsect;
@@ -922,11 +918,11 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
             }
 
 	  /* Add the dsym file as a separate file.  */
-	  cleanup = make_cleanup_bfd_unref (dsym_bfd);
+	  make_cleanup_bfd_unref (dsym_bfd);
           symbol_file_add_separate (dsym_bfd, symfile_flags, objfile);
-	  do_cleanups (cleanup);
 
 	  /* Don't try to read dwarf2 from main file or shared libraries.  */
+	  do_cleanups (old_chain);
           return;
 	}
     }
@@ -939,7 +935,9 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
 
   /* Then the oso.  */
   if (oso_vector != NULL)
-    macho_symfile_read_all_oso (objfile, symfile_flags);
+    macho_symfile_read_all_oso (&oso_vector, objfile, symfile_flags);
+
+  do_cleanups (old_chain);
 }
 
 static bfd_byte *
-- 
1.7.10.4