Synthetic symbol leak in in elf_x86_64_get_synthetic_symtab and elf_read_minimal_symbols

Message ID 86d17umpcg.fsf@gmail.com
State New, archived
Headers

Commit Message

Yao Qi Aug. 17, 2017, 10:59 a.m. UTC
  Alex Lindsay <alexlindsay239@gmail.com> writes:

> I can verify that the objdump example is fixed in HEAD, but I still
> get this leak with `valgrind --leak-check=full
> --show-leak-kinds=definite gdb ./hello`:

Yes, because your patch is not pushed in yet :)  I tweaked your patch a
little bit, and pushed it in.  Again, thanks for your contribution.  Do
you still see other memory leak issues after this fix?
  

Comments

Philippe Waroquiers Aug. 17, 2017, 12:31 p.m. UTC | #1
On Thu, 2017-08-17 at 11:59 +0100, Yao Qi wrote:
> Alex Lindsay <alexlindsay239@gmail.com> writes:
> 
> > I can verify that the objdump example is fixed in HEAD, but I still
> > get this leak with `valgrind --leak-check=full
> > --show-leak-kinds=definite gdb ./hello`:
> 
> Yes, because your patch is not pushed in yet :)  I tweaked your patch a
> little bit, and pushed it in.  Again, thanks for your contribution.  Do
> you still see other memory leak issues after this fix?
> 
I still see significant leaks with 8.0.50.20170817-git
Below a mail I sent to Pedro about this (IIUC, he last modified
 dwarf_decode_line_header memory management, which is the
biggest leak I observed).

Attached the new leaks.

Philippe


-------------------
Hello Pedro,

Following some discussions on gdb patches, I wanted to run
the gdb test suite under Valgrind, just in case it would leak.
But before that, I did a small manual test, and with that,
Valgrind already reports a significant leak, see below.
So, it might be a little bit too early for me to
run the full test suite :).


The test consists in running a very small C executable,
that does a loop of malloc calls:
#include <stdlib.h>

static void test()
  {
    void* leak __attribute__((unused));
    int i;
    for (i = 0; i < 1000; i++)
       leak = (void*)malloc( 1 );
  }
  int main()
  {
    test();
    return 0;
  }


I step inside this small executable to execute a few
malloc calls, then do:
  thread apply all bt full
  info var .*
  info func .*
  x /s leak
  continue
  quit

The biggest leak is as below.
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);
)

==31555== 1,102,606 (124,592 direct, 978,014 indirect) bytes in 1,198 blocks are definitely lost in loss record 12,252 of 12,262
==31555==    at 0x4C28215: operator new(unsigned long) (vg_replace_malloc.c:334)
==31555==    by 0x589266: dwarf_decode_line_header(sect_offset, dwarf2_cu*) (dwarf2read.c:17916)
==31555==    by 0x58BD4C: handle_DW_AT_stmt_list (dwarf2read.c:9362)
==31555==    by 0x58BD4C: read_file_scope (dwarf2read.c:9440)
==31555==    by 0x58BD4C: process_die(die_info*, dwarf2_cu*) (dwarf2read.c:8503)
==31555==    by 0x58FC97: process_full_comp_unit (dwarf2read.c:8289)
==31555==    by 0x58FC97: process_queue (dwarf2read.c:7823)
==31555==    by 0x58FC97: dw2_do_instantiate_symtab(dwarf2_per_cu_data*) (dwarf2read.c:2928)
==31555==    by 0x590D85: dwarf2_read_symtab(partial_symtab*, objfile*) (dwarf2read.c:7689)
==31555==    by 0x619E66: psymtab_to_symtab(objfile*, partial_symtab*) (psymtab.c:775)
==31555==    by 0x61BC8C: psym_expand_symtabs_matching(objfile*, gdb::function_view<bool (char const*, bool)>, gdb::function_view<bool (char const*)>, gdb::function_view<void (compunit_symtab*)>, search_domain) (psymtab.c:1431)
==31555==    by 0x64B7F5: expand_symtabs_matching(gdb::function_view<bool (char const*, bool)>, gdb::function_view<bool (char const*)>, gdb::function_view<void (compunit_symtab*)>, search_domain) (symfile.c:3834)
==31555==    by 0x654E07: search_symbols(char const*, search_domain, int, char const**, symbol_search**) (symtab.c:4307)
==31555==    by 0x655B9A: symtab_symbol_info(char*, search_domain, int) [clone .isra.57] (symtab.c:4557)
==31555==    by 0x493808: cmd_func(cmd_list_element*, char*, int) (cli-decode.c:1902)
==31555==    by 0x677AF5: execute_command(char*, int) (top.c:650)

(there are more than 350 definely or possibly loss records in total with this small test,
I am attaching the full leak report).

Thanks

Philippe

Side question: is there a systematic run of gdb test suite under Valgrind (or similar tool) ?
Once I (more or less) master what is happening, I might setup a night run of gdb test suite
under Valgrind, but that might only be useful if gdb is relatively 'memcheck leak/error' free.
As far as I can see, GDB 7.12 was quite 'leak-free' (the same test only leaked a few blocks)
so it seems we can have significant regressions in that area.
  
Pedro Alves Aug. 17, 2017, 5:42 p.m. UTC | #2
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.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 10d63b0..d2c194e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@ 
+2017-08-17  Alex Lindsay  <alexlindsay239@gmail.com>  (tiny change)
+
+	* elfread.c (elf_read_minimal_symbols): xfree synthsyms.
+
 2017-08-17  Ruslan Kabatsayev  <b7.10110111@gmail.com>
 
 	* NEWS: Mention new shortcuts for nexti and stepi in TUI
diff --git a/gdb/elfread.c b/gdb/elfread.c
index ece704c..a654661 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1132,6 +1132,9 @@  elf_read_minimal_symbols (struct objfile *objfile, int symfile_flags,
 	synth_symbol_table[i] = synthsyms + i;
       elf_symtab_read (reader, objfile, ST_SYNTHETIC, synthcount,
 		       synth_symbol_table.get (), true);
+
+      xfree (synthsyms);
+      synthsyms = NULL;
     }
 
   /* Install any minimal symbols that have been collected as the current