Merge lp:~mhr3/unity/fix-861144 into lp:unity

[Michal Hruby]

The changes in this branch try to make the panel-service more crash-proof by disallowing casts of random pointers to IndicatorObjectEntry instances, as well as installing a weak notifier on the Indicators (in case they are unreferenced after instantiation).

Please note that this should go also into unity/4.0 branch.

[Mikkel Kamstrup Erlandsen (kamstrup)]

+ {
+ /* check that there really is such IndicatorObjectEntry */
+ if (g_hash_table_lookup (priv->entry2indicator_hash, entry) != NULL)
+ {
+ return entry;
+ }

Can you add an extra check in there:

+ if (!INDICATOR_IS_OBJECT_ENTRY (entry))
+ {
+ g_critical ("Invalid IndicatorObjectEntry reference %p", entry);
+ return NULL;
+ }

Not that it makes the code much more safe, but it has a slim chance of
making debugging easier should we ever see it again

[Michal Hruby (mhr3)]

I'd love to add that, but IndicatorObjectEntry is just a simple struct, sorry...

[Marco Trevisan (Treviño) (3v1n0)]

get_entry_by_id has been previously added to be very fast and to avoid unneeded loops or checks.
If well used it shouldn't lead to crashes, was it causing some?
However the new lookup improves the safety.

[Mikkel Kamstrup Erlandsen (kamstrup)]

Marco - at the very least it was taking a string directly from DBus
and parsing it into a memory address and accessing that address
without any sanity checks. That is *highly* unsafe and can lead to
arbitrary impossible-to-debug bugs at any later point in time. Granted
- after this patch it is still highly unsafe, if slightly less than
before :-)

For P I think we should find a way to stop passing pointers over DBus.
It is just wrong in so many ways.

[Marco Trevisan (Treviño) (3v1n0)]

On the first version the pointer was just used as an ID that was used by an hashtable, but using this way caused some slowdown. However I know that the current implementation is potentially unsafe, but since we're mostly getting events from that entries, the risks are controlled. Also I never experienced a crash caused by that.

However I agree that this part would need a better design. I'm available for working on improving this part on all the stack (from libindicator, to unity itself).

[Marco Trevisan (Treviño) (3v1n0)]

Ah, however to fix this crash (bug #861144) I guess that we only need (at the current state) to add a

   g_return_if_fail (object);

In the panel_service_show_entry function. I forgot to add it previously, and I'd suggest to use this check always, after that the entry_id has been parsed, leaving the old get_entry_by_id implementation.
Otherwise we'll duplicate the number of hashtable lookups.

Otherwise using something like
   void panel_service_get_indicator_entry_by_id(PanelService *self, IndicatorObject **io, IndicatorObjectEntry **entry);

To get also the IndicatorObject and to avoid further lookups.

A simple way to optimize all these operations, by the way, would be to link an entry to its own parent indicator (also if this could lead to other issues, though).

[Michal Hruby (mhr3)]

> Ah, however to fix this crash (bug #861144) I guess that we only need (at the
> current state) to add a
>
> g_return_if_fail (object);
>

Yes that should also suffice, but we *really* don't want unchecked pointer casting.

> In the panel_service_show_entry function. I forgot to add it previously, and
> I'd suggest to use this check always, after that the entry_id has been parsed,
> leaving the old get_entry_by_id implementation.
> Otherwise we'll duplicate the number of hashtable lookups.
>
> Otherwise using something like
> void panel_service_get_indicator_entry_by_id(PanelService *self,
> IndicatorObject **io, IndicatorObjectEntry **entry);
>
> To get also the IndicatorObject and to avoid further lookups.
>

Ok, I'll fix it to do that to save a couple of cpu cycles.

> A simple way to optimize all these operations, by the way, would be to link an
> entry to its own parent indicator (also if this could lead to other issues,
> though).

Feel free to do so, there are too many scary bits in libindicator... Although that'd be most likely an ABI break, so P stuff.

[Marco Trevisan (Treviño) (3v1n0)]

> Feel free to do so, there are too many scary bits in libindicator... Although
> that'd be most likely an ABI break, so P stuff.

Well, I agree. But I think that would be redesigned at all with Ted.

[Marco Trevisan (Treviño) (3v1n0)]

Ah, +1 ;)

