After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 756766 - Leak fixes
Leak fixes
Status: RESOLVED FIXED
Product: libsecret
Classification: Other
Component: General
unspecified
Other All
: Normal normal
: ---
Assigned To: libsecret maintainer(s)
libsecret maintainer(s)
: 736738 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-10-17 21:40 UTC by Christophe Fergeau
Modified: 2017-09-15 09:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
build-sys: Update valgrind headers (436.81 KB, patch)
2015-10-17 21:40 UTC, Christophe Fergeau
none Details | Review
secret-item: Fix 'convenince' typo (814 bytes, patch)
2015-10-17 21:40 UTC, Christophe Fergeau
committed Details | Review
Fix 'attributes' typo in API doc (982 bytes, patch)
2015-10-17 21:40 UTC, Christophe Fergeau
committed Details | Review
test-item: Fix GError leak (2.77 KB, patch)
2015-10-17 21:40 UTC, Christophe Fergeau
committed Details | Review
test-item: Fix GHashTable leak (1.60 KB, patch)
2015-10-17 21:40 UTC, Christophe Fergeau
committed Details | Review
test-item: Fix string leaks (3.29 KB, patch)
2015-10-17 21:40 UTC, Christophe Fergeau
committed Details | Review
item: Free SecreItem::value in finalize() (2.92 KB, patch)
2015-10-17 21:40 UTC, Christophe Fergeau
committed Details | Review
item: Fix local variable leak in on_create_path() (2.33 KB, patch)
2015-10-17 21:40 UTC, Christophe Fergeau
committed Details | Review
Fix secret_schema_unref detection of 0 refcount (1.28 KB, patch)
2015-10-17 21:40 UTC, Christophe Fergeau
none Details | Review
test-prompt: Fix GError leak (1008 bytes, patch)
2015-10-17 21:41 UTC, Christophe Fergeau
committed Details | Review
prompt: Don't leak 'owner_name' in secret_prompt_perform() (2.87 KB, patch)
2015-10-17 21:41 UTC, Christophe Fergeau
committed Details | Review
test-collection: Fix GError leaks (2.86 KB, patch)
2015-10-17 21:41 UTC, Christophe Fergeau
committed Details | Review
test-collection: Fix string leaks (3.54 KB, patch)
2015-10-17 21:41 UTC, Christophe Fergeau
committed Details | Review
collection: Fix on_create_path() leak (2.42 KB, patch)
2015-10-17 21:41 UTC, Christophe Fergeau
committed Details | Review
Don't leak ItemClosure::collection_path (2.34 KB, patch)
2015-10-17 21:41 UTC, Christophe Fergeau
committed Details | Review
Free CollectionClosure::collection_path when it's freed (875 bytes, patch)
2015-10-17 21:41 UTC, Christophe Fergeau
committed Details | Review
secret-sync: Fix SecretSync leak (1.70 KB, patch)
2015-10-17 21:41 UTC, Christophe Fergeau
committed Details | Review
build-sys: Update valgrind headers (437.36 KB, patch)
2015-10-19 10:12 UTC, Christophe Fergeau
committed Details | Review
Fix secret_schema_unref detection of 0 refcount (1.55 KB, patch)
2015-10-19 10:12 UTC, Christophe Fergeau
committed Details | Review

Description Christophe Fergeau 2015-10-17 21:40:14 UTC
Here is a patch series fixing various leaks I found running valgrind on the
test suite.
Comment 1 Christophe Fergeau 2015-10-17 21:40:22 UTC
Created attachment 313575 [details] [review]
build-sys: Update valgrind headers

libsecret ships headers from valgrind, but they seem to come from an old
version. Update to the headers from valgrind-3.11.0-1.fc23.x86_64
in order to get VG_DO_ADDED_LEAK_CHECK definition.
Comment 2 Christophe Fergeau 2015-10-17 21:40:26 UTC
Created attachment 313576 [details] [review]
secret-item: Fix 'convenince' typo
Comment 3 Christophe Fergeau 2015-10-17 21:40:31 UTC
Created attachment 313577 [details] [review]
Fix 'attributes' typo in API doc
Comment 4 Christophe Fergeau 2015-10-17 21:40:35 UTC
Created attachment 313578 [details] [review]
test-item: Fix GError leak

