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 -- 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
