GNOME Bugzilla – Bug 768548
Cursor data is leaked when the favourites button is toggled
Last modified: 2016-07-11 08:31:47 UTC
Any action (eg., toggling the favourites button) that calls photos_base_item_refresh, and eventually photos_base_item_populate_from_cursor, on an existing BaseItem leaks all the existing data gathered from a TrackerSparqlCursor. We are not freeing the older values before updating them. Valgrind says: 1 bytes in 1 blocks are definitely lost in loss record 111 of 27,579 at 0x4C28BF6: malloc (vg_replace_malloc.c:299) by 0x956B528: g_malloc (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x958422E: g_strdup (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x434212: photos_base_item_populate_from_cursor (photos-base-item.c:1529) by 0x434870: photos_base_item_constructed (photos-base-item.c:1643) by 0x44FB6D: photos_local_item_constructed (photos-local-item.c:222) 1 bytes in 1 blocks are definitely lost in loss record 112 of 27,579 at 0x4C28BF6: malloc (vg_replace_malloc.c:299) by 0x956B528: g_malloc (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x958422E: g_strdup (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x434419: photos_base_item_populate_from_cursor (photos-base-item.c:1564) by 0x434870: photos_base_item_constructed (photos-base-item.c:1643) by 0x44FB6D: photos_local_item_constructed (photos-local-item.c:222) 11 bytes in 1 blocks are definitely lost in loss record 2,199 of 27,579 at 0x4C28BF6: malloc (vg_replace_malloc.c:299) by 0x956B528: g_malloc (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x958422E: g_strdup (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x4342F4: photos_base_item_populate_from_cursor (photos-base-item.c:1542) by 0x434870: photos_base_item_constructed (photos-base-item.c:1643) by 0x44FB6D: photos_local_item_constructed (photos-local-item.c:222) 11 bytes in 1 blocks are definitely lost in loss record 2,200 of 27,579 at 0x4C28BF6: malloc (vg_replace_malloc.c:299) by 0x956B528: g_malloc (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x95842A7: g_strndup (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x8FEDC08: ??? (in /usr/lib64/libgio-2.0.so.0.4600.2) by 0x956A084: g_markup_parse_context_parse (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x8FED435: ??? (in /usr/lib64/libgio-2.0.so.0.4600.2) by 0x8FEE59C: g_content_type_get_description (in /usr/lib64/libgio-2.0.so.0.4600.2) by 0x431757: photos_base_item_default_update_type_description (photos-base-item.c:479) by 0x43413B: photos_base_item_update_info_from_type (photos-base-item.c:1506) by 0x43433A: photos_base_item_populate_from_cursor (photos-base-item.c:1544) by 0x434870: photos_base_item_constructed (photos-base-item.c:1643) by 0x44FB6D: photos_local_item_constructed (photos-local-item.c:222) 13 bytes in 1 blocks are definitely lost in loss record 2,491 of 27,579 at 0x4C28BF6: malloc (vg_replace_malloc.c:299) by 0x956B528: g_malloc (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x958422E: g_strdup (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x434445: photos_base_item_populate_from_cursor (photos-base-item.c:1566) by 0x434870: photos_base_item_constructed (photos-base-item.c:1643) by 0x44FB6D: photos_local_item_constructed (photos-local-item.c:222) ... and so on.
We are also leaking a query string: 98 bytes in 1 blocks are definitely lost in loss record 24,071 of 29,264 at 0x4C2AB8B: realloc (vg_replace_malloc.c:785) by 0xA0E1B1C: __vasprintf_chk (in /usr/lib64/libc-2.22.so) by 0x95A9998: g_vasprintf (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x95843AC: g_strdup_vprintf (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x9584468: g_strdup_printf (in /usr/lib64/libglib-2.0.so.0.4600.2) by 0x480E93: photos_utils_set_favorite (photos-utils.c:1434) by 0x4318F4: photos_base_item_default_set_favorite (photos-base-item.c:443) by 0x4394E4: photos_base_item_set_favorite (photos-base-item.c:2796)
Created attachment 331056 [details] [review] base-item: Don't leak the cursor data while refreshing
Created attachment 331057 [details] [review] utils: Don't leak the query string
r+ on both
Review of attachment 331056 [details] [review]: ::: src/photos-base-item.c @@ +1541,3 @@ const gchar *uri; + photos_base_item_clear_cursor_data (self); The more I think about it, a function similar to g_set_object, but for C strings would be a better option here. That way the g_free and g_strdup will be tightly coupled in a single place, which is more readable and maintainable.
Created attachment 331123 [details] [review] Add photos_utils_set_string
Created attachment 331124 [details] [review] Add photos_utils_take_string
Created attachment 331125 [details] [review] base-item: Don't leak the cursor data while refreshing
(In reply to Debarshi Ray from comment #8) > Created attachment 331125 [details] [review] [review] > base-item: Don't leak the cursor data while refreshing Aren't you missing releasing the data on _finalize? You're freeing the strings on refresh, but you still should clear the data on base-item destruction, shouldn't you?
(In reply to Rafael Fonseca from comment #9) > (In reply to Debarshi Ray from comment #8) > > Created attachment 331125 [details] [review] [review] [review] > > base-item: Don't leak the cursor data while refreshing > > Aren't you missing releasing the data on _finalize? You're freeing the > strings on refresh, but you still should clear the data on base-item > destruction, shouldn't you? We were already releasing the strings in _finalize, no? I think we were only leaking them during refresh.
(In reply to Debarshi Ray from comment #10) > We were already releasing the strings in _finalize, no? I think we were only > leaking them during refresh. Oh, I see. I got confused by the previous version of this patch.
Created attachment 331185 [details] [review] Add photos_utils_set_string Made some improvements based on an implementation (https://paste.gnome.org/pndyzil5r) by Emmanuele Bassi: * a g_return_val_if_fail guard * a direct pointer comparison check
Created attachment 331186 [details] [review] Add photos_utils_take_string
Thanks for the reviews, Rafael!