Hi Gowtham, On Wednesday 01 of October 2014 16:24:27 Gowtham Anandha Babu wrote: > Hi Szymon, > > > -----Original Message----- > > From: Szymon Janc [mailto:szymon.janc@xxxxxxxxx] > > Sent: Wednesday, October 01, 2014 2:31 PM > > To: Gowtham Anandha Babu > > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; 'Dmitry Kasatkin'; 'Bharat Panda' > > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > > > Hi Gowtham, > > > > On Wednesday 01 of October 2014 14:21:22 Gowtham Anandha Babu wrote: > > > Hi Szymon, > > > > > > > -----Original Message----- > > > > From: Szymon Janc [mailto:szymon.janc@xxxxxxxxx] > > > > Sent: Wednesday, October 01, 2014 12:28 PM > > > > To: Gowtham Anandha Babu > > > > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; d.kasatkin@xxxxxxxxxxx; > > > > bharat.panda@xxxxxxxxxxx; cpgs@xxxxxxxxxxx > > > > Subject: Re: [PATCH] obexd/src/main: Fix memory leak on obex server > > > > > > > > Hi Gowtham, > > > > > > > > On Wednesday 01 of October 2014 12:01:43 Gowtham Anandha Babu > > wrote: > > > > > Freeing the variables at appropriate place. > > > > > --- > > > > > obexd/src/main.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/obexd/src/main.c b/obexd/src/main.c index > > > > > 80645f8..7a1ab98 100644 > > > > > --- a/obexd/src/main.c > > > > > +++ b/obexd/src/main.c > > > > > @@ -293,8 +293,9 @@ int main(int argc, char *argv[]) > > > > > char *old_root = option_root, *home = getenv("HOME"); > > > > > if (home) { > > > > > option_root = g_strdup_printf("%s/%s", home, > > > > old_root); > > > > > - g_free(old_root); > > > > > + g_free(home); > > > > > } > > > > > + g_free(old_root); > > > > > } > > > > > > > > > > if (option_capability == NULL) > > > > > > > > > > > > > This patch is incorrect in multiple ways: > > > > - freeing old_root without altering option_root would result in use-after- > > free > > > > few lines later in root_folder_setup() or (if program didn't crash already) > > > > double free on exit. > > > > - freeing home pointer would result in crash or undefined behavior: > > > > from getenv manual: "As typically implemented, getenv() returns a > > > > pointer to > > > > a string within the environment list. The caller must take care not to > > > > modify this string, since that would change the environment of the > > process. > > > > ...The string pointed to by the return value of getenv() may be statically > > > > allocated," > > > > > > > > But I agree that original code is a bit confusing. Maybe something > > > > like this would make it more readable? > > > > > > > > --- a/obexd/src/main.c > > > > +++ b/obexd/src/main.c > > > > @@ -290,8 +290,9 @@ int main(int argc, char *argv[]) > > > > } > > > > > > > > if (option_root[0] != '/') { > > > > - char *old_root = option_root, *home = getenv("HOME"); > > > > + const char *home = getenv("HOME"); > > > > if (home) { > > > > + char *old_root = option_root; > > > > option_root = g_strdup_printf("%s/%s", home, old_root); > > > > g_free(old_root); > > > > } > > > > > > > > > > > > -- > > > > Best regards, > > > > Szymon Janc > > > > > > I agree with you. > > > But why can't it be like this > > > > > > if (option_root[0] != '/') { > > > const char *home = getenv("HOME"); > > > if (home) { > > > option_root = g_strdup_printf("%s/%s", home, > > option_root); > > > } > > > } > > > --- > > > (checked, it's not crashing) > > > > > > In that case you are leaking option_root. That is why temporary old_root is > > needed so that original option_root can be freed. > > > > -- > > Best regards, > > Szymon Janc > > Sorry If I am wrong, at the end option_root was freed after g_mail_loop_unref(main_loop). Is that enough ? > Or Do we need to free that intermediate option_root inside g_strdup_printf() ? Memory pointed by old option_root needs to be freed since pointer is overwritten. I suggest exploring valgrind for tracking memory leaks (see HACKING for guideline). > One more clarification: > > static gboolean parse_debug(const char *key, const char *value, > gpointer user_data, GError **error) > { > if (value) > option_debug = g_strdup(value); > else > option_debug = g_strdup("*"); > > return TRUE; > } > > > In the above function, option_debug needs be freed right? As Like below: > > static gboolean parse_debug(const char *key, const char *value, > gpointer user_data, GError **error) > { > g_free(option_debug); > if (value) > option_debug = g_strdup(value); > else > option_debug = g_strdup("*"); > > return TRUE; > } There is no need to free that since option_debug is initially NULL. -- Best regards, Szymon Janc -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
