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

Joel Brobecker
2013-06-03T13:32:24Z

[-- 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