Patchwork [RFAv3,1/2] Fix leak of identifier in macro definition.

login
register
mail settings
Submitter Philippe Waroquiers
Date Jan. 26, 2019, 10:31 p.m.
Message ID <20190126223117.6718-2-philippe.waroquiers@skynet.be>
Download mbox | patch
Permalink /patch/31220/
State New
Headers show

Comments

Philippe Waroquiers - Jan. 26, 2019, 10:31 p.m.
Valgrind detects leaks like the following (gdb.base/macscp.exp).
This patch fixes 1 of the 3 leaks (the last one in the list below).

The remaining leaks are better fixed in splay_tree_remove  and
splay_tree_insert in libiberty.
Tested on debian/amd64, natively and under valgrind.

==22285== 64 (48 direct, 16 indirect) bytes in 1 blocks are definitely lost in loss record 737 of 3,377
==22285==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==22285==    by 0x4049E7: xmalloc (common-utils.c:44)
==22285==    by 0x533A20: new_macro_key(macro_table*, char const*, macro_source_file*, int) (macrotab.c:355)
==22285==    by 0x53438B: macro_define_function(macro_source_file*, int, char const*, int, char const**, char const*) (macrotab.c:822)
==22285==    by 0x52F945: macro_define_command(char const*, int) (macrocmd.c:409)
...
==22285== 128 (96 direct, 32 indirect) bytes in 2 blocks are definitely lost in loss record 1,083 of 3,377
==22285==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==22285==    by 0x4049E7: xmalloc (common-utils.c:44)
==22285==    by 0x533A20: new_macro_key(macro_table*, char const*, macro_source_file*, int) (macrotab.c:355)
==22285==    by 0x534277: macro_define_object_internal(macro_source_file*, int, char const*, char const*, macro_special_kind) (macrotab.c:776)
==22285==    by 0x52F7E0: macro_define_command(char const*, int) (macrocmd.c:414)
...
==22285== 177 bytes in 19 blocks are definitely lost in loss record 1,193 of 3,377
==22285==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==22285==    by 0x4049E7: xmalloc (common-utils.c:44)
==22285==    by 0x52F5BD: extract_identifier(char const**, int) (macrocmd.c:316)
==22285==    by 0x52F77D: macro_define_command(char const*, int) (macrocmd.c:355)

This is the second version of the patch.

Compared to first version, the changes are:
  * Handled the comment of Simon to have extract_identifier returning
    a unique_ptr.
  * Handled the comment of Tom that suggested rather to fix one of the
    leaks in splay-tree.c (which is a libiberty patch).

gdb/ChangeLog
2019-01-26  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* macrocmd.c (extract_identifier): Return
	a gdb::unique_xmalloc_ptr<char> instead of a char *, and update
	callers.
---
 gdb/macrocmd.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
Simon Marchi - Feb. 6, 2019, 4:08 a.m.
On 2019-01-26 17:31, Philippe Waroquiers wrote:
> Valgrind detects leaks like the following (gdb.base/macscp.exp).
> This patch fixes 1 of the 3 leaks (the last one in the list below).
> 
> The remaining leaks are better fixed in splay_tree_remove  and
> splay_tree_insert in libiberty.
> Tested on debian/amd64, natively and under valgrind.
> 
> ==22285== 64 (48 direct, 16 indirect) bytes in 1 blocks are definitely
> lost in loss record 737 of 3,377
> ==22285==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
> ==22285==    by 0x4049E7: xmalloc (common-utils.c:44)
> ==22285==    by 0x533A20: new_macro_key(macro_table*, char const*,
> macro_source_file*, int) (macrotab.c:355)
> ==22285==    by 0x53438B: macro_define_function(macro_source_file*,
> int, char const*, int, char const**, char const*) (macrotab.c:822)
> ==22285==    by 0x52F945: macro_define_command(char const*, int)
> (macrocmd.c:409)
> ...
> ==22285== 128 (96 direct, 32 indirect) bytes in 2 blocks are
> definitely lost in loss record 1,083 of 3,377
> ==22285==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
> ==22285==    by 0x4049E7: xmalloc (common-utils.c:44)
> ==22285==    by 0x533A20: new_macro_key(macro_table*, char const*,
> macro_source_file*, int) (macrotab.c:355)
> ==22285==    by 0x534277:
> macro_define_object_internal(macro_source_file*, int, char const*,
> char const*, macro_special_kind) (macrotab.c:776)
> ==22285==    by 0x52F7E0: macro_define_command(char const*, int)
> (macrocmd.c:414)
> ...
> ==22285== 177 bytes in 19 blocks are definitely lost in loss record
> 1,193 of 3,377
> ==22285==    at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
> ==22285==    by 0x4049E7: xmalloc (common-utils.c:44)
> ==22285==    by 0x52F5BD: extract_identifier(char const**, int) 
> (macrocmd.c:316)
> ==22285==    by 0x52F77D: macro_define_command(char const*, int)
> (macrocmd.c:355)
> 
> This is the second version of the patch.
> 
> Compared to first version, the changes are:
>   * Handled the comment of Simon to have extract_identifier returning
>     a unique_ptr.
>   * Handled the comment of Tom that suggested rather to fix one of the
>     leaks in splay-tree.c (which is a libiberty patch).

