From patchwork Thu Aug 17 22:32:06 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 22203 Received: (qmail 88872 invoked by alias); 17 Aug 2017 22:32:39 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 88646 invoked by uid 89); 17 Aug 2017 22:32:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=cust X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 17 Aug 2017 22:32:12 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B84A8883B4; Thu, 17 Aug 2017 22:32:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B84A8883B4 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4690260240; Thu, 17 Aug 2017 22:32:07 +0000 (UTC) Subject: [PATCH] Plug line_header leaks (Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols) To: Philippe Waroquiers , Yao Qi References: <20170811092709.GH8039@1170ee0b50d5> <20170811154542.GK8039@1170ee0b50d5> <86d17umpcg.fsf@gmail.com> <1502973107.1766.64.camel@skynet.be> Cc: Alex Lindsay , "H.J. Lu" , GDB From: Pedro Alves Message-ID: Date: Thu, 17 Aug 2017 23:32:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: On 08/17/2017 06:42 PM, Pedro Alves wrote: > On 08/17/2017 01:31 PM, Philippe Waroquiers wrote: > >> My knowledge of c++ is close to 0, so I cannot help much >> to find the source of the leak. >> I am wondering however who owns the memory allocated >> at dwarf2read.c:9362 : >> line_header_up lh = dwarf_decode_line_header (line_offset, cu); >> when the logic goes later on to line 9389 >> gdb_assert (die->tag != DW_TAG_partial_unit); >> (for info: in the c version 7.11, this assert was followed by >> make_cleanup (free_cu_line_header, cu); >> ) > > That does look like the reason for the leak. I'm taking a look. Here's what I pushed. From 4c8aa72d0eb714a91ca2e47b816d0b4a0cb27843 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 17 Aug 2017 22:53:53 +0100 Subject: [PATCH] Plug line_header leaks This plugs a couple leaks introduced by commit fff8551cf549 ("dwarf2read.c: Some C++fycation, use std::vector, std::unique_ptr"). The first problem is that nothing owns the temporary line_header that handle_DW_AT_stmt_list creates in some cases. Before the commit mentioned above, the temporary line_header case used to have: make_cleanup (free_cu_line_header, cu); and that cleanup was assumed to be run by process_die, after handle_DW_AT_stmt_list returns and before child DIEs were processed. The second problem is found in setup_type_unit_groups: that also used to have a similar make_cleanup call, and ended up with a similar leak after the commit mentioned above. Fix both cases by recording in dwarf2_cu whether a line header is owned by the cu/die, and have process_die explicitly free the line_header if so, making use of a new RAII object that also replaces the reset_die_in_process cleanup, while at it. Thanks to Philippe Waroquiers for noticing the leak and pointing in the right direction. gdb/ChangeLog: 2017-08-17 Pedro Alves * dwarf2read.c (struct dwarf2_cu) : New field. (reset_die_in_process): Delete, replaced by ... (process_die_scope): ... this new class. Make it responsible for freeing cu->line_header too. (process_die): Use process_die_scope. (handle_DW_AT_stmt_list): Record the line header's owner CU/DIE in cu->line_header_die_owner. Don't release the line header if it's owned by the CU. (setup_type_unit_groups): Make the CU/DIE own the line header. Don't release the line header here. --- gdb/ChangeLog | 14 +++++++++ gdb/dwarf2read.c | 89 +++++++++++++++++++++++++++++++++++--------------------- 2 files changed, 70 insertions(+), 33 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index d2c194e..50b7237 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,17 @@ +2017-08-17 Pedro Alves + + * dwarf2read.c (struct dwarf2_cu) : New + field. + (reset_die_in_process): Delete, replaced by ... + (process_die_scope): ... this new class. Make it responsible for + freeing cu->line_header too. + (process_die): Use process_die_scope. + (handle_DW_AT_stmt_list): Record the line header's owner CU/DIE in + cu->line_header_die_owner. Don't release the line header if it's + owned by the CU. + (setup_type_unit_groups): Make the CU/DIE own the line header. + Don't release the line header here. + 2017-08-17 Alex Lindsay (tiny change) * elfread.c (elf_read_minimal_symbols): xfree synthsyms. diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 4f2fdce..0e28144 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -546,6 +546,12 @@ struct dwarf2_cu /* Header data from the line table, during full symbol processing. */ struct line_header *line_header; + /* Non-NULL if LINE_HEADER is owned by this DWARF_CU. Otherwise, + it's owned by dwarf2_per_objfile::line_header_hash. If non-NULL, + this is the DW_TAG_compile_unit die for this CU. We'll hold on + to the line header as long as this DIE is being processed. See + process_die_scope. */ + die_info *line_header_die_owner; /* A list of methods which need to have physnames computed after all type information has been read. */ @@ -8471,28 +8477,44 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu) } } -/* Reset the in_process bit of a die. */ - -static void -reset_die_in_process (void *arg) +/* RAII object that represents a process_die scope: i.e., + starts/finishes processing a DIE. */ +class process_die_scope { - struct die_info *die = (struct die_info *) arg; +public: + process_die_scope (die_info *die, dwarf2_cu *cu) + : m_die (die), m_cu (cu) + { + /* We should only be processing DIEs not already in process. */ + gdb_assert (!m_die->in_process); + m_die->in_process = true; + } - die->in_process = 0; -} + ~process_die_scope () + { + m_die->in_process = false; + + /* If we're done processing the DIE for the CU that owns the line + header, we don't need the line header anymore. */ + if (m_cu->line_header_die_owner == m_die) + { + delete m_cu->line_header; + m_cu->line_header = NULL; + m_cu->line_header_die_owner = NULL; + } + } + +private: + die_info *m_die; + dwarf2_cu *m_cu; +}; /* Process a die and its children. */ static void process_die (struct die_info *die, struct dwarf2_cu *cu) { - struct cleanup *in_process; - - /* We should only be processing those not already in process. */ - gdb_assert (!die->in_process); - - die->in_process = 1; - in_process = make_cleanup (reset_die_in_process,die); + process_die_scope scope (die, cu); switch (die->tag) { @@ -8583,8 +8605,6 @@ process_die (struct die_info *die, struct dwarf2_cu *cu) new_symbol (die, NULL, cu); break; } - - do_cleanups (in_process); } /* DWARF name computation. */ @@ -9362,7 +9382,9 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu, line_header_up lh = dwarf_decode_line_header (line_offset, cu); if (lh == NULL) return; - cu->line_header = lh.get (); + + cu->line_header = lh.release (); + cu->line_header_die_owner = die; if (dwarf2_per_objfile->line_header_hash == NULL) slot = NULL; @@ -9378,6 +9400,7 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu, /* This newly decoded line number information unit will be owned by line_header_hash hash table. */ *slot = cu->line_header; + cu->line_header_die_owner = NULL; } else { @@ -9392,7 +9415,6 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu, dwarf_decode_lines (cu->line_header, comp_dir, cu, NULL, lowpc, decode_mapping); - lh.release (); } /* Process DW_TAG_compile_unit or DW_TAG_partial_unit. */ @@ -9530,7 +9552,8 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu) return; } - cu->line_header = lh.get (); + cu->line_header = lh.release (); + cu->line_header_die_owner = die; if (first_time) { @@ -9541,21 +9564,23 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu) process_full_type_unit still needs to know if this is the first time. */ - tu_group->num_symtabs = lh->file_names.size (); - tu_group->symtabs = XNEWVEC (struct symtab *, lh->file_names.size ()); + tu_group->num_symtabs = cu->line_header->file_names.size (); + tu_group->symtabs = XNEWVEC (struct symtab *, + cu->line_header->file_names.size ()); - for (i = 0; i < lh->file_names.size (); ++i) + for (i = 0; i < cu->line_header->file_names.size (); ++i) { - file_entry &fe = lh->file_names[i]; + file_entry &fe = cu->line_header->file_names[i]; - dwarf2_start_subfile (fe.name, fe.include_dir (lh.get ())); + dwarf2_start_subfile (fe.name, fe.include_dir (cu->line_header)); if (current_subfile->symtab == NULL) { - /* NOTE: start_subfile will recognize when it's been passed - a file it has already seen. So we can't assume there's a - simple mapping from lh->file_names to subfiles, plus - lh->file_names may contain dups. */ + /* NOTE: start_subfile will recognize when it's been + passed a file it has already seen. So we can't + assume there's a simple mapping from + cu->line_header->file_names to subfiles, plus + cu->line_header->file_names may contain dups. */ current_subfile->symtab = allocate_symtab (cust, current_subfile->name); } @@ -9568,16 +9593,14 @@ setup_type_unit_groups (struct die_info *die, struct dwarf2_cu *cu) { restart_symtab (tu_group->compunit_symtab, "", 0); - for (i = 0; i < lh->file_names.size (); ++i) + for (i = 0; i < cu->line_header->file_names.size (); ++i) { - struct file_entry *fe = &lh->file_names[i]; + file_entry &fe = cu->line_header->file_names[i]; - fe->symtab = tu_group->symtabs[i]; + fe.symtab = tu_group->symtabs[i]; } } - lh.release (); - /* The main symtab is allocated last. Type units don't have DW_AT_name so they don't have a "real" (so to speak) symtab anyway. There is later code that will assign the main symtab to all symbols