This fixes:
==16901== 94 (16 direct, 78 indirect) bytes in 1 blocks are definitely lost in loss record 901 of 1,154
==16901==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==16901==    by 0x36E564F679: g_malloc (gmem.c:97)
==16901==    by 0x36E5666CD2: g_slice_alloc (gslice.c:1007)
==16901==    by 0x36E563522B: g_error_new_valist (gerror.c:386)
==16901==    by 0x36E563560A: g_set_error (gerror.c:560)
==16901==    by 0x4C2CFB2: secret_item_initable_init (secret-item.c:480)
==16901==    by 0x36E6E5D83E: g_initable_new_valist (ginitable.c:228)
==16901==    by 0x36E6E5D8F5: g_initable_new (ginitable.c:146)
==16901==    by 0x4C3E711: secret_item_new_for_dbus_path_sync (secret-paths.c:286)
==16901==    by 0x402506: test_new_sync_noexist (test-item.c:121)
==16901==    by 0x36E566FB92: test_case_run (gtestutils.c:2124)
==16901==    by 0x36E566FB92: g_test_run_suite_internal (gtestutils.c:2185)
==16901==    by 0x36E566FD5A: g_test_run_suite_internal (gtestutils.c:2196)
==16901==    by 0x36E56700DA: g_test_run_suite (gtestutils.c:2249)
==16901==    by 0x36E5670110: g_test_run (gtestutils.c:1553)
==16901==    by 0x4C5A0EA: egg_tests_run_with_loop (egg-testing.c:167)
==16901==    by 0x406AAE: main (test-item.c:880)
Comment 5 Christophe Fergeau 2015-10-17 21:40:40 UTC
Created attachment 313579 [details] [review]
test-item: Fix GHashTable leak

This fixes:
==16901== 248 (88 direct, 160 indirect) bytes in 1 blocks are definitely lost in loss record 1,108 of 1,154
==16901==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==16901==    by 0x36E564F679: g_malloc (gmem.c:97)
==16901==    by 0x36E5666CD2: g_slice_alloc (gslice.c:1007)
==16901==    by 0x36E563860D: g_hash_table_new_full (ghash.c:711)
==16901==    by 0x4047B2: test_set_attributes_async (test-item.c:493)
==16901==    by 0x36E566FB92: test_case_run (gtestutils.c:2124)
==16901==    by 0x36E566FB92: g_test_run_suite_internal (gtestutils.c:2185)
==16901==    by 0x36E566FD5A: g_test_run_suite_internal (gtestutils.c:2196)
==16901==    by 0x36E56700DA: g_test_run_suite (gtestutils.c:2249)
==16901==    by 0x36E5670110: g_test_run (gtestutils.c:1553)
==16901==    by 0x4C5A0EA: egg_tests_run_with_loop (egg-testing.c:167)
==16901==    by 0x406AAE: main (test-item.c:880)
Comment 6 Christophe Fergeau 2015-10-17 21:40:45 UTC
Created attachment 313580 [details] [review]
test-item: Fix string leaks

This fixes:

==16901== 7 bytes in 1 blocks are definitely lost in loss record 9 of 1,154
==16901==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==16901==    by 0x36E564F679: g_malloc (gmem.c:97)
==16901==    by 0x36E56688AE: g_strdup (gstrfuncs.c:356)
==16901==    by 0x4C3098F: secret_item_get_label (secret-item.c:1922)
==16901==    by 0x402A65: test_create_sync (test-item.c:203)
==16901==    by 0x36E566FB92: test_case_run (gtestutils.c:2124)
==16901==    by 0x36E566FB92: g_test_run_suite_internal (gtestutils.c:2185)
==16901==    by 0x36E566FD5A: g_test_run_suite_internal (gtestutils.c:2196)
==16901==    by 0x36E56700DA: g_test_run_suite (gtestutils.c:2249)
==16901==    by 0x36E5670110: g_test_run (gtestutils.c:1553)
==16901==    by 0x4C5A0EA: egg_tests_run_with_loop (egg-testing.c:167)
==16901==    by 0x406AAE: main (test-item.c:880)
==16901==-
==16901== 7 bytes in 1 blocks are definitely lost in loss record 10 of 1,154
==16901==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==16901==    by 0x36E564F679: g_malloc (gmem.c:97)
==16901==    by 0x36E56688AE: g_strdup (gstrfuncs.c:356)
==16901==    by 0x4C3098F: secret_item_get_label (secret-item.c:1922)
==16901==    by 0x402DB6: test_create_async (test-item.c:249)
==16901==    by 0x36E566FB92: test_case_run (gtestutils.c:2124)
==16901==    by 0x36E566FB92: g_test_run_suite_internal (gtestutils.c:2185)
==16901==    by 0x36E566FD5A: g_test_run_suite_internal (gtestutils.c:2196)
==16901==    by 0x36E56700DA: g_test_run_suite (gtestutils.c:2249)
==16901==    by 0x36E5670110: g_test_run (gtestutils.c:1553)
==16901==    by 0x4C5A0EA: egg_tests_run_with_loop (egg-testing.c:167)
==16901==    by 0x406AAE: main (test-item.c:880)
==16901==-
Comment 7 Christophe Fergeau 2015-10-17 21:40:50 UTC
Created attachment 313581 [details] [review]
item: Free SecreItem::value in finalize()