Thanks, this LGTM with a small nit to fix:

> @@ -419,7 +419,7 @@ macro_define_command (const char *exp, int 
> from_tty)
>  static void
>  macro_undef_command (const char *exp, int from_tty)
>  {
> -  char *name;
> +  gdb::unique_xmalloc_ptr<char> name;
> 
>    if (!exp)
>      error (_("usage: macro undef NAME"));
> @@ -428,8 +428,7 @@ macro_undef_command (const char *exp, int from_tty)
>    name = extract_identifier (&exp, 0);

Declare name when assigning it, to avoid constructing an empty object 
and then assigning it.

>    if (! name)

Could you fix this to "name == NULL" or "name == nullptr" while at it?

Thanks!

Simon

Patch

diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
index 706a8353e2..7913c9871c 100644
--- a/gdb/macrocmd.c
+++ b/gdb/macrocmd.c
@@ -288,7 +288,7 @@  skip_ws (const char **expp)
    function will also allow "..." forms as used in varargs macro
    parameters.  */
 
-static char *
+static gdb::unique_xmalloc_ptr<char>
 extract_identifier (const char **expp, int is_parameter)
 {
   char *result;
@@ -317,7 +317,7 @@  extract_identifier (const char **expp, int is_parameter)
   memcpy (result, *expp, len);
   result[len] = '\0';
   *expp += len;
-  return result;
+  return gdb::unique_xmalloc_ptr<char> (result);
 }
 
 struct temporary_macro_definition : public macro_definition
@@ -346,14 +346,13 @@  static void
 macro_define_command (const char *exp, int from_tty)
 {
   temporary_macro_definition new_macro;
-  char *name = NULL;
 
   if (!exp)
     error (_("usage: macro define NAME[(ARGUMENT-LIST)] [REPLACEMENT-LIST]"));
 
   skip_ws (&exp);
-  name = extract_identifier (&exp, 0);
-  if (! name)
+  gdb::unique_xmalloc_ptr<char> name = extract_identifier (&exp, 0);
+  if (name == NULL)
     error (_("Invalid macro name."));
   if (*exp == '(')
     {
@@ -380,7 +379,7 @@  macro_define_command (const char *exp, int from_tty)
 	      /* Must update new_macro as well...  */
 	      new_macro.argv = (const char * const *) argv;
 	    }
-	  argv[new_macro.argc] = extract_identifier (&exp, 1);
+	  argv[new_macro.argc] = extract_identifier (&exp, 1).release ();
 	  if (! argv[new_macro.argc])
 	    error (_("Macro is missing an argument."));
 	  ++new_macro.argc;
@@ -404,14 +403,15 @@  macro_define_command (const char *exp, int from_tty)
       ++exp;
       skip_ws (&exp);
 
-      macro_define_function (macro_main (macro_user_macros), -1, name,
+      macro_define_function (macro_main (macro_user_macros), -1, name.get (),
 			     new_macro.argc, (const char **) new_macro.argv,
 			     exp);
     }
   else
     {
       skip_ws (&exp);
-      macro_define_object (macro_main (macro_user_macros), -1, name, exp);
+      macro_define_object (macro_main (macro_user_macros), -1, name.get (),
+			   exp);
     }
 }
 
@@ -419,7 +419,7 @@  macro_define_command (const char *exp, int from_tty)
 static void
 macro_undef_command (const char *exp, int from_tty)
 {
-  char *name;
+  gdb::unique_xmalloc_ptr<char> name;
 
   if (!exp)
     error (_("usage: macro undef NAME"));
@@ -428,8 +428,7 @@  macro_undef_command (const char *exp, int from_tty)
   name = extract_identifier (&exp, 0);
   if (! name)
     error (_("Invalid macro name."));
-  macro_undef (macro_main (macro_user_macros), -1, name);
-  xfree (name);
+  macro_undef (macro_main (macro_user_macros), -1, name.get ());
 }