ayatana-commits team mailing list archive

[Ted Gould <ted@xxxxxxxx>]

Ted Gould has proposed merging lp:~ted/dbusmenu/proxy-text-fix into lp:dbusmenu.

Requested reviews:
  DBus Menu Team (dbusmenu-team)

For more details, see:
https://code.launchpad.net/~ted/dbusmenu/proxy-text-fix/+merge/51848

Wow.  You're not going to believe it.  Basically if you pass a string into remove that is also stored in the hashtable the string will get free'd and written over fast enough that the signal handler sends bad data.  That took forever to debug.
-- 
https://code.launchpad.net/~ted/dbusmenu/proxy-text-fix/+merge/51848
Your team ayatana-commits is subscribed to branch lp:dbusmenu.

=== modified file 'libdbusmenu-glib/client.c'
--- libdbusmenu-glib/client.c	2011-02-24 14:33:38 +0000
+++ libdbusmenu-glib/client.c	2011-03-02 03:26:13 +0000
@@ -1168,7 +1168,7 @@
 			gchar * property;
 
 			while (g_variant_iter_next(&properties, "s", &property)) {
-				g_debug("Removing property '%s' on %d", property, id);
+				/* g_debug("Removing property '%s' on %d", property, id); */
 				dbusmenu_menuitem_property_remove(menuitem, property);
 			}
 		}

=== modified file 'libdbusmenu-glib/menuitem.c'
--- libdbusmenu-glib/menuitem.c	2011-03-01 11:15:19 +0000
+++ libdbusmenu-glib/menuitem.c	2011-03-02 03:26:13 +0000
@@ -1146,6 +1146,7 @@
 {
 	g_return_val_if_fail(DBUSMENU_IS_MENUITEM(mi), FALSE);
 	g_return_val_if_fail(property != NULL, FALSE);
+	g_return_val_if_fail(g_utf8_validate(property, -1, NULL), FALSE);
 
 	DbusmenuMenuitemPrivate * priv = DBUSMENU_MENUITEM_GET_PRIVATE(mi);
 	GVariant * default_value = NULL;
@@ -1181,6 +1182,7 @@
 
 
 	gboolean replaced = FALSE;
+	gboolean remove = FALSE;
 	gpointer currentval = g_hash_table_lookup(priv->properties, property);
 
 	if (value != NULL) {
@@ -1195,10 +1197,21 @@
 		gchar * lprop = g_strdup(property);
 		g_variant_ref_sink(value);
 
-		g_hash_table_replace(priv->properties, lprop, value);
+		/* Really important that this is _insert as that means the value
+		   that we just created in the _strdup is free'd and not the one
+		   currently in the hashtable.  That could be the same as the one
+		   being passed in and then the signal emit would be done with a
+		   bad value */
+		g_hash_table_insert(priv->properties, lprop, value);
 	} else {
 		if (currentval != NULL) {
-			g_hash_table_remove(priv->properties, property);
+		/* So the question you should be asking if you're paying attention
+		   is "Why not just do the remove here?"  It's a good question with
+		   an interesting answer.  Bascially it's the same reason as above,
+		   in a couple cases the passed in properties is the value in the hash
+		   table so we can avoid strdup'ing it by removing it (and thus free'ing
+		   it) after the signal emition */
+			remove = TRUE;
 			replaced = TRUE;
 		}
 	}
@@ -1219,6 +1232,10 @@
 		g_signal_emit(G_OBJECT(mi), signals[PROPERTY_CHANGED], 0, property, signalval, TRUE);
 	}
 
+	if (remove) {
+		g_hash_table_remove(priv->properties, property);
+	}
+
 	return TRUE;
 }