This fixes:
==20768== 67 (40 direct, 27 indirect) bytes in 1 blocks are definitely lost in loss record 1,133 of 1,588
==20768==    at 0x4C28C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==20768==    by 0x6002FCC: g_malloc (gmem.c:94)
==20768==    by 0x601B523: g_slice_alloc (gslice.c:1007)
==20768==    by 0x601B563: g_slice_alloc0 (gslice.c:1032)
==20768==    by 0x4E5FB64: secret_value_new_full (secret-value.c:140)
==20768==    by 0x4E67B63: service_decode_aes_secret (secret-session.c:457)
==20768==    by 0x4E67D6B: _secret_session_decode_secret (secret-session.c:513)
==20768==    by 0x4E50A61: on_item_load_secret (secret-item.c:1157)
==20768==    by 0x5A3346D: g_task_return_now (gtask.c:1104)
==20768==    by 0x5A33575: g_task_return (gtask.c:1162)
==20768==    by 0x5A33E76: g_task_return_pointer (gtask.c:1537)
==20768==    by 0x5AAC7E6: reply_cb (gdbusproxy.c:2579)
==20768==    by 0x5A3346D: g_task_return_now (gtask.c:1104)
==20768==    by 0x5A33575: g_task_return (gtask.c:1162)
==20768==    by 0x5A33E76: g_task_return_pointer (gtask.c:1537)
==20768==    by 0x5A9BB7D: g_dbus_connection_call_done (gdbusconnection.c:5704)
==20768==    by 0x5A3346D: g_task_return_now (gtask.c:1104)
==20768==    by 0x5A334B6: complete_in_idle_cb (gtask.c:1118)
==20768==    by 0x5FFD3D0: g_idle_dispatch (gmain.c:5441)
==20768==    by 0x5FFAA18: g_main_dispatch (gmain.c:3154)
==20768==    by 0x5FFB85C: g_main_context_dispatch (gmain.c:3769)
==20768==    by 0x5FFBA40: g_main_context_iterate (gmain.c:3840)
==20768==    by 0x5FFBE66: g_main_loop_run (gmain.c:4034)
==20768==    by 0x4E510DA: secret_item_load_secret_sync (secret-item.c:1311)
==20768==    by 0x405238: test_load_secret_sync (test-item.c:592)
==20768==    by 0x60258FA: test_case_run (gtestutils.c:2158)
==20768==    by 0x6025CBB: g_test_run_suite_internal (gtestutils.c:2241)
==20768==    by 0x6025D64: g_test_run_suite_internal (gtestutils.c:2253)
==20768==    by 0x6025F7B: g_test_run_suite (gtestutils.c:2328)
==20768==    by 0x6024C1C: g_test_run (gtestutils.c:1596)
==20768==    by 0x4E7CA25: egg_tests_run_with_loop (egg-testing.c:167)
==20768==    by 0x406B52: main (test-item.c:887)
Comment 8 Christophe Fergeau 2015-10-17 21:40:55 UTC
Created attachment 313582 [details] [review]
item: Fix local variable leak in on_create_path()

