Bug 216034

In fact, the backtrace is missing the dbus_g_proxy_manager_replace_name_owner
frame it looks like.

The crash is triggered by this block of code:

   │707       else                                                             │
   │708         {                                                              │
   │709           DBusGProxyNameOwnerInfo *info;                               │
   │710           GSList *link;                                                │
   │711                                                                        │
   │712           /* Name owner changed or deleted */                          │
   │713                                                                        │
   │714           names = g_hash_table_lookup (manager->owner_names, prev_owner│
   │715                                                                        │
  >│716           link = g_slist_find_custom (names, name, find_name_in_info); │
   │717                                                                        │
where name is "org.gnome.YelpService" and names looks bogus

(gdb) p *(struct _GSList *) 0x8ed8f48
$18 = {data = 0x0, next = 0x0}

Also, note this function (dbus_g_proxy_manager_replace_name_owner) has another
code path that could conceivably add an element to the names list with null data:

   │716           link = g_slist_find_custom (names, name, find_name_in_info); │
   │717                                                                        │
   │718           info = NULL;                                                 │
   │719           if (link != NULL)                                            │
   │720             {                                                          │
                  (fill in info here ...)
   │727             }                                                          |
   │728                                                                        │
   │729           if (new_owner[0] == '\0')                                    │
   │730             {                                                          │
                   (...)
   │748             }                                                          │
   │749           else                                                         │
   │750             {                                                          │
   │751               insert_nameinfo (manager, new_owner, info);              │
   │752             }  

and insert_nameinfo does
   │563       names = g_slist_append (names, info);                            │

I have no idea if it hit this code path earlier or not, I just noticed it when
snooping around.

*** Bug 215952 has been marked as a duplicate of this bug. ***

This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux major release.  Product Management has requested further
review of this request by Red Hat Engineering, for potential inclusion in a Red
Hat Enterprise Linux Major release.  This request is not yet committed for
inclusion.

It also looks to me like info might be leaked in that function you cite:


Here we remove info from the owner_names table:


      if (link != NULL)
        {
          info = link->data;

          names = g_slist_delete_link (names, link);

          if (names == NULL)
            g_hash_table_remove (manager->owner_names, prev_owner);
        }


And here we do nothing with it, assuming new_owner is not empty:

      if (new_owner[0] == '\0')
        {
          DBusGProxyUnassociateData data;
          GSList *tmp;

          data.name = name;
          data.destroyed = NULL;

          /* A service went away, we need to unassociate proxies */
          g_hash_table_foreach (manager->proxy_lists,
                                unassociate_proxies, &data);

          UNLOCK_MANAGER (manager);

          for (tmp = data.destroyed; tmp; tmp = tmp->next)
            dbus_g_proxy_destroy (tmp->data);
          g_slist_free (data.destroyed);

          LOCK_MANAGER (manager);
        }
 
I think info may need freeing in that case ?

Created attachment 142259 [details]
possible patch

Here is an untested patch that adresses both issues. 
Does that look reasonable, John ?

John says the patch looks good, and there very similar patches in upstream bugzilla.

Thanks Matthias, I've included this patch

 * Tue Nov 28 2006 David Zeuthen <davidz> - 0.70-5
 - Add dbus-glib-0.70-fix-info-leak.patch
 - Resolves: #216034

Package is partially built in Brew (waiting on s390x).

Ray, can you verify that this package works? Thanks.

I don't have a reliable way to reproduce the problem and it's only happened to
me a few times, unfortunately.

I wonder if we could write a test case that just takes control of a bus name and
releases it over and over again.

You don't want to take control and release it, instead you want to
go from one owner to another owner. Calling :

 mugshot --replace

a few times (it won't quit the old one with mugshot < 1.1.27, but it
still replaces the D-Bus name) should take down your session pretty reliably.

According to a conversation that Havoc and I had, the patch in the
upstream bug report is most likely more correct than the one here ...
the proxy code in D-Bus should simply not care changes in the
name owner for names it doesn't have a proxy for, so inserting a 
dummy info entry doesn't make sense. (We didn't review it line by
line.)

A package has been built which should help the problem described in 
this bug report. This report is therefore being closed with a resolution 
of CURRENTRELEASE. You may reopen this bug report if the solution does 
not work for you.