Merge lp:~mc-return/compiz/compiz.merge-fix-memory-leaks-in-libcompizconfig into lp:compiz/0.9.10

[MC Return]

Hopefully fixed all memory leaks in libcompizconfig.
Minor whitespace fixes.

(LP: #1076297)

Note: Usually switch-case commands do not use {} brackets for the individual cases, but I kept the style already used.

Note 2: case TypeAction has no break, so it is the same as default (probably intentional).

[Daniel van Vugt (vanvugt)]

One branch/proposal per bug please. It gets too confusing for Launchpad when there are multiple branches to fix a single bug.

review: Needs Resubmitting

[MC Return (mc-return)]

> One branch/proposal per bug please. It gets too confusing for Launchpad when
> there are multiple branches to fix a single bug.

Created a new bug report for memory leaks in libcompizconfig.
The old one showed wrong line numbers anyway, so it was outdated already.

(Info: cppcheck seems to be wrong on the third reported leak of the pointer upgradeName, so I did not fix it)

review: Needs Resubmitting

[Daniel van Vugt (vanvugt)]

There's a return under asprintf that needs fixing also:
    char *keyName = NULL;
    char *sectionName = strdup (ccsPluginGetName (ccsSettingGetParent (setting)));
    char *iniValue = NULL;

    CCSContextPrivate *cPrivate = GET_PRIVATE (CCSContextPrivate, context);

    if (asprintf (&keyName, "+s%d_%s", cPrivate->screenNum, ccsSettingGetName (setting)) == -1)
---> return FALSE;

    if (ccsIniGetString (dict, sectionName, keyName, &iniValue))
    {
        CCSSetting *newSetting = malloc (sizeof (CCSSetting));

        if (!newSetting)
        {
            free (sectionName);
            return FALSE;
        }

and also two more in ccsProcessSettingMinus ...

        if (!newSetting)
---> return FALSE;
        ...
           default:
                /* FIXME: cleanup */
---> return FALSE;

review: Needs Fixing

[MC Return (mc-return)]

> There's a return under asprintf that needs fixing also:
> char *keyName = NULL;
> char *sectionName = strdup (ccsPluginGetName (ccsSettingGetParent
> (setting)));
> char *iniValue = NULL;
>
> CCSContextPrivate *cPrivate = GET_PRIVATE (CCSContextPrivate, context);
>
> if (asprintf (&keyName, "+s%d_%s", cPrivate->screenNum, ccsSettingGetName
> (setting)) == -1)
> ---> return FALSE;
>
> if (ccsIniGetString (dict, sectionName, keyName, &iniValue))
> {
> CCSSetting *newSetting = malloc (sizeof (CCSSetting));
>
> if (!newSetting)
> {
> free (sectionName);
> return FALSE;
> }
>
> and also two more in ccsProcessSettingMinus ...
>
> if (!newSetting)
> ---> return FALSE;
> ...
> default:
> /* FIXME: cleanup */
> ---> return FALSE;

Oh - you are right. Thanks for making me aware - I did not notice it :)
Hopefully fixed in r3459.

[Daniel van Vugt (vanvugt)]

I think the FIXME needs to be removed and replaced with:
    free (newSetting)
at least.

review: Needs Fixing

[MC Return (mc-return)]

> I think the FIXME needs to be removed and replaced with:
> free (newSetting)
> at least.

Done in r3461.

review: Needs Resubmitting

[MC Return (mc-return)]

Ping.

[Daniel van Vugt (vanvugt)]

Another FIXME in the area of the code being changed (line 4422)
            default:
                /* FIXME: cleanup */
                return FALSE;

review: Needs Fixing

[MC Return (mc-return)]

> Another FIXME in the area of the code being changed (line 4422)
> default:
> /* FIXME: cleanup */
> return FALSE;

Hopefully fixed in r3462. :)

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

Thats correct.
On 20/11/2012 9:24 AM, "MC Return" <email address hidden> wrote:

> The proposal to merge
> lp:~mc-return/compiz/compiz.merge-fix-memory-leaks-in-libcompizconfig into
> lp:compiz has been updated.
>
> Description changed to:
>
> Note: Usually switch-case commands do not use {} brackets for the
> individual cases, but I kept the style already used.
>
> Note 2: case TypeAction has no break, so it is the same as default
> (probably intentional).
>
> For more details, see:
>
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix-memory-leaks-in-libcompizconfig/+merge/133432
> --
>
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix-memory-leaks-in-libcompizconfig/+merge/133432
> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>

[Daniel van Vugt (vanvugt)]

Second, third, fifth and sixth blocks are missing two frees each:
    free (keyName);
    free (iniValue);
So that's two frees missing twice in each function = 4 frees.

Sorry I didn't notice before.

I recommend a single return statement per function to simplify memory management and easily avoid leaks of any sort:

Bool ret = FALSE;
void *p1 = NULL;
void *p2 = NULL;
...
if (error)
    goto cleanup;
...
if (some other error)
    goto cleanup;
...
ret = TRUE;
cleanup:
free (p1);
free (p2);
return ret;

free() is safe to use with NULL. But for other resource types you'd just use "if (r != INVALID) FreeR(r)".

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

Avoid using goto please.

Use a dedicated cleanup function.

void freeFunctionData (Foo *, Bar *)
{
free (foo);
barDestroy (bar);
}
On 20/11/2012 6:29 PM, "Daniel van Vugt" <email address hidden>
wrote:

> Review: Needs Fixing
>
> Second, third, fifth and sixth blocks are missing two frees each:
> free (keyName);
> free (iniValue);
> So that's two frees missing twice in each function = 4 frees.
>
> Sorry I didn't notice before.
>
> I recommend a single return statement per function to simplify memory
> management and easily avoid leaks of any sort:
>
> Bool ret = FALSE;
> void *p1 = NULL;
> void *p2 = NULL;
> ...
> if (error)
> goto cleanup;
> ...
> if (some other error)
> goto cleanup;
> ...
> ret = TRUE;
> cleanup:
> free (p1);
> free (p2);
> return ret;
>
> free() is safe to use with NULL. But for other resource types you'd just
> use "if (r != INVALID) FreeR(r)".
> --
>
> https://code.launchpad.net/~mc-return/compiz/compiz.merge-fix-memory-leaks-in-libcompizconfig/+merge/133432
> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>

[MC Return (mc-return)]

I did not rewrite everything here, but the leaks should be fixed.
IMHO we do not really need anything more complicated here...

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

This will do for now, but the relevant cleanup should be refactored.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

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

Could you please resubmit this for 0.9.9 as well?

Thanks.

review: Needs Resubmitting

[MC Return (mc-return)]

@3v1nO:
I could do that, but this is not the only important fix that went into 0.9.10-dev.
It would mean a lot of non-exciting work and would cost a lot of time to backport everything important to 0.9.9...
I am sure I do not want to do that unpaid in my free time...

The best solution would be to make current 0.9.10-dev the 0.9.9.2 release.

=== modified file 'compizconfig/libcompizconfig/src/main.c'
--- compizconfig/libcompizconfig/src/main.c	2013-02-21 18:33:06 +0000
+++ compizconfig/libcompizconfig/src/main.c	2013-04-02 20:34:21 +0000
@@ -4376,14 +4376,22 @@
     CCSContextPrivate *cPrivate = GET_PRIVATE (CCSContextPrivate, context);
 
     if (asprintf (&keyName, "+s%d_%s", cPrivate->screenNum, ccsSettingGetName (setting)) == -1)
+    {
+	free (sectionName);
 	return FALSE;
+    }
 
     if (ccsIniGetString (dict, sectionName, keyName, &iniValue))
     {
 	CCSSetting *newSetting = malloc (sizeof (CCSSetting));
 
 	if (!newSetting)
+	{
+	    free (sectionName);
+	    free (keyName);
+	    free (iniValue);
 	    return FALSE;
+	}
 
 	ccsObjectInit (newSetting, &ccsDefaultObjectAllocator);
 
@@ -4484,8 +4492,13 @@
 	    }
 	    case TypeAction:
 	    default:
-		/* FIXME: cleanup */
+	    {
+		free (newSetting);
+		free (sectionName);
+		free (keyName);
+		free (iniValue);
 		return FALSE;
+	    }
 	}
 
         CCSSettingList listIter = upgrade->clearValueSettings;