This fixes:
==22678== 48 bytes in 1 blocks are definitely lost in loss record 1,045 of 1,560
==22678==    at 0x4C28C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==22678==    by 0x6002FCC: g_malloc (gmem.c:94)
==22678==    by 0x60032AE: g_malloc_n (gmem.c:330)
==22678==    by 0x601DBF0: g_strdup (gstrfuncs.c:363)
==22678==    by 0x4E6508B: on_create_item_called (secret-paths.c:1991)
==22678==    by 0x5A3346D: g_task_return_now (gtask.c:1104)
==22678==    by 0x5A33575: g_task_return (gtask.c:1162)
==22678==    by 0x5A33E76: g_task_return_pointer (gtask.c:1537)
==22678==    by 0x5A9BB7D: g_dbus_connection_call_done (gdbusconnection.c:5704)
==22678==    by 0x5A3346D: g_task_return_now (gtask.c:1104)
==22678==    by 0x5A334B6: complete_in_idle_cb (gtask.c:1118)
==22678==    by 0x5FFD3D0: g_idle_dispatch (gmain.c:5441)
==22678==    by 0x5FFAA18: g_main_dispatch (gmain.c:3154)
==22678==    by 0x5FFB85C: g_main_context_dispatch (gmain.c:3769)
==22678==    by 0x5FFBA40: g_main_context_iterate (gmain.c:3840)
==22678==    by 0x5FFBE66: g_main_loop_run (gmain.c:4034)
==22678==    by 0x4E7C9E4: loop_wait_until (egg-testing.c:151)
==22678==    by 0x4E7C85C: egg_test_wait_until (egg-testing.c:105)
==22678==    by 0x402D22: test_create_async (test-item.c:245)
==22678==    by 0x60258FA: test_case_run (gtestutils.c:2158)
==22678==    by 0x6025CBB: g_test_run_suite_internal (gtestutils.c:2241)
==22678==    by 0x6025D64: g_test_run_suite_internal (gtestutils.c:2253)
==22678==    by 0x6025F7B: g_test_run_suite (gtestutils.c:2328)
==22678==    by 0x6024C1C: g_test_run (gtestutils.c:1596)
==22678==    by 0x4E7CA3E: egg_tests_run_with_loop (egg-testing.c:167)
==22678==    by 0x406B52: main (test-item.c:887)
=
Comment 9 Christophe Fergeau 2015-10-17 21:40:59 UTC
Created attachment 313583 [details] [review]
Fix secret_schema_unref detection of 0 refcount

g_atomic_int_add (&schema->refs, -1); will return the value of 'refs'
_before_ adding -1 to it, so checking this value for 0 to see if the
refcount dropped to 0 after adding -1 is not going to work and will
cause a leak.
Using g_atomic_int_dec_and_test() fixes this problem as this will return
TRUE when the value drops to 0 after being decremented.
Comment 10 Christophe Fergeau 2015-10-17 21:41:03 UTC
Created attachment 313584 [details] [review]
test-prompt: Fix GError leak
Comment 11 Christophe Fergeau 2015-10-17 21:41:08 UTC
Created attachment 313585 [details] [review]
prompt: Don't leak 'owner_name' in secret_prompt_perform()

This fixes:
==16832== 7 bytes in 1 blocks are definitely lost in loss record 10 of 1,184
==16832==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==16832==    by 0x36E564F679: g_malloc (gmem.c:97)
==16832==    by 0x36E56688AE: g_strdup (gstrfuncs.c:356)
==16832==    by 0x36E6EDEB98: g_dbus_proxy_get_name_owner (gdbusproxy.c:2372)
==16832==    by 0x4C38B64: secret_prompt_perform (secret-prompt.c:468)
==16832==    by 0x4C39F4C: secret_service_real_prompt_async (secret-service.c:339)
==16832==    by 0x4C3D126: secret_service_prompt (secret-service.c:1711)
==16832==    by 0x4C42182: on_create_collection_called (secret-paths.c:1706)
==16832==    by 0x36E6E74BA6: g_simple_async_result_complete (gsimpleasyncresult.c:763)
==16832==    by 0x36E6ED1921: g_dbus_connection_call_done (gdbusconnection.c:5508)
==16832==    by 0x36E6E74BA6: g_simple_async_result_complete (gsimpleasyncresult.c:763)
==16832==    by 0x36E6E74C08: complete_in_idle_cb (gsimpleasyncresult.c:775)
==16832==    by 0x36E5649A89: g_main_dispatch (gmain.c:3122)
==16832==    by 0x36E5649A89: g_main_context_dispatch (gmain.c:3737)
==16832==    by 0x36E5649E1F: g_main_context_iterate.isra.29 (gmain.c:3808)
==16832==    by 0x36E564A141: g_main_loop_run (gmain.c:4002)
==16832==    by 0x4C427F1: secret_service_create_collection_dbus_path_sync (secret-paths.c:1908)
==16832==    by 0x4C299F9: secret_collection_create_sync (secret-collection.c:1211)
==16832==    by 0x403084: test_create_sync (test-collection.c:299)
==16832==    by 0x36E566FB92: test_case_run (gtestutils.c:2124)
==16832==    by 0x36E566FB92: g_test_run_suite_internal (gtestutils.c:2185)
==16832==    by 0x36E566FD5A: g_test_run_suite_internal (gtestutils.c:2196)
==16832==    by 0x36E56700DA: g_test_run_suite (gtestutils.c:2249)
==16832==    by 0x36E5670110: g_test_run (gtestutils.c:1553)
==16832==    by 0x4C5A0EA: egg_tests_run_with_loop (egg-testing.c:167)
==16832==    by 0x4068B0: main (test-collection.c:1028)
Comment 12 Christophe Fergeau 2015-10-17 21:41:13 UTC
Created attachment 313586 [details] [review]
test-collection: Fix GError leaks

