Yesterday, while I was at university waiting for a seminar about network security, I was on #gnome-it, and pbor asked me to look at a leak in EggRecentModel. I began wading through the code, but I didn’t found the leakage in time before having to leave.
In the evening, after I got back home, Marta went to her church choir practice, and I looked at recent-files/egg-recent-model.c using Valgrind (and practicing with it, since I’m not that good with this tool); using the function chain reported by valgrind, I noticed that the leak was inside the logic used by egg_recent_model_filter. This function scans the list of EggRecentItem objects built from the on-disk storage and filter out using the three pre-defined filter functions (while my RecentManager object supports only custom filtering function, and the RecentChooser implementations might filter the recently used resources list using a more complex RecentFilter object). The problem is that it used the infamous GList scan using while():
while (list) {
SomeData *obj = l->data;
do something with the object
list = list->next;
}
This code is perfectly valid: it walks the list by exhausting it; that is: the list pointer reaches the end of the list once it reaches the end of the while. Valid code does not mean right code, though. Suppose you want to move the items of a list inside another list. By using the code above, you would have:
GList *newlist;
/* init the target list */
newlist = NULL;
while (list) {
SomeData *obj = l->data;
if (some_condition)
newlist = g_list_append (newlist, obj);
list = list->next;
}
This code is also perfectly valid for filtering a list. Suppose, now, that you want to destroy the original list and return the filtered copy; you’ll have to free the GList using g_list_free() at the end of the while cycle:
GList *newlist;
/* init the target list */
newlist = NULL;
while (list) {
SomeData *obj = l->data;
if (some_condition)
newlist = g_list_append (newlist, obj);
list = list->next;
}
g_list_free (list);
return newlist;
But we said above that the list is walked by exhausting it; hence, the list pointer is now NULL (the exit condition checked by the while cycle). Thus you are leaking the old list; to avoid this leakage, you must walk the list keeping a pointer to the list start. In order to do so, the right thing to do is to use an iterator pointer, and a for() cycle to keep the cycle compact and avoid messing up with the list = list->next assignments on every cycle break condition:
GList *newlist, *l;
for (l = list, newlist = NULL; l; l = l->next) {
SomeData *obj = l->data;
if (some_condition)
newlist = g_list_append (newlist, obj);
}
g_list_free (list);
return newlist;
This code does not leak the old list, but it leaks the data that was not moved to the new list; if the function to free the data is called some_list_free, the code above becomes:
GList *newlist, *l;
for (l = list, newlist = NULL; l; l = l->next) {
SomeData *obj = l->data;
if (some_condition)
newlist = g_list_append (newlist, obj);
else
some_data_free (obj);
}
g_list_free (list);
return newlist;
As a performance sidenote: if the list is long, you might consider using g_list_prepend, since it’s a constant time operation, while g_list_append walks the list; just remember to return the reversed new list:
GList *newlist, *l;
for (l = list, newlist = NULL; l; l = l->next) {
SomeData *obj = l->data;
if (some_condition)
newlist = g_list_prepend (newlist, obj);
else
some_data_free (obj);
}
g_list_free (list);
return g_list_reverse (newlist);
I had noticed the leak around 10:30pm and began fixing it, but I had to pick up Marta and both returned home at 11:30pm, when we went straight to bed and fell asleep almost immediately. This morning, I found bug #323002 awaiting, with a fix for the first part of the leak (thanks to Paolo). Now, libegg/recent-files HEAD has the leak fully plugged. Subsequent runs of valgrind showed no mor leaks related to the recent-files.
Now, I’m going to check my libegg/recentchooser and my libegg/bookmarkfile code for leaks.
[hacking]
[coding+style]
[memory+leaks]
