Re: [PATCH 1/3] fix memory leak

On Wed, 2011-03-30 at 14:39 +0200, Nikola Pajkovsky wrote: > Denys Vlasenko <dvlasenk(a)redhat.com&gt; writes: > > > On Wed, 2011-03-30 at 14:02 +0200, Nikola Pajkovsky wrote: > >> Signed-off-by: Nikola Pajkovsky <npajkovs(a)redhat.com&gt; > >> --- > >> src/cli/report.cpp | 2 ++ > >> 1 files changed, 2 insertions(+), 0 deletions(-) > >> > >> diff --git a/src/cli/report.cpp b/src/cli/report.cpp > >> index 2b964fc..953f2b3 100644 > >> --- a/src/cli/report.cpp > >> +++ b/src/cli/report.cpp > >> @@ -466,6 +466,8 @@ static void ask_for_missing_settings(const char *event_name) > >> event_option_t *opt = get_event_option_from_list(opt_name, > >> event_config->options); > >> > >> + free(opt->value); > >> + > >> char result[512]; > >> > >> char *question = xasprintf("%s: ", (opt->label) ? opt->label: opt->name); > > ...followed by: > > > > switch (opt->type) { > > case OPTION_TYPE_TEXT: > > case OPTION_TYPE_NUMBER: > > read_from_stdin(question, result, 512); > > opt->value = xstrdup(result); > > break; > > case OPTION_TYPE_PASSWORD: > > { > > bool changed = set_echo(false); > > read_from_stdin(question, result, 512); > > if (changed) > > set_echo(true); > > > > opt->value = xstrdup(result); > > /* Newline was not added by pressing Enter because ECHO was > > disabled, so add it now. */ > > puts(""); > > break; > > } > > case OPTION_TYPE_BOOL: > > if (ask_yesno(question)) > > opt->value = xstrdup("yes"); > > else > > opt->value = xstrdup("no"); > > > > break; > > case OPTION_TYPE_INVALID: > > break; > > }; > > > > > > What will happen if we fall through switch() without assigning anything > > to opt->value? > > > > (This might be currently impossible, because opt->type maybe > > always contains one of "allowed" values only, but for the sake > > of writing robust code, let's assume we can't be 100% sure it does). > > > > Answer: opt->value will still point to the free'd memory. Bug. > > I will set to NULL Like this? free(opt->value); opt->value = NULL; This is ok with me.