@@ -4514,10 +4527,11 @@
 	
 	return TRUE;
     }
-    
+
     free (keyName);
     free (sectionName);
-    
+    free (iniValue);
+
     return FALSE;
 }
 
@@ -4534,14 +4548,22 @@
     CCSContextPrivate *cPrivate = GET_PRIVATE (CCSContextPrivate, context);
 
     if (asprintf (&keyName, "-s%d_%s", cPrivate->screenNum, ccsSettingGetName (setting)) == -1)
+    {
+	free (sectionName);
 	return FALSE;
+    }
 
     if (ccsIniGetString (dict, sectionName, keyName, &iniValue))
     {
 	CCSSetting *newSetting = malloc (sizeof (CCSSetting));
 
 	if (!newSetting)
+	{
+	    free (sectionName);
+	    free (keyName);
+	    free (iniValue);
 	    return FALSE;
+	}
 
 	ccsObjectInit (newSetting, &ccsDefaultObjectAllocator);
 
@@ -4642,8 +4664,13 @@
 	    }
 	    case TypeAction:
 	    default:
-		/* FIXME: cleanup */
+	    {
+		free (newSetting);
+		free (sectionName);
+		free (keyName);
+		free (iniValue);
 		return FALSE;
+	    }
 	}
 
         CCSSettingList listIter = upgrade->addValueSettings;
@@ -4675,6 +4702,7 @@
     
     free (keyName);
     free (sectionName);
+    free (iniValue);
 
     return FALSE;
 }