[Bug 47429] Support SalutContact and SautContact-manager with Bonjour backend

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=47429

--- Comment #11 from Olli Salli <olli.salli at collabora.co.uk> 2012-03-25 07:47:41 PDT ---
(In reply to comment #7)
> (In reply to comment #6)
> > > > + else if (!salut_bonjour_contact_has_services (SALUT_BONJOUR_CONTACT
> > > > +     (contact)))
> > > > +   {
> > > > +     g_object_ref (contact);
> > > > +   }
> > > 
> > > but later on:
> > > 
> > > + if (salut_bonjour_contact_has_services (SALUT_BONJOUR_CONTACT (contact)))
> > > +   {
> > > +     /* We reffed this contact as it has services */
> > > +     g_object_unref (contact);
> > > +   }
> > > 
> > > So this reffing with services confuses me?
> 
> me too, I just followed whats in avahi-contact-manager so it follows a similar
> logic. how do you propose I change this ?

I investigated this. It's indeed the same logic as in the Avahi backend.
(Arguably a better abstraction would have this multiple service management in
teh base class).

The contact manager holds *one* reference to all contacts which have at least
one service. So on the first service add for a contact, it adds a ref. (Contact
objects can exist without having been discovered from the network e.g. because
they've been added to a channel's members, see e.g.
salut_muc_channel_add_members).

The latter code excerpt with the g_object_unref is in the dispose_contact
virtual method. That is only called by close_all, when disconnecting. Then we
won't receive any service remove events so to not leave the contact dangling,
if we previously added a ref due to a service add, we remove it. So that much
actually makes sense when one ponders it hard with their friend grep(1).

But there is a problem in the Bonjour code in this area: 

+      DEBUG ("Contact Removed : %s", name);
+      contact = g_hash_table_lookup (mgr->contacts, name);
+
+      if (contact != NULL)
+        {
+          salut_bonjour_contact_remove_service (SALUT_BONJOUR_CONTACT
(contact),
+              interfaceIndex, name, regtype, domain);
+          g_object_unref (contact);
+        }

When a single service is removed, it unconditionally removes the manager's ref
to the contact. Even if it wasn't the last service on that contact. If you have
multiple services, by the time the second of them goes away, it'll be dropping
references it never had.

This should be like the corresponding Avahi code instead:

  if (contact != NULL)
    {
      salut_avahi_contact_remove_service (SALUT_AVAHI_CONTACT (contact),
          interface, protocol, name, type, domain);
      if (!salut_avahi_contact_has_services (SALUT_AVAHI_CONTACT (contact)))
         g_object_unref (contact);

    }

The alternative would be to ref each time we discover a service for a contact,
but then the manager would have to ask the contact via some API how many times
it has to unref it in dispose_contact() - or record this in some data structure
of its own. Both messy. So just make the service remove code path in
service_browse_cb() consider whether it was the last service, and if so, drop
our current *single* ref to the contact.

So much about that. Review for rest of the current code:

Aside from that multiple unref trap, SalutBonjourContactManager looks OK to me.

Service ref management
======================

You're storing pointers to DNSServiceRefs in your mappings, and later calling
stuff like DNSServiceRefDeallocate by dereferencing the pointer. The
DNSServiceRefs are stored in your other modules' private structures, like in
BonjourSelf and the BonjourContact resolvers. This means that if watched
service refs were left over, you'll end up dereferencing dangling pointers, as
the discovery client is destroyed later than some of the modules (they all hold
a ref to the disco client, so at most one of them can be destroyed later than
it).

DNSServiceRef by itself is a pointer type. So we can copy and store it safely
(if we couldn't, we couldn't dereference a pointer to one to pass it to dns-sd
API functions). Just change watch_svc_ref and drop_svc_ref to take a
DNSServiceRef instead of a pointer to one, and store DNSServiceRefs, not
pointers to them, in the tables. Then this ServiceRef management stuff doesn't
have to read the actual ref pointers from private structs of your other
modules, and all this becomes much less prone to obscure errors of the like
where you've left a watch over and are now calling dns-sd functions on
something completely different through a dangling pointer.

In drop_svc_ref:

+  if (!priv->svc_ref_table)
+    return;
+
+  if (!priv->svc_source_table)
+    return;
+

With the changes you did according to Jonny's review, these never are NULL so
this is totally redundant.

BonjourContact
==============

+  const char *txt_record = ctx->txt_record;
+  tmp = (char *) TXTRecordGetValuePtr (txt_length, txt_record,

This is OK. You pass the pointer to the TXT record bytes to
TXTRecordGetValuePtr, as you should be doing.

+  tmp = (char *) TXTRecordGetValuePtr (txt_length, &txt_record,

This OTOH is completely wrong. You're passing a pointer to a pointer (and
random stuff following it) in your stack, to be interpreted as TXT record
bytes. The TXT record should be interpreted exactly in the same fashion for ALL
attributes as for the caps ones.

+gboolean
+salut_bonjour_contact_add_service (SalutBonjourContact *self,
+                                   uint32_t interface,
+                                   const char *name,
+                                   const char *type,
+                                   const char *domain)
+{

...

+  ctx->name = name;
+  ctx->type = type;
+  ctx->domain = domain;

You're storing pointers to strings Bonjour passed to you, in the long lived
context structure. They will point to something totally different in a minute,
so the resolver comparisons will never find the resolvers correctly. g_strdup
them and g_free them when the context is destroyed.

(This might be why you omitted the if () for there still being services for
unrefing a contact in the contact manager - because there always happened to be
services, as the resolver to be removed couldn't be found?)

This code has these other critical problems with the cleanup actually:

1) If the service is resolved again while it has an ongoing GetAddrInfo (that's
a slow network operation so it's very much possible), you'll leak the txt
record and address_ref (because the pointers to them are overwritten with new
stuff)

Fix: when you free the txt record or drop address ref, set the pointers to them
to NULL. Verify both are initialized to that as well. Then you can determine
that a GetAddrInfo is currently running in resolve_cb, by them being non-NULL.
In that case, cancel the old operation by dropping the svc ref watch and
freeing the txt record contents, and only then start another GetAddrInfo
which'll lead to parsing the txt record.

2) If remove_service() is called while the corresponding resolver has a
GetAddrInfo running, it will continue running... and eventually crash when it
gets a result, because the ctx has already been freed.

Fix: Again determine from address_ref and txt_record being non-NULL if a
GetAddrInfo is running, and in that case cancel it as well while destroying the
resolver.

Now that the resolver context struct gains more to clean up, please add a
salut_bonjour_resolve_ctx_free which you use everywhere to deallocate the
ctx's, instead of trying to remember to free all heap members separately in
those places.

Please test the same cases again after these changes to verify you've
implemented them correctly.

ps. Now that you're just doing smaller fixes and not full revamps of the code,
please don't amend and amend your patches again - I want to see the fixes, and
if they exist, from new commits, not re-review the full code repeatedly.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.