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