review: Approve

[Tim Penhey (thumper)]

Looks fine.

review: Approve

=== modified file 'services/panel-indicator-entry-accessible.c'
--- services/panel-indicator-entry-accessible.c	2011-09-02 14:56:07 +0000
+++ services/panel-indicator-entry-accessible.c	2011-11-17 09:51:25 +0000
@@ -235,9 +235,7 @@
     {
       atk_object_set_role (accessible, ATK_ROLE_IMAGE);
       if (piea->priv->entry->accessible_desc != NULL)
-        {
- 	  atk_object_set_name (accessible, atk_object_get_name (ATK_OBJECT (piea->priv->entry->image)));
-	}
+        atk_object_set_name (accessible, piea->priv->entry->accessible_desc);
     }
 
   if (piea->priv->entry->accessible_desc != NULL)

=== modified file 'services/panel-root-accessible.c'
--- services/panel-root-accessible.c	2011-10-05 15:28:19 +0000
+++ services/panel-root-accessible.c	2011-11-17 09:51:25 +0000
@@ -88,6 +88,26 @@
   return accessible;
 }
 
+static void
+on_indicator_removed (gpointer *data, GObject *removed_indicator)
+{
+  PanelRootAccessible *root = data[0];
+  AtkObject *child = data[1];
+
+  g_return_if_fail (PANEL_IS_ROOT_ACCESSIBLE (root));
+  g_return_if_fail (ATK_IS_OBJECT (child));
+
+  gint index = g_slist_index (root->priv->a11y_children, child);
+  if (index >= 0)
+    {
+      root->priv->a11y_children = g_slist_remove (root->priv->a11y_children, child);
+      g_signal_emit_by_name (root, "children-changed::remove", index, child);
+      g_object_unref (child);
+    }
+
+  g_free (data);
+}
+
 /* Implementation of AtkObject methods */
 
 static void
@@ -111,14 +131,23 @@
       IndicatorObject *indicator;
 
       indicator = panel_service_get_indicator_nth (root->priv->service, i);
-      if (indicator != NULL)
+      if (INDICATOR_IS_OBJECT (indicator))
         {
-	  AtkObject *child;
-
-	  child = panel_indicator_accessible_new (indicator);
+          AtkObject *child;
+          gpointer *data;
+
+          child = panel_indicator_accessible_new (indicator);
+          /* FIXME: use proper signals once we support dynamic adding/removing
+           * of indicators */
+          data = g_new0 (gpointer, 2);
+          data[0] = root;
+          data[1] = child;
+          g_object_weak_ref (G_OBJECT (indicator), 
+              (GWeakNotify) on_indicator_removed, data);
+
           atk_object_set_parent (child, accessible);
-	  root->priv->a11y_children = g_slist_append (root->priv->a11y_children, child);
-	}
+          root->priv->a11y_children = g_slist_append (root->priv->a11y_children, child);
+        }
     }
 }
 

=== modified file 'services/panel-service.c'
--- services/panel-service.c	2011-10-13 16:46:47 +0000
+++ services/panel-service.c	2011-11-17 09:51:25 +0000
@@ -325,15 +325,30 @@
   return NULL;
 }
 
