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.

[lp:~compiz-team/compiz/compiz.fix_1018602]

3266. By Sam Spilsbury on 2012-06-28

Use g_variant_iter_next if we don't require the read out value to be free'd immediately

[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)

[lp:~compiz-team/compiz/compiz.fix_1018602]

3267. By Sam Spilsbury on 2012-06-29

Remove variantType and just specify the string

3268. By Sam Spilsbury on 2012-06-29

Plug memleak, s/g_free/free and s/"f"/"d"/

3269. By Sam Spilsbury on 2012-06-29

use g_strfreev

3270. By Sam Spilsbury on 2012-06-29

Merge lp:compiz

3271. By Sam Spilsbury on 2012-06-29

Plug other memleaks.

3272. By Sam Spilsbury on 2012-06-29

Merge lp:compiz

Unmerged revisions

3272. By Sam Spilsbury on 2012-06-29

Merge lp:compiz

=== modified file 'compizconfig/gsettings/src/gsettings.c'
--- compizconfig/gsettings/src/gsettings.c	2012-05-21 15:37:47 +0000
+++ compizconfig/gsettings/src/gsettings.c	2012-06-29 02:02:20 +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,
@@ -122,10 +147,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)
@@ -163,26 +189,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;
 }
@@ -277,36 +308,17 @@
 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);
@@ -316,8 +328,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)
     {
@@ -325,14 +337,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);
@@ -342,14 +354,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);
@@ -359,14 +371,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, "f", &value))
+		*arrayCounter++ = value;
 
 	    list = ccsGetValueListFromFloatArray ((float *) array, nItems, setting);
 	    free (array);
@@ -375,24 +387,21 @@
     case TypeString:
     case TypeMatch:
 	{
-	    char **array = calloc (1, (nItems + 1) * sizeof (char *));
-	    char **arrayCounter = array;
+	    gchar **array = calloc (1, (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_free (array);
 	}
 	break;
     case TypeColor:
@@ -403,7 +412,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,
@@ -418,7 +427,6 @@
     }
 
     free (cleanSettingName);
-    free (variantType);
 
     if (list)
     {
@@ -639,8 +647,7 @@
 		char       *pathName)
 {
     GSettings  		*settings = getSettingsObjectForCCSSetting (setting);
-    GVariant 		*value;
-    gchar		*variantType = NULL;
+    GVariant 		*value = NULL;
     CCSSettingValueList list;
 
     char *cleanSettingName = translateKeyForGSettings (setting->name);
@@ -652,7 +659,6 @@
     {
     case TypeBool:
 	{
-	    variantType = "ab";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("ab"));
 	    while (list)
 	    {
@@ -665,7 +671,6 @@
 	break;
     case TypeInt:
 	{
-	    variantType = "ai";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("ai"));
 	    while (list)
 	    {
@@ -678,7 +683,6 @@
 	break;
     case TypeFloat:
 	{
-	    variantType = "ad";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("ad"));
 	    while (list)
 	    {
@@ -691,7 +695,6 @@
 	break;
     case TypeString:
 	{
-	    variantType = "as";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
 	    while (list)
 	    {
@@ -704,7 +707,6 @@
 	break;
     case TypeMatch:
 	{
-	    variantType = "as";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
 	    while (list)
 	    {
@@ -717,7 +719,6 @@
 	break;
     case TypeColor:
 	{
-	    variantType = "as";
 	    GVariantBuilder *builder = g_variant_builder_new (G_VARIANT_TYPE ("as"));
 	    char *item;
 	    while (list)
@@ -733,11 +734,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);
@@ -920,15 +920,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);
 
@@ -942,7 +942,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);
 
@@ -1156,15 +1155,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;
 
@@ -1190,13 +1189,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);