This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFAv2] Fix leak in linespec parser.
- From: Simon Marchi <simark at simark dot ca>
- To: Philippe Waroquiers <philippe dot waroquiers at skynet dot be>, gdb-patches at sourceware dot org
- Date: Sun, 25 Nov 2018 20:42:22 -0500
- Subject: Re: [RFAv2] Fix leak in linespec parser.
- References: <20181125230451.6036-1-philippe.waroquiers@skynet.be>
On 2018-11-25 6:04 p.m., Philippe Waroquiers wrote:
> Valgrind reports the below leak.
> linespec_parser_new allocates a std::vector<symtab *> at line 2756,
> and stores the pointer to this vector in PARSER_RESULT (parser)->file_symtabs.
> At 3 different places in linespec.c, another std::vector is assigned to
> a linespec->file_symtabs, without first deleting the current value.
>
> The leak is fixed by delete-ing linespec->file_symtabs before
> assigning a new value to it, at 3 different places.
> At one of these places, declare a symtab_vector_up r, so as
> to do the delete of the previous value once a new value has
> been properly built.
>
> Tested on debian/amd64, + a bunch of tests re-run under valgrind
> (including the test that throws an error).
>
> Ok to push ?
>
> gdb/ChangeLog
> 2018-11-25 Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * linespec.c (create_sals_line_offset): Fix leak by deleting
> previous value before assigning new value.
> (convert_explicit_location_to_linespec): First build the
> symtab_vector_up r. Then likewise.
> (parse_linespec): Likewise.
>
> ==798== VALGRIND_GDB_ERROR_BEGIN
> ==798== 32 (24 direct, 8 indirect) bytes in 1 blocks are definitely lost in loss record 447 of 3,143
> ==798== at 0x4C2C48C: operator new(unsigned long) (vg_replace_malloc.c:334)
> ==798== by 0x51D401: linespec_parser_new(ls_parser*, int, language_defn const*, program_space*, symtab*, int, linespec_result*) (linespec.c:2756)
> ==798== by 0x524BF7: decode_line_full(event_location const*, int, program_space*, symtab*, int, linespec_result*, char const*, char const*) (linespec.c:3271)
> ==798== by 0x3E8893: parse_breakpoint_sals(event_location const*, linespec_result*) (breakpoint.c:9067)
> ==798== by 0x3E4E7F: create_breakpoint(gdbarch*, event_location const*, char const*, int, char const*, int, int, bptype, int, auto_boolean, breakpoint_ops const*, int, int, int, unsigned int) (breakpoint.c:9248)
> ==798== by 0x3E55F5: break_command_1(char const*, int, int) (breakpoint.c:9434)
> ==798== by 0x40BA68: cmd_func(cmd_list_element*, char const*, int) (cli-decode.c:1888)
> ==798== by 0x665300: execute_command(char const*, int) (top.c:630)
Hi Philippe,
This looks good, but I think we could simplify that a bit by returning
std::vector objects directly, not dealing with new/delete. We would move
returned data in the existing vector. Would the patch below work? I think
everything is set up correctly move-semantic-wise so that there would be no
copy of vector data
.
>From f135accbec31059b4bff017a1d42361dae25708b Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sun, 25 Nov 2018 20:40:35 -0500
Subject: [PATCH] Fix leak in linespec parser.
---
gdb/linespec.c | 38 +++++++++++++++-----------------------
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/gdb/linespec.c b/gdb/linespec.c
index 00f59f9c2866..e534cf2e81e4 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -77,10 +77,6 @@ enum class linespec_complete_what
KEYWORD,
};
-/* Typedef for unique_ptrs of vectors of symtabs. */
-
-typedef std::unique_ptr<std::vector<symtab *>> symtab_vector_up;
-
/* An address entry is used to ensure that any given location is only
added to the result a single time. It holds an address and the
program space from which the address came. */
@@ -357,7 +353,7 @@ static std::vector<symtab_and_line> decode_objc (struct linespec_state *self,
linespec_p ls,
const char *arg);
-static symtab_vector_up symtabs_from_filename
+static std::vector<symtab *> symtabs_from_filename
(const char *, struct program_space *pspace);
static std::vector<block_symbol> *find_label_symbols
@@ -389,7 +385,7 @@ static void add_all_symbol_names_from_pspace
(struct collect_info *info, struct program_space *pspace,
const std::vector<const char *> &names, enum search_domain search_domain);
-static symtab_vector_up
+static std::vector<symtab *>
collect_symtabs_from_filename (const char *file,
struct program_space *pspace);
@@ -2117,9 +2113,8 @@ create_sals_line_offset (struct linespec_state *self,
set_default_source_symtab_and_line ();
initialize_defaults (&self->default_symtab, &self->default_line);
fullname = symtab_to_fullname (self->default_symtab);
- symtab_vector_up r =
- collect_symtabs_from_filename (fullname, self->search_pspace);
- ls->file_symtabs = r.release ();
+ *ls->file_symtabs
+ = collect_symtabs_from_filename (fullname, self->search_pspace);
use_default = 1;
}
@@ -2401,9 +2396,8 @@ convert_explicit_location_to_linespec (struct linespec_state *self,
{
TRY
{
- result->file_symtabs
- = symtabs_from_filename (source_filename,
- self->search_pspace).release ();
+ *result->file_symtabs
+ = symtabs_from_filename (source_filename, self->search_pspace);
}
CATCH (except, RETURN_MASK_ERROR)
{
@@ -2627,10 +2621,9 @@ parse_linespec (linespec_parser *parser, const char *arg,
/* Check if the input is a filename. */
TRY
{
- symtab_vector_up r
+ *PARSER_RESULT (parser)->file_symtabs
= symtabs_from_filename (user_filename.get (),
PARSER_STATE (parser)->search_pspace);
- PARSER_RESULT (parser)->file_symtabs = r.release ();
}
CATCH (ex, RETURN_MASK_ERROR)
{
@@ -3790,7 +3783,6 @@ class symtab_collector
{
public:
symtab_collector ()
- : m_symtabs (new std::vector<symtab *> ())
{
m_symtab_table = htab_create (1, htab_hash_pointer, htab_eq_pointer,
NULL);
@@ -3805,15 +3797,15 @@ public:
/* Callable as a symbol_found_callback_ftype callback. */
bool operator () (symtab *sym);
- /* Releases ownership of the collected symtabs and returns them. */
- symtab_vector_up release_symtabs ()
+ /* Return an rvalue reference to the collected symtabs. */
+ std::vector<symtab *> &&release_symtabs ()
{
return std::move (m_symtabs);
}
private:
/* The result vector of symtabs. */
- symtab_vector_up m_symtabs;
+ std::vector<symtab *> m_symtabs;
/* This is used to ensure the symtabs are unique. */
htab_t m_symtab_table;
@@ -3828,7 +3820,7 @@ symtab_collector::operator () (struct symtab *symtab)
if (!*slot)
{
*slot = symtab;
- m_symtabs->push_back (symtab);
+ m_symtabs.push_back (symtab);
}
return false;
@@ -3840,7 +3832,7 @@ symtab_collector::operator () (struct symtab *symtab)
SEARCH_PSPACE is not NULL, the search is restricted to just that
program space. */
-static symtab_vector_up
+static std::vector<symtab *>
collect_symtabs_from_filename (const char *file,
struct program_space *search_pspace)
{
@@ -3872,14 +3864,14 @@ collect_symtabs_from_filename (const char *file,
/* Return all the symtabs associated to the FILENAME. If SEARCH_PSPACE is
not NULL, the search is restricted to just that program space. */
-static symtab_vector_up
+static std::vector<symtab *>
symtabs_from_filename (const char *filename,
struct program_space *search_pspace)
{
- symtab_vector_up result
+ std::vector<symtab *> result
= collect_symtabs_from_filename (filename, search_pspace);
- if (result->empty ())
+ if (result.empty ())
{
if (!have_full_symbols () && !have_partial_symbols ())
throw_error (NOT_FOUND_ERROR,
--
2.19.2