[RFA] Fix leak of set/show verbose doc, avoid xfree of static string
Commit Message
In the tests
py-pp-registration/gdb.log
default/gdb.log
foll-fork/gdb.log
setshow/gdb.log
break-interp/gdb.log
Valgrind detects a leak of the doc strings for the set and show verbose cmd.
Here is the stacktrace of the leaked set doc:
==25548== 15 bytes in 1 blocks are definitely lost in loss record 101 of 3,120
==25548== at 0x4C2BE6D: malloc (vg_replace_malloc.c:309)
==25548== by 0x409C27: xmalloc (common-utils.c:44)
==25548== by 0x778AF9: xstrdup (xstrdup.c:34)
==25548== by 0x3F860F: add_setshow_cmd_full(char const*, command_class, var_types, void*, char const*, char const*, char const*, void (*)(char const*, int, cmd_list_element*), void (*)(ui_file*, int, cmd_list_element*, char const*), cmd_list_element**, cmd_list_element**, cmd_list_element**, cmd_list_element**) [clone .constprop.10] (cli-decode.c:495)
==25548== by 0x3F8ADB: add_setshow_boolean_cmd(char const*, command_class, int*, char const*, char const*, char const*, void (*)(char const*, int, cmd_list_element*), void (*)(ui_file*, int, cmd_list_element*, char const*), cmd_list_element**, cmd_list_element**) (cli-decode.c:593)
==25548== by 0x3F7442: _initialize_cli_cmds() (cli-cmds.c:1768)
==25548== by 0x69EED3: initialize_all_files() (init.c:365)
==25548== by 0x658A84: gdb_init(char*) (top.c:2163)
==25548== by 0x5403E1: captured_main_1 (main.c:863)
==25548== by 0x5403E1: captured_main (main.c:1167)
==25548== by 0x5403E1: gdb_main(captured_main_args*) (main.c:1193)
==25548== by 0x289CA7: main (gdb.c:32)
The leak is created by top.c set_verbose 'elaborate joke':
the doc string is changed according to the verbosity:
(gdb) help set verbose
Set verbosity.
(gdb) set verbose on
(gdb) help set verbose
Set verbose printing of informational messages.
(gdb)
set_verbose creates the leak as it replaces the string allocated in
the above stacktrace by a static (non translated) string:
...
if (info_verbose)
{
c->doc = "Set verbose printing of informational messages.";
...
Also, this can possibly trigger a call to 'free' of a static string,
as c->doc_allocated is kept true, while the string is not allocated anymore.
This patch:
* fixes the leak by freeing the previous docs if doc_allocated.
* internationalize the messages.
* properly sets doc_allocated to 0 once doc strings are static.
gdb/ChangeLog
2018-12-28 Philippe Waroquiers <philippe.waroquiers@skynet.be>
* top.c (set_verbose): Free previous docs if doc_allocated.
Internationalize messages. Set doc_allocated to 0.
---
gdb/top.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
Comments
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
Philippe> gdb/ChangeLog
Philippe> 2018-12-28 Philippe Waroquiers <philippe.waroquiers@skynet.be>
Philippe> * top.c (set_verbose): Free previous docs if doc_allocated.
Philippe> Internationalize messages. Set doc_allocated to 0.
Thanks for the patch. This is ok.
Philippe> + if (c->doc && c->doc_allocated)
Philippe> + xfree ((char *) c->doc);
Philippe> + if (showcmd->doc && showcmd->doc_allocated)
Philippe> + xfree ((char *) showcmd->doc);
Ideally I guess we'd have a better way of doing this. A lot of the CLI
machinery could use some cleanup, but for now this is still an "open"
struct.
Tom
On Fri, 2018-12-28 at 09:45 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
>
> Philippe> gdb/ChangeLog
> Philippe> 2018-12-28 Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> Philippe> * top.c (set_verbose): Free previous docs if doc_allocated.
> Philippe> Internationalize messages. Set doc_allocated to 0.
>
> Thanks for the patch. This is ok.
Thanks for the review, pushed.
>
> Philippe> + if (c->doc && c->doc_allocated)
> Philippe> + xfree ((char *) c->doc);
> Philippe> + if (showcmd->doc && showcmd->doc_allocated)
> Philippe> + xfree ((char *) showcmd->doc);
>
> Ideally I guess we'd have a better way of doing this. A lot of the CLI
> machinery could use some cleanup, but for now this is still an "open"
> struct.
Effectively, as it is now, cmd_list_element memory management
is not very well encapsulated.
Philippe
@@ -1820,7 +1820,7 @@ show_history (const char *args, int from_tty)
int info_verbose = 0; /* Default verbose msgs off. */
-/* Called by do_setshow_command. An elaborate joke. */
+/* Called by do_set_command. An elaborate joke. */
void
set_verbose (const char *args, int from_tty, struct cmd_list_element *c)
{
@@ -1830,16 +1830,22 @@ set_verbose (const char *args, int from_tty, struct cmd_list_element *c)
showcmd = lookup_cmd_1 (&cmdname, showlist, NULL, 1);
gdb_assert (showcmd != NULL && showcmd != CMD_LIST_AMBIGUOUS);
+ if (c->doc && c->doc_allocated)
+ xfree ((char *) c->doc);
+ if (showcmd->doc && showcmd->doc_allocated)
+ xfree ((char *) showcmd->doc);
if (info_verbose)
{
- c->doc = "Set verbose printing of informational messages.";
- showcmd->doc = "Show verbose printing of informational messages.";
+ c->doc = _("Set verbose printing of informational messages.");
+ showcmd->doc = _("Show verbose printing of informational messages.");
}
else
{
- c->doc = "Set verbosity.";
- showcmd->doc = "Show verbosity.";
+ c->doc = _("Set verbosity.");
+ showcmd->doc = _("Show verbosity.");
}
+ c->doc_allocated = 0;
+ showcmd->doc_allocated = 0;
}
/* Init the history buffer. Note that we are called after the init file(s)