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

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

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

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;

> 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.

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

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

Done in r3461.

Ping.

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

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

Hopefully fixed in r3462. :)

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.
>

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)".

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.
>

Could you please resubmit this for 0.9.9 as well?

Thanks.

@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.