[RFA] Fix leaks in macro definitions.

[RFA] Fix leaks in macro definitions.

[Philippe Waroquiers]

Valgrind detects leaks like the following (gdb.base/macsp.exp).
This patch fixes the 3 leaks.
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)

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

	* macrocmd.c (macro_define_command): Use a unique_xmalloc_ptr
	to maintain name ownership.
	* macrotab.c (macro_define_internal): New function that
	factorizes macro_define_object_internal and macro_define_function
	code.  If the newly inserted node has replaced a existing node,
	call macro_tree_delete_key to free the unused just allocated key.
	(macro_define_object_internal): Use macro_define_internal.
	(macro_define_function): Likewise.
	(macro_undef): Free the key of the removed node.
---
 gdb/macrocmd.c | 10 +++----
 gdb/macrotab.c | 75 +++++++++++++++++++++++++++++---------------------
 2 files changed, 48 insertions(+), 37 deletions(-)

diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
index 706a8353e2..c1c42fa910 100644
--- a/gdb/macrocmd.c
+++ b/gdb/macrocmd.c
@@ -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.get () == NULL)
     error (_("Invalid macro name."));
   if (*exp == '(')
     {
@@ -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);
     }
 }
 
diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 7fcab4691b..b2343c8c29 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -743,21 +743,27 @@ check_for_redefinition (struct macro_source_file *source, int line,
     return 0;
 }
 
-/* A helper function to define a new object-like macro.  */
+/* A helper function to define a new object-like or function-like macro
+   according to KIND.  When KIND is macro_object_like,
+   the macro_special_kind must be provided as ARGC, and ARGV must be NULL.
+   When KIND is macro_function_like, ARGC and ARGV are giving the function
+   arguments.  */
 
 static void
-macro_define_object_internal (struct macro_source_file *source, int line,
-			      const char *name, const char *replacement,
-			      enum macro_special_kind kind)
+macro_define_internal (struct macro_source_file *source, int line,
+                       const char *name, enum macro_kind kind,
+		       int argc, const char **argv,
+                       const char *replacement)
 {
   struct macro_table *t = source->table;
   struct macro_key *k = NULL;
   struct macro_definition *d;
+  splay_tree_node s;
 
   if (! t->redef_ok)
-    k = check_for_redefinition (source, line, 
-				name, macro_object_like,
-				0, 0,
+    k = check_for_redefinition (source, line,
+				name, kind,
+				argc, argv,
 				replacement);
 
   /* If we're redefining a symbol, and the existing key would be
@@ -774,8 +780,28 @@ macro_define_object_internal (struct macro_source_file *source, int line,
     return;
 
   k = new_macro_key (t, name, source, line);
-  d = new_macro_definition (t, macro_object_like, kind, 0, replacement);
-  splay_tree_insert (t->definitions, (splay_tree_key) k, (splay_tree_value) d);
+  d = new_macro_definition (t, kind, argc, argv, replacement);
+  s = splay_tree_insert (t->definitions, (splay_tree_key) k, (splay_tree_value) d);
+  if ((struct macro_key *) s->key != k)
+    {
+      /* If the node inserted is not using k, it means we have a redefinition.
+	 We must free the newly allocated k, as it will not be stored in
+	 the splay tree.  */
+      macro_tree_delete_key (k);
+    }
+}
+
+/* A helper function to define a new object-like macro.  */
+
+static void
+macro_define_object_internal (struct macro_source_file *source, int line,
+			      const char *name, const char *replacement,
+			      enum macro_special_kind special_kind)
+{
+  macro_define_internal (source, line,
+			 name, macro_object_like,
+			 special_kind, 0,
+			 replacement);
 }
 
 void
@@ -802,29 +828,12 @@ macro_define_function (struct macro_source_file *source, int line,
                        const char *name, int argc, const char **argv,
                        const char *replacement)
 {
-  struct macro_table *t = source->table;
-  struct macro_key *k = NULL;
-  struct macro_definition *d;
-
-  if (! t->redef_ok)
-    k = check_for_redefinition (source, line,
-				name, macro_function_like,
-				argc, argv,
-				replacement);
-
-  /* See comments about duplicate keys in macro_define_object.  */
-  if (k && ! key_compare (k, name, source, line))
-    return;
-
-  /* We should also check here that all the argument names in ARGV are
-     distinct.  */
-
-  k = new_macro_key (t, name, source, line);
-  d = new_macro_definition (t, macro_function_like, argc, argv, replacement);
-  splay_tree_insert (t->definitions, (splay_tree_key) k, (splay_tree_value) d);
+  macro_define_internal (source, line,
+			 name, macro_function_like,
+			 argc, argv,
+			 replacement);
 }
 
-
 void
 macro_undef (struct macro_source_file *source, int line,
              const char *name)
