Merge lp:~compiz-team/compiz/compiz.fix_1018602 into lp:compiz/0.9.8

[Sam Spilsbury]

Fixes LP (#1018602) : An invalid read when using g_variant_iter_loop.

No tests yet, tests will be added to gsettings in another commit when the lcc testing work lands.

[Daniel van Vugt (vanvugt)]

review: Approve

[Daniel van Vugt (vanvugt)]

Oops. There is a problem under case TypeMatch:
array is no longer initialized with zeros because you changed calloc to malloc. So the free() loop after it could potentially free an invalid pointer if g_variant_iter_loop doesn't find exactly nItems.

Also, strdup(value) is not necessary if you use:
     while (g_variant_iter_next (iter, "s", &value))
  *arrayCounter++ = value;
and then change the free() calls below it to g_free().

On that note, the variable variantType should be removed from readListValue. Just use "s", "i", "b" and "d" instead. It will be smaller, faster and more readable.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

Coolio, fixed the calloc issue, plugged a few other memleaks and fixed some gvariant errors I noticed.

I abstracted out the variantType stuff into a function - it should probably be transparent to the user as to the implementation detail of having to pass an arbitrary string to g_variant_get

[Sam Spilsbury (smspillaz)]

Thanks for the tip on using g_variant_iter_next ... that's now fixed too.

[Daniel van Vugt (vanvugt)]

The addition of compizconfigTypeToVariantType makes no sense.

We already have a tight coupling between:
    TYPE x;
and:
    g_variant_iter_loop (&iter, TYPESTRING, &x)

Using:
    g_variant_iter_loop (&iter, someVariable, &x)
only makes the code harder to read, with looser coupling and weaker cohesion.

Please remove compizconfigTypeToVariantType and just use:
    g_variant_iter_loop (&iter, "X", &x)

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

It's the same as:
    printf(secretFormatString, x, y, z);
It's too hard to read so adds maintenance risk.

Everyone would much prefer to read:
    printf("The results are: %d %s %f\n", x, y, z);

[Sam Spilsbury (smspillaz)]

I think the printf analogy in this case doesn't apply here, the type string is merely an implementation detail - we know from the code what we want to extract already, and the type string is there to operate the valist API.

In any case, I'll change it.

[Daniel van Vugt (vanvugt)]

Maybe. But back to the actual case of g_variant_iter_loop(I, F, V)... F and V are tightly coupled and need to match up so you should see the explicit value of F and type of V within the same block of code...

int v;
g_variant_iter_loop(&x, "i", &v)

[Sam Spilsbury (smspillaz)]

I had another look at the code and variantType is more or less used as a placeholder so that copypasting was easier and so that we could return if there was no variant type for that setting. The fact that we are copypasting is a problem, but I modified the function so that it returns a boolean for whether or not we can read that type of setting.

[Daniel van Vugt (vanvugt)]

It works OK, but I can see some potentially serious mistakes:

203: "f" should be "d"
(http://developer.gnome.org/glib/2.30/glib-GVariant.html#GVariantClass)

235: Missing g_free() loop:
    for (i = 0; i < nItems; i++)
        if (array[i])
            g_free(array[i]);

239: g_free() on calloc'd block (214). Either change g_free() to free(), or calloc() to g_malloc0().

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

> It works OK, but I can see some potentially serious mistakes:
>
> 203: "f" should be "d"
> (http://developer.gnome.org/glib/2.30/glib-GVariant.html#GVariantClass)

Thanks, fixed.

>
> 235: Missing g_free() loop:
> for (i = 0; i < nItems; i++)
> if (array[i])
> g_free(array[i]);

My understanding is that g_variant_iter_next returns a shallow copy of the array, so free () is sufficient in this case (unlike the previous code where we copied the values out of the array. Valgrind reports there are no leaks here.

>
> 239: g_free() on calloc'd block (214). Either change g_free() to free(), or
> calloc() to g_malloc0().

Fixed, although I think g_malloc is a wrapper around malloc itself (eg, doesn't use g_slice_alloc)

[Daniel van Vugt (vanvugt)]

No, you still need the g_free loop to free each array element after g_variant_iter_next:
  http://developer.gnome.org/glib/2.32/glib-GVariant.html#g-variant-iter-next
It's possible valgrind doesn't see the leak because the memory is managed by glib and freed at exit. But it's still a leak even still.

Also, I was referring to g_malloc0() to replace calloc. Not g_malloc(). :)

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

You're right, I was confusing the array with the values themselves. Fixing it now.

[Daniel van Vugt (vanvugt)]

Still needs fixing. Please don't click resubmit till after you've pushed the changes.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Sorry, my fault. I commented on an old version of the proposal. But still needs fixing.

[Sam Spilsbury (smspillaz)]

Fixed that, plugged other memleaks.

[Daniel van Vugt (vanvugt)]

Thanks for the extra leak fixes but they're not part of fixing the crash.

I give up. It looks OK now but I don't want to manually test and valgrind it again. We'll be valgrinding a lot more in the near future.

review: Approve

=== modified file 'compizconfig/gsettings/src/gsettings.c'
--- compizconfig/gsettings/src/gsettings.c	2012-06-28 09:03:09 +0000
+++ compizconfig/gsettings/src/gsettings.c	2012-06-29 07:54:24 +0000
@@ -33,6 +33,31 @@
 
 #include "gsettings.h"
 
+static gboolean
+compizconfigTypeHasVariantType (CCSSettingType type)
+{
+    gint i = 0;
+
+    static const unsigned int nVariantTypes = 6;
+    static const CCSSettingType variantTypes[] =
+    {
+	TypeString,
+	TypeMatch,
+	TypeColor,
+	TypeBool,
+	TypeInt,
+	TypeFloat
+    };
+
+    for (; i < nVariantTypes; i++)
+    {
+	if (variantTypes[i] == type)
+	    return TRUE;
+    }
+
+    return FALSE;
+}
+
 static void
 valueChanged (GSettings   *settings,
 	      gchar	  *keyname,
@@ -121,10 +146,11 @@
     GList *l = settingsList;
     gchar *schemaName = getSchemaNameForPlugin (plugin);
     GVariant        *writtenPlugins;
+    gsize            writtenPluginsLen;
+    gsize            newWrittenPluginsSize;
+    gchar           **newWrittenPlugins;
     char	    *plug;
-    GVariant        *newWrittenPlugins;
-    GVariantBuilder *newWrittenPluginsBuilder;
-    GVariantIter    *iter;
+    GVariantIter    iter;
     gboolean	    found = FALSE;
 
     while (l)
@@ -143,6 +169,8 @@
 	    return settingsObj;
 	}
 
+	g_free (name);
+
 	l = g_list_next (l);
     }
 
@@ -162,26 +190,31 @@
 
     writtenPlugins = g_settings_get_value (currentProfileSettings, "plugins-with-set-keys");
 
-    newWrittenPluginsBuilder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
+    g_variant_iter_init (&iter, writtenPlugins);
+    newWrittenPluginsSize = g_variant_iter_n_children (&iter);
 
-    iter = g_variant_iter_new (writtenPlugins);
-    while (g_variant_iter_loop (iter, "s", &plug))
+    while (g_variant_iter_loop (&iter, "s", &plug))
     {
-	g_variant_builder_add (newWrittenPluginsBuilder, "s", plug);
-
 	if (!found)
 	    found = (g_strcmp0 (plug, plugin) == 0);
     }
 
     if (!found)
-	g_variant_builder_add (newWrittenPluginsBuilder, "s", plugin);
-
-    newWrittenPlugins = g_variant_new ("as", newWrittenPluginsBuilder);
-    g_settings_set_value (currentProfileSettings, "plugins-with-set-keys", newWrittenPlugins);
-
-    g_variant_iter_free (iter);
-    g_variant_unref (newWrittenPlugins);
-    g_variant_builder_unref (newWrittenPluginsBuilder);
+	newWrittenPluginsSize++;
+
+    newWrittenPlugins = g_variant_dup_strv (writtenPlugins, &writtenPluginsLen);
+
+    if (writtenPluginsLen > newWrittenPluginsSize)
+    {
+	newWrittenPlugins = g_realloc (newWrittenPlugins, (newWrittenPluginsSize + 1) * sizeof (gchar *));
+	newWrittenPlugins[writtenPluginsLen + 1]  = g_strdup (plugin);
+	newWrittenPlugins[newWrittenPluginsSize] = NULL;
+    }
+
+    g_settings_set_strv (currentProfileSettings, "plugins-with-set-keys", (const gchar * const *)newWrittenPlugins);
+
+    g_free (schemaName);
+    g_strfreev (newWrittenPlugins);
 
     return settingsObj;
 }
@@ -213,36 +246,52 @@
 {
     CCSContext   *context = (CCSContext *)user_data;
     char	 *uncleanKeyName;
-    char	 *path;
+    char	 *path, *pathOrig;
     char         *token;
     int          index;
     unsigned int screenNum;
     CCSPlugin    *plugin;
     CCSSetting   *setting;
 
-    g_object_get (G_OBJECT (settings), "path", &path, NULL);
+    g_object_get (G_OBJECT (settings), "path", &pathOrig, NULL);
 
+    path = pathOrig;
     path += strlen (COMPIZ) + 1;
 
     token = strsep (&path, "/"); /* Profile name */
     if (!token)
+    {
+	g_free (pathOrig);
 	return;
+    }
 
     token = strsep (&path, "/"); /* plugins */
     if (!token)
+    {
+	g_free (pathOrig);
 	return;
+    }
 
     token = strsep (&path, "/"); /* plugin */
     if (!token)
+    {
+	g_free (pathOrig);
 	return;
+    }
 
     plugin = ccsFindPlugin (context, token);
     if (!plugin)
+    {
+	g_free (pathOrig);
 	return;
+    }
 
     token = strsep (&path, "/"); /* screen%i */
     if (!token)
+    {
+	g_free (pathOrig);
 	return;
+    }
 
     sscanf (token, "screen%d", &screenNum);
 
@@ -253,6 +302,7 @@
     {
 	printf ("GSettings Backend: unable to find setting %s, for path %s\n", uncleanKeyName, path);
 	free (uncleanKeyName);
+	g_free (pathOrig);
 	return;
     }
 
@@ -270,42 +320,24 @@
     }
 
     free (uncleanKeyName);
+    g_free (pathOrig);
 }
 
 static Bool
 readListValue (CCSSetting *setting)
 {
     GSettings		*settings = getSettingsObjectForCCSSetting (setting);
-    gchar		*variantType;
+    gboolean		hasVariantType;
     unsigned int        nItems, i = 0;
     CCSSettingValueList list = NULL;
     GVariant		*value;
-    GVariantIter	*iter;
+    GVariantIter	iter;
 
     char *cleanSettingName = translateKeyForGSettings (setting->name);
     
-    switch (setting->info.forList.listType)
-    {
-    case TypeString:
-    case TypeMatch:
-    case TypeColor:
-	variantType = g_strdup ("s");
-	break;
-    case TypeBool:
-	variantType = g_strdup ("b");
-	break;
-    case TypeInt:
-	variantType = g_strdup ("i");
-	break;
-    case TypeFloat:
-	variantType = g_strdup ("d");
-	break;
-    default:
-	variantType = NULL;
-	break;
-    }
+    hasVariantType = compizconfigTypeHasVariantType (setting->info.forList.listType);
 
-    if (!variantType)
+    if (!hasVariantType)
 	return FALSE;
 
     value = g_settings_get_value (settings, cleanSettingName);
@@ -315,8 +347,8 @@
 	return TRUE;
     }
 
-    iter = g_variant_iter_new (value);
-    nItems = g_variant_iter_n_children (iter);
+    g_variant_iter_init (&iter, value);
+    nItems = g_variant_iter_n_children (&iter);
 
     switch (setting->info.forList.listType)
     {
@@ -324,14 +356,14 @@
 	{
 	    Bool *array = malloc (nItems * sizeof (Bool));
 	    Bool *arrayCounter = array;
+	    gboolean value;
 
 	    if (!array)
 		break;
-	    
-	    /* Reads each item from the variant into the position pointed
-	     * at by arrayCounter */
-	    while (g_variant_iter_loop (iter, variantType, arrayCounter))
-		arrayCounter++;
+
+	    /* Reads each item from the variant into arrayCounter */
+	    while (g_variant_iter_loop (&iter, "b", &value))
+		*arrayCounter++ = value;
 
 	    list = ccsGetValueListFromBoolArray (array, nItems, setting);
 	    free (array);
@@ -341,14 +373,14 @@
 	{
 	    int *array = malloc (nItems * sizeof (int));
 	    int *arrayCounter = array;
+	    gint value;
 
 	    if (!array)
 		break;
-	    
-	    /* Reads each item from the variant into the position pointed
-	     * at by arrayCounter */
-	    while (g_variant_iter_loop (iter, variantType, arrayCounter))
-		arrayCounter++;
+
+	    /* Reads each item from the variant into arrayCounter */
+	    while (g_variant_iter_loop (&iter, "i", &value))
+		*arrayCounter++ = value;
 
 	    list = ccsGetValueListFromIntArray (array, nItems, setting);
 	    free (array);
@@ -358,14 +390,14 @@
 	{
 	    double *array = malloc (nItems * sizeof (double));
 	    double *arrayCounter = array;
+	    gdouble value;
 
 	    if (!array)
 		break;
-	    
-	    /* Reads each item from the variant into the position pointed
-	     * at by arrayCounter */
-	    while (g_variant_iter_loop (iter, variantType, arrayCounter))
-		arrayCounter++;
+
+	    /* Reads each item from the variant into arrayCounter */
+	    while (g_variant_iter_loop (&iter, "d", &value))
+		*arrayCounter++ = value;
 
 	    list = ccsGetValueListFromFloatArray ((float *) array, nItems, setting);
 	    free (array);
@@ -374,24 +406,21 @@
     case TypeString:
     case TypeMatch:
 	{
-	    char **array = calloc (1, (nItems + 1) * sizeof (char *));
-	    char **arrayCounter = array;
+	    gchar **array = g_malloc0 ((nItems + 1) * sizeof (gchar *));
+	    gchar **arrayCounter = array;
+	    gchar *value;
 
 	    if (!array)
-	    {
 		break;
-	    }
-	    
-	    /* Reads each item from the variant into the position pointed
-	     * at by arrayCounter */
-	    while (g_variant_iter_loop (iter, variantType, arrayCounter))
-		arrayCounter++;
+
+	    array[nItems] = NULL;
+
+	    /* Reads each item from the variant into arrayCounter */
+	    while (g_variant_iter_next (&iter, "s", &value))
+		*arrayCounter++ = value;
 
 	    list = ccsGetValueListFromStringArray (array, nItems, setting);
-	    for (i = 0; i < nItems; i++)
-		if (array[i])
-		    free (array[i]);
-	    free (array);
+	    g_strfreev (array);
 	}
 	break;
     case TypeColor:
@@ -402,7 +431,7 @@
 	    if (!array)
 		break;
 
-	    while (g_variant_iter_loop (iter, variantType, &colorValue))
+	    while (g_variant_iter_loop (&iter, "s", &colorValue))
     	    {
 		memset (&array[i], 0, sizeof (CCSSettingColorValue));
 		ccsStringToColor (colorValue,
@@ -417,7 +446,6 @@
     }
 
     free (cleanSettingName);
-    free (variantType);
 
     if (list)
     {
@@ -638,8 +666,7 @@
 		char       *pathName)
 {
     GSettings  		*settings = getSettingsObjectForCCSSetting (setting);
-    GVariant 		*value;
-    gchar		*variantType = NULL;
+    GVariant 		*value = NULL;
     CCSSettingValueList list;
 
     char *cleanSettingName = translateKeyForGSettings (setting->name);
@@ -651,7 +678,6 @@
     {
     case TypeBool:
 	{
-	    variantType = "ab";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("ab"));
 	    while (list)
 	    {
@@ -664,7 +690,6 @@
 	break;
     case TypeInt:
 	{
-	    variantType = "ai";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("ai"));
 	    while (list)
 	    {
@@ -677,7 +702,6 @@
 	break;
     case TypeFloat:
 	{
-	    variantType = "ad";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("ad"));
 	    while (list)
 	    {
@@ -690,7 +714,6 @@
 	break;
     case TypeString:
 	{
-	    variantType = "as";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
 	    while (list)
 	    {
@@ -703,7 +726,6 @@
 	break;
     case TypeMatch:
 	{
-	    variantType = "as";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
 	    while (list)
 	    {
@@ -716,7 +738,6 @@
 	break;
     case TypeColor:
 	{
-	    variantType = "as";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
 	    char *item;
 	    while (list)
@@ -732,11 +753,10 @@
     default:
 	printf("GSettings backend: attempt to write unsupported list type %d!\n",
 	       setting->info.forList.listType);
-	variantType = NULL;
 	break;
     }
 
-    if (variantType != NULL)
+    if (value)
     {
 	g_settings_set_value (settings, cleanSettingName, value);
 	g_variant_unref (value);
@@ -911,7 +931,7 @@
 }
 
 static void
-updateCurrentProfileName (char *profile)
+updateCurrentProfileName (const char *profile)
 {
     GVariant        *profiles;
     char	    *prof;
@@ -919,15 +939,15 @@
     char	    *currentProfilePath;
     GVariant        *newProfiles;
     GVariantBuilder *newProfilesBuilder;
-    GVariantIter    *iter;
+    GVariantIter    iter;
     gboolean        found = FALSE;
 
     profiles = g_settings_get_value (compizconfigSettings, "existing-profiles");
 
     newProfilesBuilder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
 
-    iter = g_variant_iter_new (profiles);
-    while (g_variant_iter_loop (iter, "s", &prof))
+    g_variant_iter_init (&iter, profiles);
+    while (g_variant_iter_loop (&iter, "s", &prof))
     {
 	g_variant_builder_add (newProfilesBuilder, "s", prof);
 
@@ -941,7 +961,6 @@
     newProfiles = g_variant_new ("as", newProfilesBuilder);
     g_settings_set_value (compizconfigSettings, "existing-profiles", newProfiles);
 
-    g_variant_iter_free (iter);
     g_variant_unref (newProfiles);
     g_variant_builder_unref (newProfilesBuilder);
 
@@ -962,8 +981,14 @@
 {
     char *profile = strdup (ccsGetProfile (context));
 
-    if (!profile || !strlen (profile))
-	profile = strdup (DEFAULTPROF);
+    if (!profile)
+	profile = strdup (DEFAULTPROF);
+
+    if (!strlen (profile))
+    {
+	free (profile);
+	profile = strdup (DEFAULTPROF);
+    }
 
     if (g_strcmp0 (profile, currentProfile))
 	updateCurrentProfileName (profile);
@@ -1155,15 +1180,15 @@
     GVariant        *newProfiles;
     GVariantBuilder *newProfilesBuilder;
     char            *plugin, *prof;
-    GVariantIter    *iter;
+    GVariantIter    iter;
     char            *profileSettingsPath = g_strconcat (PROFILEPATH, profile, "/", NULL);
     GSettings       *profileSettings = g_settings_new_with_path (PROFILE_SCHEMA_ID, profileSettingsPath);
 
     plugins = g_settings_get_value (currentProfileSettings, "plugins-with-set-keys");
     profiles = g_settings_get_value (compizconfigSettings, "existing-profiles");
 
-    iter = g_variant_iter_new (plugins);
-    while (g_variant_iter_loop (iter, "s", &plugin))
+    g_variant_iter_init (&iter, plugins);
+    while (g_variant_iter_loop (&iter, "s", &plugin))
     {
 	GSettings *settings;
 
@@ -1189,13 +1214,12 @@
     }
 
     /* Remove the profile from existing-profiles */
-    g_variant_iter_free (iter);
     g_settings_reset (profileSettings, "plugins-with-set-values");
 
-    iter = g_variant_iter_new (profiles);
+    g_variant_iter_init (&iter, profiles);
     newProfilesBuilder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
 
-    while (g_variant_iter_loop (iter, "s", &prof))
+    while (g_variant_iter_loop (&iter, "s", &prof))
     {
 	if (g_strcmp0 (prof, profile))
 	    g_variant_builder_add (newProfilesBuilder, "s", prof);