This fixes (among others):
==16832== 95 (16 direct, 79 indirect) bytes in 1 blocks are definitely lost in loss record 927 of 1,184
==16832==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==16832==    by 0x36E564F679: g_malloc (gmem.c:97)
==16832==    by 0x36E5666CD2: g_slice_alloc (gslice.c:1007)
==16832==    by 0x36E563522B: g_error_new_valist (gerror.c:386)
==16832==    by 0x36E563560A: g_set_error (gerror.c:560)
==16832==    by 0x4C27FB4: secret_collection_initable_init (secret-collection.c:543)
==16832==    by 0x36E6E5D83E: g_initable_new_valist (ginitable.c:228)
==16832==    by 0x36E6E5D8F5: g_initable_new (ginitable.c:146)
==16832==    by 0x4C3E1E3: secret_collection_new_for_dbus_path_sync (secret-paths.c:158)
==16832==    by 0x404733: test_delete_sync (test-collection.c:623)
==16832==    by 0x36E566FB92: test_case_run (gtestutils.c:2124)
==16832==    by 0x36E566FB92: g_test_run_suite_internal (gtestutils.c:2185)
==16832==    by 0x36E566FD5A: g_test_run_suite_internal (gtestutils.c:2196)
==16832==    by 0x36E56700DA: g_test_run_suite (gtestutils.c:2249)
==16832==    by 0x36E5670110: g_test_run (gtestutils.c:1553)
==16832==    by 0x4C5A0EA: egg_tests_run_with_loop (egg-testing.c:167)
==16832==    by 0x4068B0: main (test-collection.c:1028)
Comment 13 Christophe Fergeau 2015-10-17 21:41:17 UTC
Created attachment 313587 [details] [review]
test-collection: Fix string leaks

This fixes:
==16832== 6 bytes in 1 blocks are definitely lost in loss record 8 of 1,184
==16832==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==16832==    by 0x36E564F679: g_malloc (gmem.c:97)
==16832==    by 0x36E56688AE: g_strdup (gstrfuncs.c:356)
==16832==    by 0x4C2B242: secret_collection_get_label (secret-collection.c:1835)
==16832==    by 0x40313D: test_create_sync (test-collection.c:305)
==16832==    by 0x36E566FB92: test_case_run (gtestutils.c:2124)
==16832==    by 0x36E566FB92: g_test_run_suite_internal (gtestutils.c:2185)
==16832==    by 0x36E566FD5A: g_test_run_suite_internal (gtestutils.c:2196)
==16832==    by 0x36E56700DA: g_test_run_suite (gtestutils.c:2249)
==16832==    by 0x36E5670110: g_test_run (gtestutils.c:1553)
==16832==    by 0x4C5A0EA: egg_tests_run_with_loop (egg-testing.c:167)
==16832==    by 0x4068B0: main (test-collection.c:1028)
==16832==-
==16832== 6 bytes in 1 blocks are definitely lost in loss record 9 of 1,184
==16832==    at 0x4A06C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==16832==    by 0x36E564F679: g_malloc (gmem.c:97)
==16832==    by 0x36E56688AE: g_strdup (gstrfuncs.c:356)
==16832==    by 0x4C2B242: secret_collection_get_label (secret-collection.c:1835)
==16832==    by 0x40337C: test_create_async (test-collection.c:333)
==16832==    by 0x36E566FB92: test_case_run (gtestutils.c:2124)
==16832==    by 0x36E566FB92: g_test_run_suite_internal (gtestutils.c:2185)
==16832==    by 0x36E566FD5A: g_test_run_suite_internal (gtestutils.c:2196)
==16832==    by 0x36E56700DA: g_test_run_suite (gtestutils.c:2249)
==16832==    by 0x36E5670110: g_test_run (gtestutils.c:1553)
==16832==    by 0x4C5A0EA: egg_tests_run_with_loop (egg-testing.c:167)
==16832==    by 0x4068B0: main (test-collection.c:1028)
Comment 14 Christophe Fergeau 2015-10-17 21:41:22 UTC
Created attachment 313588 [details] [review]
collection: Fix on_create_path() leak