@@ -841,8 +850,10 @@ macro_undef (struct macro_source_file *source, int line,
          arguments like '-DFOO -UFOO -DFOO=2'.  */
       if (source == key->start_file
           && line == key->start_line)
-        splay_tree_remove (source->table->definitions, n->key);
-
+	{
+	  splay_tree_remove (source->table->definitions, n->key);
+	  macro_tree_delete_key (key);
+	}
       else
         {
           /* This function is the only place a macro's end-of-scope
-- 
2.20.1

Re: [RFA] Fix leaks in macro definitions.

[Simon Marchi]

On 2019-01-15 00:56, Philippe Waroquiers wrote:
> Valgrind detects leaks like the following (gdb.base/macsp.exp).
> This patch fixes the 3 leaks.
> 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)
> 
> gdb/ChangeLog
> 2019-01-15  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* macrocmd.c (macro_define_command): Use a unique_xmalloc_ptr
> 	to maintain name ownership.
> 	* macrotab.c (macro_define_internal): New function that
> 	factorizes macro_define_object_internal and macro_define_function
> 	code.  If the newly inserted node has replaced a existing node,
> 	call macro_tree_delete_key to free the unused just allocated key.
> 	(macro_define_object_internal): Use macro_define_internal.
> 	(macro_define_function): Likewise.
> 	(macro_undef): Free the key of the removed node.
> ---
>  gdb/macrocmd.c | 10 +++----
>  gdb/macrotab.c | 75 +++++++++++++++++++++++++++++---------------------
>  2 files changed, 48 insertions(+), 37 deletions(-)
> 
> diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c
> index 706a8353e2..c1c42fa910 100644
> --- a/gdb/macrocmd.c
> +++ b/gdb/macrocmd.c
> @@ -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.get () == NULL)

Nit, you don't have to use ".get ()" here.

I think extract_identifier should return a gdb::unique_xmalloc_ptr.  
This call site lower:

     argv[new_macro.argc] = extract_identifier (&exp, 1);

can then use ".release ()".

> @@ -774,8 +780,28 @@ macro_define_object_internal (struct
> macro_source_file *source, int line,
>      return;
> 
>    k = new_macro_key (t, name, source, line);
> -  d = new_macro_definition (t, macro_object_like, kind, 0, 
> replacement);
> -  splay_tree_insert (t->definitions, (splay_tree_key) k, 
> (splay_tree_value) d);
> +  d = new_macro_definition (t, kind, argc, argv, replacement);
> +  s = splay_tree_insert (t->definitions, (splay_tree_key) k,
> (splay_tree_value) d);
> +  if ((struct macro_key *) s->key != k)
> +    {
> +      /* If the node inserted is not using k, it means we have a 
> redefinition.
> +	 We must free the newly allocated k, as it will not be stored in
> +	 the splay tree.  */
> +      macro_tree_delete_key (k);
> +    }

Would you consider that a bug in the splay tree implementation?  Should 
it use the new key and free the old one with the key deallocation 
function?

> @@ -841,8 +850,10 @@ macro_undef (struct macro_source_file *source, int 
> line,
>           arguments like '-DFOO -UFOO -DFOO=2'.  */
>        if (source == key->start_file
>            && line == key->start_line)
> -        splay_tree_remove (source->table->definitions, n->key);
> -
> +	{
> +	  splay_tree_remove (source->table->definitions, n->key);
> +	  macro_tree_delete_key (key);
> +	}
>        else
>          {
>            /* This function is the only place a macro's end-of-scope

Same here, should it be the splay tree code that deletes the key?

Simon

Re: [RFA] Fix leaks in macro definitions.

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Would you consider that a bug in the splay tree implementation?
Simon> Should it use the new key and free the old one with the key
Simon> deallocation function?

Yes, I would think so as well, given that we pass key- and
value-deletion functions to splay_tree_new_with_allocator.

Tom

Re: [RFA] Fix leaks in macro definitions.

[Simon Marchi]

On 2019-01-15 12:15, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> Would you consider that a bug in the splay tree implementation?
> Simon> Should it use the new key and free the old one with the key
> Simon> deallocation function?
> 
> Yes, I would think so as well, given that we pass key- and
> value-deletion functions to splay_tree_new_with_allocator.

Ok.  Fixing this in the splay tree code would be a quite long task 
(reviewing all usages in binutils-gdb and gcc, I don't think this code 
is available externally?), so I am ok with this patch which fixes the 
issue in the mean time.  Tom?

Simon

Re: [RFA] Fix leaks in macro definitions.

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Ok.  Fixing this in the splay tree code would be a quite long task
Simon> (reviewing all usages in binutils-gdb and gcc, I don't think this code
Simon> is available externally?), so I am ok with this patch which fixes the
Simon> issue in the mean time.  Tom?

Actually, I just misunderstood the patch and/or the splay-tree API.

For some of the patch, the splay tree is doing the right thing.
This applies to the macro_define_* patches.


This though:

@@ -841,8 +850,10 @@ macro_undef (struct macro_source_file *source, int line,
          arguments like '-DFOO -UFOO -DFOO=2'.  */
       if (source == key->start_file
           && line == key->start_line)
-        splay_tree_remove (source->table->definitions, n->key);
-
+	{
+	  splay_tree_remove (source->table->definitions, n->key);
+	  macro_tree_delete_key (key);
+	}
       else
         {
           /* This function is the only place a macro's end-of-scope


This one seems like it is definitely a splay-tree bug.  The issue is
that it deletes a node but not the node's key.

I think it would be best by far to fix this in splay_tree_remove.
I agree it's hard, but working around this seems worse to me.

Tom

Re: [RFA] Fix leaks in macro definitions.

[Philippe Waroquiers]

On Tue, 2019-01-15 at 10:15 -0700, Tom Tromey wrote:
> > > > > > "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> Would you consider that a bug in the splay tree implementation?
> Simon> Should it use the new key and free the old one with the key
> Simon> deallocation function?
> 
> Yes, I would think so as well, given that we pass key- and
> value-deletion functions to splay_tree_new_with_allocator.
> 
> Tom

For sure, what splay-tree.h/.c is doing now is underspecified,
and is not very clean/orthogonal for value/key memory mgt.
However, fixing this implies to revisit all the users of the
splay tree.
If this library is only used by gcc and gdb, then probably
the change in libiberty can be coordinated so that gcc and
gdb code is changed in sync.

But if libiberty is used 'in the field' by various other code
basis, fixing the splay tree to internally release the keys
in the cases exposed in the gdb leak (and patch) will cause
(silent) heap corruptions (double free) in all the splay tree
users that are today properly freeing the keys.
Not really a nice backward compatible fix :).

Deciding for a fix in splay tree for sure implies
more discussions (no idea who knows where libiberty is used).

Thanks

Philippe

Re: [RFA] Fix leaks in macro definitions.

[Philippe Waroquiers]

On Tue, 2019-01-15 at 11:50 -0500, Simon Marchi wrote:
> > +  gdb::unique_xmalloc_ptr<char> name (extract_identifier (&exp, 0));
> > +  if (name.get () == NULL)
> 
> Nit, you don't have to use ".get ()" here.
One day, I will really have to learn some C++ :).

> 
> I think extract_identifier should return a gdb::unique_xmalloc_ptr.  
> This call site lower:
> 
>      argv[new_macro.argc] = extract_identifier (&exp, 1);
> 
> can then use ".release ()".
Yes, that looks like a cleaner approach.

I will update the patch once the question about how to
handle the splay tree leak is clarified
i.e. fix in GDB, or (incompatible) fix in libiberty.
(see other mail with Tom).

Thanks

Philippe

Re: [RFA] Fix leaks in macro definitions.

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I think it would be best by far to fix this in splay_tree_remove.
Tom> I agree it's hard, but working around this seems worse to me.

I looked, and there is one spot in gcc that passes a key deletion
function, and one spot in binutils-gdb (the spot in question).

The module in gcc (gcc/lto/lto.c) does not call splay_tree_remove.

So, I think landing the fix upstream in gcc should be totally fine.

Tom

Re: [RFA] Fix leaks in macro definitions.

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> Deciding for a fix in splay tree for sure implies
Philippe> more discussions (no idea who knows where libiberty is used).

This isn't typically a consideration for libiberty.  It's not released
separately from the gcc and binutils-gdb trees, so if someone else out
there is using it, they need to keep watch over what they do when they
start using a new version.

Tom

Re: [RFA] Fix leaks in macro definitions.

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Philippe> Deciding for a fix in splay tree for sure implies
Philippe> more discussions (no idea who knows where libiberty is used).

Tom> This isn't typically a consideration for libiberty.  It's not released
Tom> separately from the gcc and binutils-gdb trees, so if someone else out
Tom> there is using it, they need to keep watch over what they do when they
Tom> start using a new version.

BTW, I can send a patch to gcc if you would prefer.

Tom

Re: [RFA] Fix leaks in macro definitions.

[Philippe Waroquiers]

On Thu, 2019-01-17 at 15:24 -0700, Tom Tromey wrote:
> > > > > > "Tom" == Tom Tromey <tom@tromey.com> writes:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> Deciding for a fix in splay tree for sure implies
> Philippe> more discussions (no idea who knows where libiberty is used).
> 
> Tom> This isn't typically a consideration for libiberty.  It's not released
> Tom> separately from the gcc and binutils-gdb trees, so if someone else out
> Tom> there is using it, they need to keep watch over what they do when they
> Tom> start using a new version.
> 
> BTW, I can send a patch to gcc if you would prefer.
Yes, if the below patch is ok, it would be really nice if you could send
it to gcc:
https://sourceware.org/ml/gdb-patches/2019-01/msg00437.html

(of course, fix/update the patch as you wish/as needed).

Thanks

Philippe

Re: [RFA] Fix leaks in macro definitions.

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Tom > BTW, I can send a patch to gcc if you would prefer.

Philippe> Yes, if the below patch is ok, it would be really nice if you could send
Philippe> it to gcc:
Philippe> https://sourceware.org/ml/gdb-patches/2019-01/msg00437.html
Philippe> (of course, fix/update the patch as you wish/as needed).

I didn't see a patch below; but I had gone ahead & sent the patch
yesterday:

https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01101.html

Tom