From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23429 invoked by alias); 3 Jun 2013 13:32:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 23418 invoked by uid 89); 3 Jun 2013 13:32:24 -0000 X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_HOSTKARMA_NO,TW_BJ,TW_YM autolearn=no version=3.3.1 Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 03 Jun 2013 13:32:23 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 507672EBEA; Mon, 3 Jun 2013 09:32:21 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id LSuGxjZ8KWwv; Mon, 3 Jun 2013 09:32:21 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D21582EB2F; Mon, 3 Jun 2013 09:32:20 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id CA48AC255A; Mon, 3 Jun 2013 17:32:14 +0400 (RET) Date: Mon, 03 Jun 2013 13:32:00 -0000 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 13/40] fix cleanup handling in macho_symfile_read Message-ID: <20130603133214.GA23924@adacore.com> References: <668178b2c763a59e4651dabd10ee38fdddc37d2a.1368124285.git.tromey@redhat.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="GvXjxJ+pjyke8COw" Content-Disposition: inline In-Reply-To: <668178b2c763a59e4651dabd10ee38fdddc37d2a.1368124285.git.tromey@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Virus-Found: No X-SW-Source: 2013-06/txt/msg00014.txt.bz2 --GvXjxJ+pjyke8COw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 3608 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 --GvXjxJ+pjyke8COw Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-Revert-fix-cleanup-handling-in-macho_symfile_read.patch" Content-length: 1679 >From 47ca19ca1b982adb3e63f00f6e132942a2e624dc Mon Sep 17 00:00:00 2001 From: Joel Brobecker 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 + + Revert (indirectly causes a SIGSEGV): + * machoread.c (macho_symfile_read): Assign first cleanup to + 'back_to'. + 2013-06-03 Yao Qi * 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 --GvXjxJ+pjyke8COw--