This fixes:
==2111== 45 bytes in 1 blocks are definitely lost in loss record 1,180 of 1,795
==2111==    at 0x4C28C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==2111==    by 0x6002FCC: g_malloc (gmem.c:94)
==2111==    by 0x60032AE: g_malloc_n (gmem.c:330)
==2111==    by 0x601DBF0: g_strdup (gstrfuncs.c:363)
==2111==    by 0x603C073: g_variant_dup_string (gvariant.c:1503)
==2111==    by 0x4E644D5: on_create_collection_prompt (secret-paths.c:1682)
==2111==    by 0x5A1CB66: g_simple_async_result_complete (gsimpleasyncresult.c:801)
==2111==    by 0x4E5C12F: on_real_prompt_completed (secret-service.c:322)
==2111==    by 0x5A1CB66: g_simple_async_result_complete (gsimpleasyncresult.c:801)
==2111==    by 0x4E5A6A1: perform_prompt_complete (secret-prompt.c:288)
==2111==    by 0x4E5A7B5: on_prompt_completed (secret-prompt.c:314)
==2111==    by 0x5A97B7B: emit_signal_instance_in_idle_cb (gdbusconnection.c:3701)
==2111==    by 0x5FFD3D0: g_idle_dispatch (gmain.c:5441)
==2111==    by 0x5FFAA18: g_main_dispatch (gmain.c:3154)
==2111==    by 0x5FFB85C: g_main_context_dispatch (gmain.c:3769)
==2111==    by 0x5FFBA40: g_main_context_iterate (gmain.c:3840)
==2111==    by 0x5FFBE66: g_main_loop_run (gmain.c:4034)
==2111==    by 0x4E7C9E1: loop_wait_until (egg-testing.c:151)
==2111==    by 0x4E7C859: egg_test_wait_until (egg-testing.c:105)
==2111==    by 0x4032ED: test_create_async (test-collection.c:328)
==2111==    by 0x60258FA: test_case_run (gtestutils.c:2158)
==2111==    by 0x6025CBB: g_test_run_suite_internal (gtestutils.c:2241)
==2111==    by 0x6025D64: g_test_run_suite_internal (gtestutils.c:2253)
==2111==    by 0x6025F7B: g_test_run_suite (gtestutils.c:2328)
==2111==    by 0x6024C1C: g_test_run (gtestutils.c:1596)
==2111==    by 0x4E7CA3B: egg_tests_run_with_loop (egg-testing.c:167)
==2111==    by 0x406948: main (test-collection.c:1033)
Comment 15 Christophe Fergeau 2015-10-17 21:41:27 UTC
Created attachment 313589 [details] [review]
Don't leak ItemClosure::collection_path

