From patchwork Mon Nov 26 01:42:22 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 30306 Received: (qmail 20292 invoked by alias); 26 Nov 2018 01:42:28 -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 20279 invoked by uid 89); 26 Nov 2018 01:42:27 -0000 Authentication-Results: sourceware.org; auth=none 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, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=H*Ad:D*ca, ownership X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 26 Nov 2018 01:42:25 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id AE2141E48F; Sun, 25 Nov 2018 20:42:22 -0500 (EST) Subject: Re: [RFAv2] Fix leak in linespec parser. To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20181125230451.6036-1-philippe.waroquiers@skynet.be> From: Simon Marchi Message-ID: <56a8ee8f-a5b3-f34f-6179-abc9c791dd13@simark.ca> Date: Sun, 25 Nov 2018 20:42:22 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: <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 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 > > * 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 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> 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 decode_objc (struct linespec_state *self, linespec_p ls, const char *arg); -static symtab_vector_up symtabs_from_filename +static std::vector symtabs_from_filename (const char *, struct program_space *pspace); static std::vector *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 &names, enum search_domain search_domain); -static symtab_vector_up +static std::vector 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 ()) { 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 &&release_symtabs () { return std::move (m_symtabs); } private: /* The result vector of symtabs. */ - symtab_vector_up m_symtabs; + std::vector 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 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 symtabs_from_filename (const char *filename, struct program_space *search_pspace) { - symtab_vector_up result + std::vector 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,