* [PATCH] Free results of varobj_get_type and type_to_string @ 2015-01-27 22:48 Simon Marchi 2015-01-29 7:38 ` Joel Brobecker 0 siblings, 1 reply; 5+ messages in thread From: Simon Marchi @ 2015-01-27 22:48 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi varobj_get_type and type_to_string return an allocated string, which is not freed at a couple of places. gdb/ChangeLog: * mi/mi-cmd-var.c (mi_cmd_var_info_type): Free result of varobj_get_type. (varobj_update_one): Same. * varobj.c (update_type_if_necessary): Free curr_type_str and new_type_str. (varobj_get_type): Specify in comment that the result needs to be freed by the caller. --- gdb/mi/mi-cmd-var.c | 12 ++++++++++-- gdb/varobj.c | 5 ++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c index 01838b1..4aabb57 100644 --- a/gdb/mi/mi-cmd-var.c +++ b/gdb/mi/mi-cmd-var.c @@ -447,14 +447,18 @@ mi_cmd_var_info_type (char *command, char **argv, int argc) { struct ui_out *uiout = current_uiout; struct varobj *var; + char *type; if (argc != 1) error (_("-var-info-type: Usage: NAME.")); /* Get varobj handle, if a valid var obj name was specified. */ var = varobj_get_handle (argv[0]); + type = varobj_get_type (var); + + ui_out_field_string (uiout, "type", type); - ui_out_field_string (uiout, "type", varobj_get_type (var)); + xfree (type); } void @@ -765,7 +769,11 @@ varobj_update_one (struct varobj *var, enum print_values print_values, } if (r->type_changed) - ui_out_field_string (uiout, "new_type", varobj_get_type (r->varobj)); + { + char *type = varobj_get_type (r->varobj); + ui_out_field_string (uiout, "new_type", type); + xfree (type); + } if (r->type_changed || r->children_changed) ui_out_field_int (uiout, "new_num_children", diff --git a/gdb/varobj.c b/gdb/varobj.c index a10560f..9735958 100644 --- a/gdb/varobj.c +++ b/gdb/varobj.c @@ -972,7 +972,8 @@ varobj_add_child (struct varobj *var, struct varobj_item *item) } /* Obtain the type of an object Variable as a string similar to the one gdb - prints on the console. */ + prints on the console. The caller is responsible for freeing the string. + */ char * varobj_get_type (struct varobj *var) @@ -1303,6 +1304,8 @@ update_type_if_necessary (struct varobj *var, struct value *new_value) var->num_children = -1; return 1; } + xfree (curr_type_str); + xfree (new_type_str); } } -- 1.9.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Free results of varobj_get_type and type_to_string 2015-01-27 22:48 [PATCH] Free results of varobj_get_type and type_to_string Simon Marchi @ 2015-01-29 7:38 ` Joel Brobecker 2015-01-29 16:28 ` Simon Marchi 0 siblings, 1 reply; 5+ messages in thread From: Joel Brobecker @ 2015-01-29 7:38 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches Hi Simon, On Tue, Jan 27, 2015 at 01:33:38PM -0500, Simon Marchi wrote: > varobj_get_type and type_to_string return an allocated string, which is > not freed at a couple of places. > > gdb/ChangeLog: > > * mi/mi-cmd-var.c (mi_cmd_var_info_type): Free result of > varobj_get_type. > (varobj_update_one): Same. > * varobj.c (update_type_if_necessary): Free curr_type_str and > new_type_str. > (varobj_get_type): Specify in comment that the result needs to be > freed by the caller. Thanks looking into this. Comments below. > struct ui_out *uiout = current_uiout; > struct varobj *var; > + char *type; Would you mind renaming this variable "type_name" instead of type. I don't know about the others, but "type" is now wired into my brain as to be a "struct type *"... > @@ -765,7 +769,11 @@ varobj_update_one (struct varobj *var, enum print_values print_values, > } > > if (r->type_changed) > - ui_out_field_string (uiout, "new_type", varobj_get_type (r->varobj)); > + { > + char *type = varobj_get_type (r->varobj); > + ui_out_field_string (uiout, "new_type", type); > + xfree (type); > + } Same here, please. Also, can you add an empty line after the local declarations? This is part of GDB's Coding Style. > /* Obtain the type of an object Variable as a string similar to the one gdb > - prints on the console. */ > + prints on the console. The caller is responsible for freeing the string. > + */ Thanks for updating functions' documentation. I really appreciate that. > char * > varobj_get_type (struct varobj *var) > @@ -1303,6 +1304,8 @@ update_type_if_necessary (struct varobj *var, struct value *new_value) > var->num_children = -1; > return 1; > } > + xfree (curr_type_str); > + xfree (new_type_str); In this case, you're still missing the case where the function returns, I believe. One way to handle the situation, I think in a way that makes the allocation + deallocation localized would be to introduce a variable containing the result of the strcmp? For instance: new_type_str = type_to_string (new_type); curr_type_str = varobj_get_type (var); type_name_has_changed = strcmp (curr_type_str, new_type_str) != 0; xfree (new_type_str); xfree (curr_type_str); if (type_name_has_changed) { -- Joel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Free results of varobj_get_type and type_to_string 2015-01-29 7:38 ` Joel Brobecker @ 2015-01-29 16:28 ` Simon Marchi 2015-01-30 19:46 ` Joel Brobecker 0 siblings, 1 reply; 5+ messages in thread From: Simon Marchi @ 2015-01-29 16:28 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches On 15-01-29 12:11 AM, Joel Brobecker wrote: > Hi Simon, > > On Tue, Jan 27, 2015 at 01:33:38PM -0500, Simon Marchi wrote: >> varobj_get_type and type_to_string return an allocated string, which is >> not freed at a couple of places. >> >> gdb/ChangeLog: >> >> * mi/mi-cmd-var.c (mi_cmd_var_info_type): Free result of >> varobj_get_type. >> (varobj_update_one): Same. >> * varobj.c (update_type_if_necessary): Free curr_type_str and >> new_type_str. >> (varobj_get_type): Specify in comment that the result needs to be >> freed by the caller. > > Thanks looking into this. Comments below. > >> struct ui_out *uiout = current_uiout; >> struct varobj *var; >> + char *type; > > Would you mind renaming this variable "type_name" instead of type. > I don't know about the others, but "type" is now wired into my brain > as to be a "struct type *"... Done. >> @@ -765,7 +769,11 @@ varobj_update_one (struct varobj *var, enum print_values print_values, >> } >> >> if (r->type_changed) >> - ui_out_field_string (uiout, "new_type", varobj_get_type (r->varobj)); >> + { >> + char *type = varobj_get_type (r->varobj); >> + ui_out_field_string (uiout, "new_type", type); >> + xfree (type); >> + } > > Same here, please. > > Also, can you add an empty line after the local declarations? > This is part of GDB's Coding Style. Done and done. >> /* Obtain the type of an object Variable as a string similar to the one gdb >> - prints on the console. */ >> + prints on the console. The caller is responsible for freeing the string. >> + */ > > Thanks for updating functions' documentation. I really appreciate that. > >> char * >> varobj_get_type (struct varobj *var) >> @@ -1303,6 +1304,8 @@ update_type_if_necessary (struct varobj *var, struct value *new_value) >> var->num_children = -1; >> return 1; >> } >> + xfree (curr_type_str); >> + xfree (new_type_str); > > In this case, you're still missing the case where the function returns, > I believe. > > One way to handle the situation, I think in a way that makes the > allocation + deallocation localized would be to introduce a variable > containing the result of the strcmp? For instance: > > new_type_str = type_to_string (new_type); > curr_type_str = varobj_get_type (var); > type_name_has_changed = strcmp (curr_type_str, new_type_str) != 0; > xfree (new_type_str); > xfree (curr_type_str); > > if (type_name_has_changed) > { Done. Thanks, here's v2. From a1f4ce1e15a1ce7806c331a2600b57353331807b Mon Sep 17 00:00:00 2001 From: Simon Marchi <simon.marchi@ericsson.com> Date: Tue, 27 Jan 2015 11:43:35 -0500 Subject: [PATCH] Free results of varobj_get_type and type_to_string varobj_get_type and type_to_string return an allocated string, which is not freed at a couple of places. New in v2: * Rename char * type to type_name. * Free in all cases in update_type_if_necessary. gdb/ChangeLog: * mi/mi-cmd-var.c (mi_cmd_var_info_type): Free result of varobj_get_type. (varobj_update_one): Same. * varobj.c (update_type_if_necessary): Free curr_type_str and new_type_str. (varobj_get_type): Specify in comment that the result needs to be freed by the caller. --- gdb/mi/mi-cmd-var.c | 13 +++++++++++-- gdb/varobj.c | 10 ++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c index 01838b1..d873a17 100644 --- a/gdb/mi/mi-cmd-var.c +++ b/gdb/mi/mi-cmd-var.c @@ -447,14 +447,18 @@ mi_cmd_var_info_type (char *command, char **argv, int argc) { struct ui_out *uiout = current_uiout; struct varobj *var; + char *type_name; if (argc != 1) error (_("-var-info-type: Usage: NAME.")); /* Get varobj handle, if a valid var obj name was specified. */ var = varobj_get_handle (argv[0]); + type_name = varobj_get_type (var); - ui_out_field_string (uiout, "type", varobj_get_type (var)); + ui_out_field_string (uiout, "type", type_name); + + xfree (type_name); } void @@ -765,7 +769,12 @@ varobj_update_one (struct varobj *var, enum print_values print_values, } if (r->type_changed) - ui_out_field_string (uiout, "new_type", varobj_get_type (r->varobj)); + { + char *type_name = varobj_get_type (r->varobj); + + ui_out_field_string (uiout, "new_type", type_name); + xfree (type_name); + } if (r->type_changed || r->children_changed) ui_out_field_int (uiout, "new_num_children", diff --git a/gdb/varobj.c b/gdb/varobj.c index a10560f..dad284d 100644 --- a/gdb/varobj.c +++ b/gdb/varobj.c @@ -972,7 +972,8 @@ varobj_add_child (struct varobj *var, struct varobj_item *item) } /* Obtain the type of an object Variable as a string similar to the one gdb - prints on the console. */ + prints on the console. The caller is responsible for freeing the string. + */ char * varobj_get_type (struct varobj *var) @@ -1289,11 +1290,16 @@ update_type_if_necessary (struct varobj *var, struct value *new_value) { struct type *new_type; char *curr_type_str, *new_type_str; + int type_name_changed; new_type = value_actual_type (new_value, 0, 0); new_type_str = type_to_string (new_type); curr_type_str = varobj_get_type (var); - if (strcmp (curr_type_str, new_type_str) != 0) + type_name_changed = strcmp (curr_type_str, new_type_str) != 0; + xfree (curr_type_str); + xfree (new_type_str); + + if (type_name_changed) { var->type = new_type; -- 1.9.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Free results of varobj_get_type and type_to_string 2015-01-29 16:28 ` Simon Marchi @ 2015-01-30 19:46 ` Joel Brobecker 2015-01-30 20:17 ` Simon Marchi 0 siblings, 1 reply; 5+ messages in thread From: Joel Brobecker @ 2015-01-30 19:46 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches > >> gdb/ChangeLog: > >> > >> * mi/mi-cmd-var.c (mi_cmd_var_info_type): Free result of > >> varobj_get_type. > >> (varobj_update_one): Same. > >> * varobj.c (update_type_if_necessary): Free curr_type_str and > >> new_type_str. > >> (varobj_get_type): Specify in comment that the result needs to be > >> freed by the caller. Thank you. This looks good to me, so go ahead and push! -- Joel ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Free results of varobj_get_type and type_to_string 2015-01-30 19:46 ` Joel Brobecker @ 2015-01-30 20:17 ` Simon Marchi 0 siblings, 0 replies; 5+ messages in thread From: Simon Marchi @ 2015-01-30 20:17 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches On 15-01-30 04:03 AM, Joel Brobecker wrote: >>>> gdb/ChangeLog: >>>> >>>> * mi/mi-cmd-var.c (mi_cmd_var_info_type): Free result of >>>> varobj_get_type. >>>> (varobj_update_one): Same. >>>> * varobj.c (update_type_if_necessary): Free curr_type_str and >>>> new_type_str. >>>> (varobj_get_type): Specify in comment that the result needs to be >>>> freed by the caller. > > Thank you. This looks good to me, so go ahead and push! Pushed, thanks! ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-01-30 18:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-27 22:48 [PATCH] Free results of varobj_get_type and type_to_string Simon Marchi 2015-01-29 7:38 ` Joel Brobecker 2015-01-29 16:28 ` Simon Marchi 2015-01-30 19:46 ` Joel Brobecker 2015-01-30 20:17 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).