This fixes:
==17724== 48 bytes in 1 blocks are definitely lost in loss record 1,135 of 1,698
==17724==    at 0x4C28C50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==17724==    by 0x6002FCC: g_malloc (gmem.c:94)
==17724==    by 0x60032AE: g_malloc_n (gmem.c:330)
==17724==    by 0x601DBF0: g_strdup (gstrfuncs.c:363)
==17724==    by 0x4E650B5: on_create_item_called (secret-paths.c:1990)
==17724==    by 0x5A3346D: g_task_return_now (gtask.c:1104)
==17724==    by 0x5A33575: g_task_return (gtask.c:1162)
==17724==    by 0x5A33E76: g_task_return_pointer (gtask.c:1537)
==17724==    by 0x5A9BB7D: g_dbus_connection_call_done (gdbusconnection.c:5704)
==17724==    by 0x5A3346D: g_task_return_now (gtask.c:1104)
==17724==    by 0x5A334B6: complete_in_idle_cb (gtask.c:1118)
==17724==    by 0x5FFD3D0: g_idle_dispatch (gmain.c:5441)
==17724==    by 0x5FFAA18: g_main_dispatch (gmain.c:3154)
==17724==    by 0x5FFB85C: g_main_context_dispatch (gmain.c:3769)
==17724==    by 0x5FFBA40: g_main_context_iterate (gmain.c:3840)
==17724==    by 0x5FFBE66: g_main_loop_run (gmain.c:4034)
==17724==    by 0x4E55FF7: secret_service_store_sync (secret-methods.c:1281)
==17724==    by 0x404AC8: test_store_sync (test-methods.c:736)
==17724==    by 0x60258FA: test_case_run (gtestutils.c:2158)
==17724==    by 0x6025CBB: g_test_run_suite_internal (gtestutils.c:2241)
==17724==    by 0x6025D64: g_test_run_suite_internal (gtestutils.c:2253)
==17724==    by 0x6025F7B: g_test_run_suite (gtestutils.c:2328)
==17724==    by 0x6024C1C: g_test_run (gtestutils.c:1596)
==17724==    by 0x4E7CA68: egg_tests_run_with_loop (egg-testing.c:167)
==17724==    by 0x406190: main (test-methods.c:998)
Comment 16 Christophe Fergeau 2015-10-17 21:41:32 UTC
Created attachment 313590 [details] [review]
Free CollectionClosure::collection_path when it's freed

Similarly to what is done ItemClosure::collection_path, make sure that
CollectionClosure::collection_path is freed in
collection_closure_free().
Comment 17 Christophe Fergeau 2015-10-17 21:41:37 UTC
Created attachment 313591 [details] [review]
secret-sync: Fix SecretSync leak

The memory allocated for the SecretSync instance itself was not freed. This fixes:
==7106== 24 bytes in 1 blocks are definitely lost in loss record 855 of 1,629
==7106==    at 0x4C2A9C7: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==7106==    by 0x600303F: g_malloc0 (gmem.c:124)
==7106==    by 0x6003322: g_malloc0_n (gmem.c:354)
==7106==    by 0x4E6979D: _secret_sync_new (secret-util.c:441)
==7106==    by 0x4E65D11: secret_service_read_alias_dbus_path_sync (secret-paths.c:2334)
==7106==    by 0x4E4E09E: secret_collection_for_alias_sync (secret-collection.c:2221)
==7106==    by 0x40270E: test_for_alias_sync (test-collection.c:187)
==7106==    by 0x60258FA: test_case_run (gtestutils.c:2158)
==7106==    by 0x6025CBB: g_test_run_suite_internal (gtestutils.c:2241)
==7106==    by 0x6025D64: g_test_run_suite_internal (gtestutils.c:2253)
==7106==    by 0x6025F7B: g_test_run_suite (gtestutils.c:2328)
==7106==    by 0x6024C1C: g_test_run (gtestutils.c:1596)
==7106==    by 0x4E7CA3B: egg_tests_run_with_loop (egg-testing.c:167)
==7106==    by 0x406948: main (test-collection.c:1033)
Comment 18 Stef Walter 2015-10-19 08:41:05 UTC
Review of attachment 313575 [details] [review]:

This results in the following warnings:

