From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20264 invoked by alias); 11 Jun 2013 09:26:44 -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 20248 invoked by uid 89); 11 Jun 2013 09:26:44 -0000 X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_NO,RP_MATCHES_RCVD,SPF_PASS,TW_BJ,TW_YM autolearn=ham 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; Tue, 11 Jun 2013 09:26:41 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 131672E55F; Tue, 11 Jun 2013 05:26:40 -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 1ShGsuRWlYns; Tue, 11 Jun 2013 05:26:40 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id CF10C2E3EB; Tue, 11 Jun 2013 05:26:39 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 7CC0FC2B35; Tue, 11 Jun 2013 13:26:38 +0400 (RET) From: Joel Brobecker To: gdb-patches@sourceware.org Cc: Tom Tromey , Tristan Gingold Subject: [patch/Darwin] Fix cleanup leak in machoread.c:macho_symfile_read Date: Tue, 11 Jun 2013 10:21:00 -0000 Message-Id: <1370942792-734-1-git-send-email-brobecker@adacore.com> In-Reply-To: <20130604133005.GE12363@adacore.com> References: <20130604133005.GE12363@adacore.com> X-SW-Source: 2013-06/txt/msg00237.txt.bz2 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