Re: [PATCH] obexd/src/main: Fix memory leak on obex server

[Szymon Janc <szymon.janc@tieto.com>]

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@tieto.com]
> > Sent: Wednesday, October 01, 2014 12:28 PM
> > To: Gowtham Anandha Babu
> > Cc: linux-bluetooth@vger.kernel.org; d.kasatkin@samsung.com;
> > bharat.panda@samsung.com; cpgs@samsung.com
> > 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