Hi Szymon, > -----Original Message----- > From: Szymon Janc [mailto:szymon.janc@xxxxxxxxx] > Sent: Wednesday, October 01, 2014 6:52 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 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 I agree with you. Then I will sent the updated patch to make it more readable as suggested by you in the above discussion with some coding style fixes. Regards, Gowtham -- 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
