[patch] null after free

[John (J5) Palmieri]

On Mon, 2004-10-04 at 10:27 -0400, John (J5) Palmieri wrote:
> On Sat, 2004-10-02 at 18:59 -0400, Havoc Pennington wrote:
> > On Fri, 2004-10-01 at 16:42 -0400, John (J5) Palmieri wrote:
> > > An unfortunate breakage in glibc's getgrouplist function led me to this
> > > bug in a rarely used error code path.  Basically on errors in the
> > > fill_user_info function there is a double free of the info structure.  I
> > > have a quick fix which nulls out the structure after the free.  This
> > > ensures that double frees don't crash dbus.  This however is a bandaid
> > > and we need to look at why the double free is happening and to determine
> > > which of the frees is the correct one. 
> > > 
> > > The glibc guys took care of the glib bug so no worries there.
> > > 
> > 
> > OK, we should really figure out the real bug instead of applying the
> > bandaid - valgrind should spell it out for you, I would think, if you
> > can reproduce.
> > 
> > Havoc
> 
> I know where it is happening.  I just wasn't sure if it was happening in
> other places and didn't have time to fix it properly.  I thought I would
> get the bandaid out first and report the error while I was tracking down
> the glibc bug.  What I will do, now that I have time, is put asserts in
> the user_info_free function and fix the double frees properly where I
> can find them.  

What is actually happening is that on an error in the fill_user_info
function in dbus-sysdeps.c we are jumping to the failed section which
frees the DBusUserInfo structure:

failed:
  _DBUS_ASSERT_ERROR_IS_SET (error);
  _dbus_user_info_free (info);
  return FALSE;

However this returns FALSE and an error to the caller which then calls
free_user_info (which calls _dbus_user_info_free somewhere down the
line).  Here is an example from dbus-userdb.c
(_dbus_user_database_lookup):

if (!_dbus_user_info_fill_uid (info, uid, error))
  {
    _DBUS_ASSERT_ERROR_IS_SET (error);
    free_user_info (info);
    return NULL;
  }

This is just one example but it happens a couple of times.  The question
is where is it best to free the structure?  The DBusUserInfo field is
passed into fill_user_info so I feel this is not the place it should be
freed.  However if it is not freed there we must make sure the error is
handled everywhere else.  Any opinions?

-- 
John (J5) Palmieri
Associate Software Engineer
Desktop Group
Red Hat, Inc.
Blog: http://martianrock.com