[RFAv3,1/2] Fix leak of identifier in macro definition.
Commit Message
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(-)
Comments
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
@@ -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 ());
}