[PATCH] Plug line_header leaks (Re: Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols)

Pedro Alves
2017-08-17T22:32:39Z

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 <palves@redhat.com>
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 <palves@redhat.com>

* dwarf2read.c (struct dwarf2_cu) <line_header_die_owner>: 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 <palves@redhat.com>
+
+ * dwarf2read.c (struct dwarf2_cu) <line_header_die_owner>: 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 <alexlindsay239@gmail.com> (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);
}
\f
/* 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
--
2.5.5