diff mbox

[RFA] Fix leak in linespec parser.

Message ID 20181125220637.4362-1-philippe.waroquiers@skynet.be
State New
Headers show

Commit Message

Philippe Waroquiers Nov. 25, 2018, 10:06 p.m. UTC
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.

Tested on debian/amd64, + a bunch of tests re-run under valgrind.

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): 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)
...
---
 gdb/linespec.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Philippe Waroquiers Nov. 25, 2018, 10:24 p.m. UTC | #1
On Sun, 2018-11-25 at 23:06 +0100, 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.
> 
> Tested on debian/amd64, + a bunch of tests re-run under valgrind.
Groinch, a few more tests under valgrind shows the fix is not ok:
in case an exception is thrown, the delete-d value is then accessed
in the cleanup.
So, I will further investigate ...

Philippe

> 
> Ok to push ?
(so, not ok).

> 
> 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): 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)
> ...
> ---
>  gdb/linespec.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 00f59f9c28..9eff779eba 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -2117,8 +2117,9 @@ 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);
> +      symtab_vector_up r
> +	= collect_symtabs_from_filename (fullname, self->search_pspace);
> +      delete ls->file_symtabs;
>        ls->file_symtabs = r.release ();
>        use_default = 1;
>      }
> @@ -2401,6 +2402,7 @@ convert_explicit_location_to_linespec (struct linespec_state *self,
>      {
>        TRY
>  	{
> +	  delete result->file_symtabs;
>  	  result->file_symtabs
>  	    = symtabs_from_filename (source_filename,
>  				     self->search_pspace).release ();
> @@ -2630,6 +2632,7 @@ parse_linespec (linespec_parser *parser, const char *arg,
>  	  symtab_vector_up r
>  	    = symtabs_from_filename (user_filename.get (),
>  				     PARSER_STATE (parser)->search_pspace);
> +	  delete PARSER_RESULT (parser)->file_symtabs;
>  	  PARSER_RESULT (parser)->file_symtabs = r.release ();
>  	}
>        CATCH (ex, RETURN_MASK_ERROR)
diff mbox

Patch

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 00f59f9c28..9eff779eba 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2117,8 +2117,9 @@  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);
+      symtab_vector_up r
+	= collect_symtabs_from_filename (fullname, self->search_pspace);
+      delete ls->file_symtabs;
       ls->file_symtabs = r.release ();
       use_default = 1;
     }
@@ -2401,6 +2402,7 @@  convert_explicit_location_to_linespec (struct linespec_state *self,
     {
       TRY
 	{
+	  delete result->file_symtabs;
 	  result->file_symtabs
 	    = symtabs_from_filename (source_filename,
 				     self->search_pspace).release ();
@@ -2630,6 +2632,7 @@  parse_linespec (linespec_parser *parser, const char *arg,
 	  symtab_vector_up r
 	    = symtabs_from_filename (user_filename.get (),
 				     PARSER_STATE (parser)->search_pspace);
+	  delete PARSER_RESULT (parser)->file_symtabs;
 	  PARSER_RESULT (parser)->file_symtabs = r.release ();
 	}
       CATCH (ex, RETURN_MASK_ERROR)