-static IndicatorObjectEntry *
-get_entry_by_id (const gchar *entry_id)
+static void
+panel_service_get_indicator_entry_by_id (PanelService *self, 
+                                         const gchar *entry_id,
+                                         IndicatorObjectEntry **entry,
+                                         IndicatorObject **object)
 {
-  IndicatorObjectEntry *entry;
-  
-  if (sscanf (entry_id, "%p", &entry) == 1)
-    return entry;
+  IndicatorObject *indicator;
+  IndicatorObjectEntry *probably_entry;
+  PanelServicePrivate *priv = self->priv;
 
-  return NULL;
+  /* FIXME: eeek, why do we even do this? */
+  if (sscanf (entry_id, "%p", &probably_entry) == 1)
+  {
+    /* check that there really is such IndicatorObjectEntry */
+    indicator = g_hash_table_lookup (priv->entry2indicator_hash,
+                                     probably_entry);
+    if (object) *object = indicator;
+    if (entry) *entry = indicator != NULL ? probably_entry : NULL;
+  }
+  else
+  {
+    if (object) *object = NULL;
+    if (entry) *entry = NULL;
+  }
 }
 
 static gboolean
@@ -1132,9 +1147,11 @@
                              gint width,
                              gint height)
 {
-  PanelServicePrivate *priv = self->priv;
-  IndicatorObjectEntry *entry = get_entry_by_id (entry_id);
-  IndicatorObject *object = g_hash_table_lookup (priv->entry2indicator_hash, entry);
+  IndicatorObject      *object;
+  IndicatorObjectEntry *entry;
+  PanelServicePrivate  *priv = self->priv;
+
+  panel_service_get_indicator_entry_by_id (self, entry_id, &entry, &object);
 
   if (entry)
     {
@@ -1184,9 +1201,9 @@
           geo->width = width;
           geo->height = height;
         }
+
+      g_signal_emit (self, _service_signals[GEOMETRIES_CHANGED], 0, object, entry, x, y, width, height);
     }
-
-  g_signal_emit (self, _service_signals[GEOMETRIES_CHANGED], 0, object, entry, x, y, width, height);
 }
 
 static gboolean
@@ -1357,10 +1374,12 @@
                           gint32        y,
                           gint32        button)
 {
-  PanelServicePrivate  *priv = self->priv;
-  IndicatorObjectEntry *entry = get_entry_by_id (entry_id);
-  IndicatorObject      *object = g_hash_table_lookup (priv->entry2indicator_hash, entry);
+  IndicatorObject      *object;
+  IndicatorObjectEntry *entry;
   GtkWidget            *last_menu;
+  PanelServicePrivate  *priv = self->priv;
+
+  panel_service_get_indicator_entry_by_id (self, entry_id, &entry, &object);
 
   g_return_if_fail (entry);
 
@@ -1450,12 +1469,12 @@
                                         const gchar  *entry_id,
                                         guint32       timestamp)
 {
-  PanelServicePrivate  *priv = self->priv;
-  IndicatorObjectEntry *entry = get_entry_by_id (entry_id);
+  IndicatorObject      *object;
+  IndicatorObjectEntry *entry;
+
+  panel_service_get_indicator_entry_by_id (self, entry_id, &entry, &object);
   g_return_if_fail (entry);
 
-  IndicatorObject *object = g_hash_table_lookup (priv->entry2indicator_hash, entry);
-
   g_signal_emit_by_name(object, INDICATOR_OBJECT_SIGNAL_SECONDARY_ACTIVATE, entry,
                         timestamp);
 }
@@ -1465,11 +1484,12 @@
                             const gchar    *entry_id,
                             gint32         delta)
 {
-  PanelServicePrivate  *priv = self->priv;
-  IndicatorObjectEntry *entry = get_entry_by_id (entry_id);
+  IndicatorObject      *object;
+  IndicatorObjectEntry *entry;
+
+  panel_service_get_indicator_entry_by_id (self, entry_id, &entry, &object);
   g_return_if_fail (entry);
 
-  IndicatorObject *object = g_hash_table_lookup (priv->entry2indicator_hash, entry);
   GdkScrollDirection direction = delta < 0 ? GDK_SCROLL_DOWN : GDK_SCROLL_UP;
 
   g_signal_emit_by_name(object, INDICATOR_OBJECT_SIGNAL_ENTRY_SCROLLED, entry,