Memory leak in FcPatternObjectAddWithBinding
I believe I have found a memory leak in this function (from 'main' as of 8 Nov 21):
FcBool
FcPatternObjectAddWithBinding (FcPattern *p,
FcObject object,
FcValue value,
FcValueBinding binding,
FcBool append)
{
FcPatternElt *e;
FcValueListPtr new, *prev;
if (FcRefIsConst (&p->ref))
goto bail0;
new = FcValueListCreate ();
if (!new)
goto bail0;
value = FcValueSave (value);
if (value.type == FcTypeVoid)
goto bail1;
/*
* Make sure the stored type is valid for built-in objects
*/
if (!FcObjectValidType (object, value.type))
{
fprintf (stderr,
"Fontconfig warning: FcPattern object %s does not accept value",
FcObjectName (object));
FcValuePrintFile (stderr, value);
fprintf (stderr, "\n");
goto bail1;
}
new->value = value;
new->binding = binding;
new->next = NULL;
e = FcPatternObjectInsertElt (p, object);
if (!e)
goto bail2;
if (append)
{
for (prev = &e->values; *prev; prev = &(*prev)->next)
;
*prev = new;
}
else
{
new->next = e->values;
e->values = new;
}
return FcTrue;
bail2:
FcValueDestroy (value);
bail1:
free (new);
bail0:
return FcFalse;
}
There are two things I noted: firstly, FcValueSave can return a value that contains pointers to heap memory (e.g. via strdup). Secondly, that although "free(new)" is adequate to clean up the FcValueListCreate result, as soon as things are written to 'new' the FcValueListDestroy function should be used. Finally once FcValueSave has been called, even if not assigned to 'new', FcValueDestroy must be called on 'value'.
I would suggest changing the code thus:
-
Move "value = FcValueSave(value);" to below the FcObjectValidType() call & 'if' block. As far as I can see, it doesn't need to be any earlier. The reference to 'value.type' in the 'if' block should be the same.
-
Change "free(new);" to "FcValueListDestroy(new);"
-
Now FcValueSave() is beside the "new->value = value;" line, it might as well be directly assigned: "new->value = FcValueSave(value);". Then because of (2), "bail2:" is no longer needed because the FcValueSave() result is either in 'new' or hasn't happened, and if it's in 'new' then FcValueListDestroy() will deal with it.
-
I think there is scope to move the FcPatternObjectInsertElt() call to before new->value initialization, also, though the benefit is lower. Either way, it should "goto bail1" now.