Re: [PATCH 13/40] fix cleanup handling in macho_symfile_read

[Joel Brobecker <brobecker@adacore.com>]

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

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

[-- Attachment #2: 0001-Revert-fix-cleanup-handling-in-macho_symfile_read.patch --]
[-- Type: text/x-diff, Size: 1678 bytes --]

From 47ca19ca1b982adb3e63f00f6e132942a2e624dc Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 3 Jun 2013 17:19:51 +0400
Subject: [PATCH] Revert "fix cleanup handling in macho_symfile_read"

This patch indirectly causes a SEGV by creating a dangling pointer.
Reverting this patch while working on a clearer memory management
method for this part of the code.

gdb/Changelog:

        Revert:
        * machoread.c (macho_symfile_read): Assign first cleanup to
        'back_to'.
---
 gdb/ChangeLog   |    6 ++++++
 gdb/machoread.c |    4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8649045..906cb06 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2013-06-03  Joel Brobecker  <brobecker@adacore.com>
+
+	Revert (indirectly causes a SIGSEGV):
+	* machoread.c (macho_symfile_read): Assign first cleanup to
+	'back_to'.
+
 2013-06-03  Yao Qi  <yao@codesourcery.com>
 
 	* mi/mi-cmd-var.c (mi_no_values, mi_simple_values): Move to
diff --git a/gdb/machoread.c b/gdb/machoread.c
index d294960..9877f07 100644
--- 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);
-	  back_to = make_cleanup (xfree, symbol_table);
+	  make_cleanup (xfree, symbol_table);
 
           init_minimal_symbol_collection ();
-          make_cleanup_discard_minimal_symbols ();
+          back_to = make_cleanup_discard_minimal_symbols ();
 
 	  symcount = bfd_canonicalize_symtab (objfile->obfd, symbol_table);
 
-- 
1.7.10.4