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

Joel Brobecker
2013-06-11T09:26:44Z

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