Hi Tom,

On Thu, May 09, 2013 at 12:51:10PM -0600, Tom Tromey wrote:
> macho_symfile_read leaks a cleanup by assigning to 'back_to' too late.
> 
> 	* machoread.c (macho_symfile_read): Assign first cleanup to
> 	'back_to'.

This patch exposes what looks like a fairly awful memory interdependency
between macho_symfile_read and macho_symfile_read_all_oso via
macho_symtab_read/macho_register_oso.  By doing this, which looks
completely correct, we're actually causing the memory to be released
earlier, which then creates dangling pointers, thus causing a SEGV
during symbol reading. Below is the analysis I submitted internally.

I am planning on looking deeper at how to make things cleaner, and
avoid the use of a global variable for that. But my day is almost
over, and I think it's a significant-enough regression that it's
worth reverting until I get this fixed, which should hopefully be
a day or two at most (a side-effect of the current approach is that
it is harder to follow compared to local variables)! Attached is
the revert, tested on x86_64-darwin - so sorry for undoing this
work, but I will fix things ASAP.

The analysis file at AdaCore:
| Which fixes a cleanup leak, but in the process exposes a memory
| management problem. More precisely, macho_symfile_read allocates
| an array of asymbol's, and installs a cleanup:
| 
|           symbol_table = (asymbol **) xmalloc (storage_needed);
|           back_to = make_cleanup (xfree, symbol_table);
| 
| The old code was not saving the cleanup handle returned by
| make_cleanup (thus the leak). The problem is that the code
| then calls macho_symtab_read before calling do_cleanups:
| 
|           macho_symtab_read (objfile, symcount, symbol_table);
| 
|           install_minimal_symbols (objfile);
|           do_cleanups (back_to);
| 
| This is when things go wrong, because macho_symtab_read registers
| the symbols from SYMBOL_TABLE via macho_register_oso.  The latter
| just stacks the list of OSOs to be processed later, via the call
| to macho_symfile_read_all_oso at the end of macho_symfile_read.
| At that point in time, the cleanup has been done and the data saved
| by macho_symtab_read is now stale.
| 
| Attached is a patch that extends the lifetime of all cleanups to
| the entire function, rather than just the sub-block. This side-steps
| the issue mostly successfully, but I still get a failure in
| segv_on_reload:
| 
|     (gdb) run
|     `/private/var/folders/zz/zyxvpxvq6csfxvn_n00001zc0000gv/T/gdb-TS-LD8OGZ/D109
| +-003__segv_on_reload/g' has changed; re-reading symbols.
|     gdb(66092) malloc: *** error for object 0x7fdf41000ed0: pointer being freed
| +was not allocated
| 
| I think the memory management model between the various functions
| should be more explicit anyway, rather than depend on a global
| variable. So I will work on that.

For the record, you patch was:

> --- a/gdb/machoread.c
> +++ b/gdb/machoread.c
> @@ -871,10 +871,10 @@ macho_symfile_read (struct objfile *objfile, int symfile_flags)
>            struct cleanup *back_to;
>  
>  	  symbol_table = (asymbol **) xmalloc (storage_needed);
> -	  make_cleanup (xfree, symbol_table);
> +	  back_to = 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);

This is what I checked in:

gdb/Changelog:

        Revert:
        * machoread.c (macho_symfile_read): Assign first cleanup to
        'back_to'.

-- 
Joel