In file included from ../egg/egg-secure-memory.c:45:0:
../egg/egg-secure-memory.c: In function ‘memcpy_with_vbits’:
../build/valgrind/memcheck.h:283:5: warning: value computed is not used [-Wunused-value]
     (unsigned)VALGRIND_DO_CLIENT_REQUEST_EXPR(0,                \
     ^
../egg/egg-secure-memory.c:658:3: note: in expansion of macro ‘VALGRIND_SET_VBITS’
   VALGRIND_SET_VBITS (dest, vbits, length);
   ^
../build/valgrind/memcheck.h:283:5: warning: value computed is not used [-Wunused-value]
     (unsigned)VALGRIND_DO_CLIENT_REQUEST_EXPR(0,                \
     ^
../egg/egg-secure-memory.c:659:3: note: in expansion of macro ‘VALGRIND_SET_VBITS’
   VALGRIND_SET_VBITS (src, vbits, length);
   ^

Once those are fixed, LGTM.
Comment 19 Stef Walter 2015-10-19 08:41:30 UTC
Review of attachment 313576 [details] [review]:

Looks good to me.
Comment 20 Stef Walter 2015-10-19 08:41:50 UTC
Review of attachment 313577 [details] [review]:

Looks good to me.
Comment 21 Stef Walter 2015-10-19 08:42:26 UTC
Review of attachment 313578 [details] [review]:

Looks good to me.
Comment 22 Stef Walter 2015-10-19 08:42:40 UTC
Review of attachment 313579 [details] [review]:

Looks good to me.
Comment 23 Stef Walter 2015-10-19 08:43:08 UTC
Review of attachment 313580 [details] [review]:

Looks good to me.
Comment 24 Stef Walter 2015-10-19 08:44:24 UTC
Review of attachment 313581 [details] [review]:

Looks good to me.
Comment 25 Stef Walter 2015-10-19 08:44:56 UTC
Review of attachment 313582 [details] [review]:

Looks good to me.
Comment 26 Stef Walter 2015-10-19 08:46:22 UTC
Review of attachment 313583 [details] [review]:

This patch removes a programmer error diagnostic. We should produce a precondition style
g_warning() in the case where the caller has passed in a statically allocated or invalid
SecretSchema structure.
Comment 27 Stef Walter 2015-10-19 08:46:35 UTC
Review of attachment 313584 [details] [review]:

Looks good to me.
Comment 28 Stef Walter 2015-10-19 08:47:34 UTC
Review of attachment 313585 [details] [review]:

Looks good to me.
Comment 29 Stef Walter 2015-10-19 08:48:00 UTC
Review of attachment 313586 [details] [review]:

Looks good to me, but could you add a space after the function call before the parentheses.
Comment 30 Stef Walter 2015-10-19 08:48:17 UTC
Review of attachment 313587 [details] [review]:

Looks good to me.
Comment 31 Stef Walter 2015-10-19 08:48:55 UTC
Review of attachment 313588 [details] [review]:

Looks good to me.
Comment 32 Stef Walter 2015-10-19 08:49:47 UTC
Review of attachment 313589 [details] [review]:

Looks good to me.
Comment 33 Stef Walter 2015-10-19 08:50:01 UTC
Review of attachment 313590 [details] [review]:

Looks good to me.
Comment 34 Stef Walter 2015-10-19 08:50:30 UTC
Review of attachment 313591 [details] [review]:

Looks good to me.
Comment 35 Stef Walter 2015-10-19 08:51:00 UTC
Thanks for the work on this. Wonderful :)
Comment 36 Christophe Fergeau 2015-10-19 10:12:48 UTC
Created attachment 313651 [details] [review]
build-sys: Update valgrind headers

libsecret ships headers from valgrind, but they seem to come from an old
version. Update to the headers from valgrind-3.11.0-1.fc23.x86_64
in order to get VG_DO_ADDED_LEAK_CHECK definition.
Comment 37 Christophe Fergeau 2015-10-19 10:12:55 UTC
Created attachment 313652 [details] [review]
Fix secret_schema_unref detection of 0 refcount

g_atomic_int_add (&schema->refs, -1); will return the value of 'refs'
_before_ adding -1 to it, so checking this value for 0 to see if the
refcount dropped to 0 after adding -1 is not going to work and will
cause a leak.
Using g_atomic_int_dec_and_test() fixes this problem as this will return
TRUE when the value drops to 0 after being decremented.
Comment 38 Stef Walter 2015-10-19 10:15:17 UTC
Review of attachment 313651 [details] [review]:

Thanks. Looks good to me.
Comment 39 Stef Walter 2015-10-19 10:15:51 UTC
Review of attachment 313652 [details] [review]:

Looks good. Thanks.
Comment 40 Philip Withnall 2017-09-15 09:52:53 UTC
*** Bug 736738 has been marked as a duplicate of this bug. ***