GNOME Bugzilla – Bug 738508
Leak fixes
Last modified: 2019-02-22 11:58:54 UTC
I've run gcr/gnome-keyring test programs through valgrind ( for f in test*; do if test -x $f; then G_SLICE=always-malloc libtool --mode=execute valgrind --num-callers=40 --leak-check=full --log-file=./vg.new/$f.log ./$f; fi; done ) and then I've fixed all the leaks I could find. Some of the egg/ fixes in gnome-keyring/egg/ also need to be applied to gcr/egg/
Created attachment 288443 [details] [review] Fix various leaks in GcrSystemPrompt
Created attachment 288444 [details] [review] Fix various leaks in test-system-prompt
Created attachment 288445 [details] [review] Free GcrPkcs11Importer::queue in dispose() Its content is freed by emptying it, but the main data structure must be freed too with g_queue_free().
Created attachment 288446 [details] [review] gcr-pkcs11-importer: Unref GcrImporterData::importer 'importer' must be unref'ed when no longer used otherwise it will be leaked.
Created attachment 288447 [details] [review] system-prompt: Fix GVariant leak g_variant_get_variant() is (transfer full) so we need to release the reference we get when it's no longer needed. This fixes leak reports from valgrind similar to: ==19651== 168 (80 direct, 88 indirect) bytes in 2 blocks are definitely lost in loss record 1,137 of 1,207 ==19651== at 0x4A06BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==19651== by 0x5DE1887: g_malloc (gmem.c:97) ==19651== by 0x5DFA5B5: g_slice_alloc (gslice.c:1007) ==19651== by 0x5E21B14: g_variant_alloc (gvariant-core.c:476) ==19651== by 0x5E21BA4: g_variant_new_from_bytes (gvariant-core.c:512) ==19651== by 0x5E194F4: g_variant_new_from_trusted (gvariant.c:295) ==19651== by 0x5E19785: g_variant_new_int32 (gvariant.c:459) ==19651== by 0x5431FDC: parse_value_from_blob (gdbusmessage.c:1512) ==19651== by 0x5432A7C: parse_value_from_blob (gdbusmessage.c:1850) ==19651== by 0x5432A7C: parse_value_from_blob (gdbusmessage.c:1850) ==19651== by 0x54327E4: parse_value_from_blob (gdbusmessage.c:1770) ==19651== by 0x543267D: parse_value_from_blob (gdbusmessage.c:1723) ==19651== by 0x54328EB: parse_value_from_blob (gdbusmessage.c:1804) ==19651== by 0x543322E: g_dbus_message_new_from_blob (gdbusmessage.c:2146) ==19651== by 0x543F246: _g_dbus_worker_do_read_cb (gdbusprivate.c:750) ==19651== by 0x53B22C6: g_simple_async_result_complete (gsimpleasyncresult.c:763) ==19651== by 0x53B22F8: complete_in_idle_cb (gsimpleasyncresult.c:775) ==19651== by 0x5DDBC89: g_idle_dispatch (gmain.c:5367) ==19651== by 0x5DD9392: g_main_dispatch (gmain.c:3111) ==19651== by 0x5DDA118: g_main_context_dispatch (gmain.c:3710) ==19651== by 0x5DDA30A: g_main_context_iterate (gmain.c:3781) ==19651== by 0x5DDA731: g_main_loop_run (gmain.c:3975) ==19651== by 0x543E6E8: gdbus_shared_thread_func (gdbusprivate.c:273) ==19651== by 0x5E07B1C: g_thread_proxy (gthread.c:764) ==19651== by 0x3899207529: start_thread (pthread_create.c:310) ==19651== by 0x3898F0077C: clone (clone.S:109)
Created attachment 288448 [details] [review] Free GcrParser::filename in ::finalize()
Created attachment 288449 [details] [review] Don't leak GcrParsing in gcr_parser_parse_stream()
Created attachment 288450 [details] [review] test-certificate-chain: Add missing cleanup in teardown() This was reported as memory leaks by valgrind.
Created attachment 288451 [details] [review] system-prompter: Add missing word to debug messages
Created attachment 288452 [details] [review] ssh-agent: Fix leak in remove_by_public_key() gck_attributes_find_string() returns a newly allocated string, so it must be g_free'ed after use.
Created attachment 288453 [details] [review] ssh-agent: Fix leak in search_keys_like_attributes() gck_enumerator_next() return value must be unref'fed as it's transfer full.
Created attachment 288454 [details] [review] ssh-agent: Fix leak in op_request_identities The object returned by gck_enumerator_next() must be unref'ed.
Created attachment 288455 [details] [review] ssh-agent: Fix leak in op_v1_request_identities The object returned by gck_enumerator_next() must be unref'ed as this method is "transfer full".
Created attachment 288456 [details] [review] asn1x: Sanitize use of asn1_set_value/asn1_take_value Most callers of asn1_set_value() seems to assume this function will take ownership of the passed in GBytes, while it actually takes an additional reference.
Created attachment 288457 [details] [review] Unref GkmCredential::secret in ::dispose This fixes a memory leak.
Created attachment 288458 [details] [review] gkm-gnome2-file: Fix leak in validate_buffer() 'check' is allocated in this function but never freed.
Created attachment 288459 [details] [review] gkm-gnome2-file: Fix leaks in create_cipher() 'key' and 'iv' were allocated before calling egg_symkey_generate_simple() but this function allocates the memory needed for the 'key' and 'iv' return value, so the memory which was allocated in create_cipher() is lost and leaked. This also uses egg_secure_memory_free() to free 'key' memory as egg_symkey_generate_simple() allocates it with egg_secure_alloc().
Created attachment 288460 [details] [review] Free GkmSecretItem::schema in ::finalize()
Created attachment 288461 [details] [review] test-dbus-items: Fix memory leak
Created attachment 288462 [details] [review] egg-cleanup: Don't leak 'cleanup' on unregister When unregistering a cleanup instance, the memory allocated for the cleanup was not freed causing a memory leak.
Created attachment 288463 [details] [review] egg/test-dn: Don't leak GBytes created in test_dn_value()
Created attachment 288464 [details] [review] gkm-data-asn1: Unref buffer we got from egg_asn1x_get_integer_as_raw() Otherwise we will be leaking a reference to that buffer, and it will never get destroyed. This showed up as a memory leaked in test-data-der.
Created attachment 288465 [details] [review] test-data-asn1: Fix memory leaks in test_asn1_integers
Created attachment 288466 [details] [review] test-data-der: Fix various memory leaks
Created attachment 288467 [details] [review] gkm-gnome2-file: Free keys to 'entries' hash tables The keys to the 'publics' and 'privates' hash tables are strdup'ed strings, but these strings are never freed. This commit adds a free function for hash table keys to the g_hash_table_new_full call.
Created attachment 288468 [details] [review] test-startup: Use g_strfreev to free GStrv variable gkd_test_launch_daemon returns a "transfer full" GStrv, so it must be freed with g_strfreev after use.
Created attachment 288469 [details] [review] test-asn1: Don't leak 'asn' in test_create_quark()
Created attachment 288470 [details] [review] test-padding: Don't leak egg_padding_pkcs1_pad_02 return value egg_padding_pkcs1_pad_02 returns newly allocated data which must be g_free'ed after use.
Created attachment 288471 [details] [review] test-pam: Fix GError leak in error case When g_file_get_contents() fails and sets an error, it must be freed.
Created attachment 288472 [details] [review] test-secret: Don't leak new secrets in test_equal() Secrets created with gkm_secret_new* must be unref'fed when no longer used.
Created attachment 288473 [details] [review] test-sexp: Fix 2 leaks gcry_sexp_t objects created with gkm_sexp_parse_key() must be freed with gcry_sexp_release(), and gcry_mpi_t objects created with gkm_sexp_extract_mpi() must be freed with gcry_mpi_release() when no longer used.
Created attachment 288474 [details] [review] egg-asn1x: Fix memory leak in egg_asn1x_set_any_raw() In error cases, the Atlv variable 'tlv' which was created in this function is not going to be used, so we must free it before returning.
Created attachment 288475 [details] [review] egg-hkdf: Fix gcry_md_ht_t leak in egg_hkdf_perform The 'md2' gcry_md_ht_t variable is opened with gcry_md_open() in that method but is never closed, which causes a leak.
Created attachment 288476 [details] [review] test-import: Don't leak args.pReserved It's allocated with g_strdup_printf and must thus be freed after use.
Created attachment 288477 [details] [review] secret-store/test*: Don't leak secret data memory Both test-secret-binary and test-secret-textual call g_file_get_contents() but never free the returned data, causing a memory leak.
Created attachment 288478 [details] [review] gkm-gnome2-storage: Unref GkmGnome2Storage::login in dispose()
Created attachment 288479 [details] [review] gkm-secret-search: Fix leak in factory_create_search() gkm_attribute_get_string() returns a newly allocated string which must be freed when no longer useful.
Created attachment 288480 [details] [review] gkm-secret-textual: Fix leak in generate_attribute() The GList returned by gkm_secret_fields_get_names() must be freed with g_list_free after use as this is created using g_hash_table_get_keys().
Created attachment 288481 [details] [review] Fix leak in mock_secret_C_CreateObject 'template' is created with gkm_template_new() so it must be freed using gkm_template_free().
Created attachment 288482 [details] [review] gkm-xdg-assertion: Fix leak in factory_create_assertion() 'purpose' and 'peer' were allocated through gkm_attributes_find_string() and must thus be freed after use.
Created attachment 288483 [details] [review] Don't leak password in login_prompt_do_{specific, user) login_prompt_do_specific() and login_prompt_do_user() both set GkmWrapPrompt::prompt_data to either memory which must be freed with egg_secure_memory_strfree (through a call to auto_unlock_lookup_*) or to const memory which must not be freed (through a call to gkm_wrap_prompt_request_password). These methods currently assume the password memory does not need to be freed, which leads to this leak in test-login-auto (line numbers from 3.13.91-2-g45bb5be): ==2190== 5 bytes in 1 blocks are definitely lost in loss record 17 of 1,294 ==2190== at 0x40F8E4: egg_secure_alloc_full (egg-secure-memory.c:1056) ==2190== by 0x417F5E: egg_secure_alloc (gkm-wrap-login.c:42) ==2190== by 0x419157: gkm_wrap_login_lookup_secret (gkm-wrap-login.c:409) ==2190== by 0x407E8D: auto_unlock_lookup_object (gkm-wrap-prompt.c:198) ==2190== by 0x40B9B0: login_prompt_do_specific (gkm-wrap-prompt.c:1453) ==2190== by 0x40C13A: gkm_wrap_prompt_do_login (gkm-wrap-prompt.c:1591) ==2190== by 0x406384: auth_C_Login (gkm-wrap-layer.c:706) ==2190== by 0x40472A: test_specific (test-login-auto.c:156) ==2190== by 0x5E0A27A: test_case_run (gtestutils.c:2059) ==2190== by 0x5E0A602: g_test_run_suite_internal (gtestutils.c:2120) ==2190== by 0x5E0A6C4: g_test_run_suite_internal (gtestutils.c:2131) ==2190== by 0x5E0A6C4: g_test_run_suite_internal (gtestutils.c:2131) ==2190== by 0x5E0A847: g_test_run_suite (gtestutils.c:2184) ==2190== by 0x5E09551: g_test_run (gtestutils.c:1488) ==2190== by 0x410851: testing_thread (egg-testing.c:142) ==2190== by 0x5E0D2F4: g_thread_proxy (gthread.c:764) ==2190== by 0x3B7AE07F34: start_thread (pthread_create.c:309) ==2190== by 0x3B7AAF4C3C: clone (clone.S:111)
Created attachment 288484 [details] [review] Don't leak password data in gkm_wrap_prompt_do_credential Memory returned by auto_unlock_lookup_object() must be freed while memory returned by gkm_wrap_prompt_request_password() must not be freed. Depending on the situation, CredentialPrompt::password will contain one or the other, and currently this field is never freed, causing leaks when the password comes from auto_unlock_lookup_object(). This commit will always free CredentialPrompt::password when it's no longer needed, and will create a copy of the returned string when gkm_wrap_prompt_request_password() is called. This fixes (line numbers from 3.13.91-2-g45bb5be): ==2190== 8 bytes in 1 blocks are definitely lost in loss record 58 of 1,294 ==2190== at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==2190== by 0x5DE6DE6: g_malloc (gmem.c:97) ==2190== by 0x5E024B5: g_memdup (gstrfuncs.c:384) ==2190== by 0x41296C: gkm_template_set (gkm-attributes.c:600) ==2190== by 0x4129F0: gkm_template_set_value (gkm-attributes.c:614) ==2190== by 0x419BCD: mock_secret_C_CreateObject (mock-secret-store.c:174) ==2190== by 0x40646E: wrap_C_CreateObject (gkm-wrap-layer.c:741) ==2190== by 0x418985: find_login_keyring_item (gkm-wrap-login.c:254) ==2190== by 0x4190AF: gkm_wrap_login_lookup_secret (gkm-wrap-login.c:396) ==2190== by 0x407E8D: auto_unlock_lookup_object (gkm-wrap-prompt.c:198) ==2190== by 0x40B9B0: login_prompt_do_specific (gkm-wrap-prompt.c:1453) ==2190== by 0x40C13A: gkm_wrap_prompt_do_login (gkm-wrap-prompt.c:1591) ==2190== by 0x406384: auth_C_Login (gkm-wrap-layer.c:706) ==2190== by 0x40472A: test_specific (test-login-auto.c:156) ==2190== by 0x5E0A27A: test_case_run (gtestutils.c:2059) ==2190== by 0x5E0A602: g_test_run_suite_internal (gtestutils.c:2120) ==2190== by 0x5E0A6C4: g_test_run_suite_internal (gtestutils.c:2131) ==2190== by 0x5E0A6C4: g_test_run_suite_internal (gtestutils.c:2131) ==2190== by 0x5E0A847: g_test_run_suite (gtestutils.c:2184) ==2190== by 0x5E09551: g_test_run (gtestutils.c:1488) ==2190== by 0x410851: testing_thread (egg-testing.c:142) ==2190== by 0x5E0D2F4: g_thread_proxy (gthread.c:764) ==2190== by 0x3B7AE07F34: start_thread (pthread_create.c:309) ==2190== by 0x3B7AAF4C3C: clone (clone.S:111)
Created attachment 288485 [details] [review] Fix leak in gkm_mock_C_SetPIN()
Created attachment 288486 [details] [review] xdg: Don't leak ref in lookup_or_create_assertion_key() When lookup_or_create_assertion_key() creates a new assertion key, it own a reference on the GBytes created by create_assertion_key() and it will then take an extra ref on it when associating it with a GkmAssertion instance. This will cause a leak of the memory returned by create_assertion_key() as one of these references will never be dropped.
Created attachment 288487 [details] [review] test-xdg-module: Fix memory leak Memory from g_file_get_contents() was never freed.
Created attachment 288488 [details] [review] Unref GkmXdgTrust::bytes in finalize() This will cause leaks otherwise, for example in test-xdg-module.
Created attachment 288489 [details] [review] test-xdg-trust: Fix GChecksum leaks 'md' is created using g_checksum_new() so it must be destroyed with g_checksum_free() after use.
Created attachment 288490 [details] [review] xdg: Remove wrong unref in gkm_xdg_trust_replace_assertion Callers of lookup_or_create_assertion_key() don't own a reference on the returned GBytes so it should not be unref'fed before exiting gkm_xdg_trust_replace_assertion. This solves the following errors from valgrind when running test-xdg-trust ==15477== ==15477== Thread 2 testing: ==15477== Invalid read of size 4 ==15477== at 0x4EB6B75: g_bytes_unref (gbytes.c:306) ==15477== by 0x4EC2692: g_datalist_clear (gdataset.c:273) ==15477== by 0x4C284A3: g_object_finalize (gobject.c:1033) ==15477== by 0x42B35E: gkm_object_finalize (gkm-object.c:448) ==15477== by 0x414134: gkm_assertion_finalize (gkm-assertion.c:134) ==15477== by 0x4C2D256: g_object_unref (gobject.c:3170) ==15477== by 0x43B9C7: gkm_util_dispose_unref (gkm-util.c:137) ==15477== by 0x4ECEBD1: g_hash_table_remove_all_nodes (ghash.c:503) ==15477== by 0x4ECFC02: g_hash_table_remove_all (ghash.c:1371) ==15477== by 0x431DBB: gkm_session_dispose (gkm-session.c:409) ==15477== by 0x4C2D131: g_object_unref (gobject.c:3133) ==15477== by 0x425391: apartment_free (gkm-module.c:241) ==15477== by 0x4ECEBD1: g_hash_table_remove_all_nodes (ghash.c:503) ==15477== by 0x4ECFC02: g_hash_table_remove_all (ghash.c:1371) ==15477== by 0x4267F9: gkm_module_dispose (gkm-module.c:633) ==15477== by 0x4107D3: gkm_xdg_module_dispose (gkm-xdg-module.c:553) ==15477== by 0x4C28648: g_object_run_dispose (gobject.c:1076) ==15477== by 0x40CCDD: gkm_C_Finalize (gkm-module-ep.h:102) ==15477== by 0x40C7F0: mock_xdg_module_leave_and_finalize (mock-xdg-module.c:151) ==15477== by 0x408585: teardown (test-xdg-trust.c:138) ==15477== by 0x4F0E2FE: test_case_run (gtestutils.c:2069) ==15477== by 0x4F0E602: g_test_run_suite_internal (gtestutils.c:2120) ==15477== by 0x4F0E6C4: g_test_run_suite_internal (gtestutils.c:2131) ==15477== by 0x4F0E6C4: g_test_run_suite_internal (gtestutils.c:2131) ==15477== by 0x4F0E847: g_test_run_suite (gtestutils.c:2184) ==15477== by 0x4F0D551: g_test_run (gtestutils.c:1488) ==15477== by 0x4516B9: testing_thread (egg-testing.c:142) ==15477== by 0x4F112F4: g_thread_proxy (gthread.c:764) ==15477== by 0x3B7AE07F34: start_thread (pthread_create.c:309) ==15477== by 0x3B7AAF4C3C: clone (clone.S:111) ==15477== Address 0x52b6100 is 16 bytes inside a block of size 40 free'd ==15477== at 0x4A07577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==15477== by 0x4EEAF5F: g_free (gmem.c:190) ==15477== by 0x4F03C59: g_slice_free1 (gslice.c:1112) ==15477== by 0x4EB6BB8: g_bytes_unref (gbytes.c:310) ==15477== by 0x4ECEBB3: g_hash_table_remove_all_nodes (ghash.c:500) ==15477== by 0x4ECFC02: g_hash_table_remove_all (ghash.c:1371) ==15477== by 0x4ECF7C4: g_hash_table_destroy (ghash.c:1067) ==15477== by 0x4127E9: gkm_xdg_trust_finalize (gkm-xdg-trust.c:706) ==15477== by 0x4C2D256: g_object_unref (gobject.c:3170) ==15477== by 0x43B9C7: gkm_util_dispose_unref (gkm-util.c:137) ==15477== by 0x4ECEBD1: g_hash_table_remove_all_nodes (ghash.c:503) ==15477== by 0x4ECFC02: g_hash_table_remove_all (ghash.c:1371) ==15477== by 0x431DBB: gkm_session_dispose (gkm-session.c:409) ==15477== by 0x4C2D131: g_object_unref (gobject.c:3133) ==15477== by 0x425391: apartment_free (gkm-module.c:241) ==15477== by 0x4ECEBD1: g_hash_table_remove_all_nodes (ghash.c:503) ==15477== by 0x4ECFC02: g_hash_table_remove_all (ghash.c:1371) ==15477== by 0x4267F9: gkm_module_dispose (gkm-module.c:633) ==15477== by 0x4107D3: gkm_xdg_module_dispose (gkm-xdg-module.c:553) ==15477== by 0x4C28648: g_object_run_dispose (gobject.c:1076) ==15477== by 0x40CCDD: gkm_C_Finalize (gkm-module-ep.h:102) ==15477== by 0x40C7F0: mock_xdg_module_leave_and_finalize (mock-xdg-module.c:151) ==15477== by 0x408585: teardown (test-xdg-trust.c:138) ==15477== by 0x4F0E2FE: test_case_run (gtestutils.c:2069) ==15477== by 0x4F0E602: g_test_run_suite_internal (gtestutils.c:2120) ==15477== by 0x4F0E6C4: g_test_run_suite_internal (gtestutils.c:2131) ==15477== by 0x4F0E6C4: g_test_run_suite_internal (gtestutils.c:2131) ==15477== by 0x4F0E847: g_test_run_suite (gtestutils.c:2184) ==15477== by 0x4F0D551: g_test_run (gtestutils.c:1488) ==15477== by 0x4516B9: testing_thread (egg-testing.c:142) ==15477== by 0x4F112F4: g_thread_proxy (gthread.c:764) ==15477== by 0x3B7AE07F34: start_thread (pthread_create.c:309) ==15477== by 0x3B7AAF4C3C: clone (clone.S:111)
Created attachment 288491 [details] [review] xdg: Fix ref leak in remove_assertion_from_trust() When a transaction is used, remove_assertion_from_trust() will steal the assertion and its key from the 'assertions' hash table. The stolen assertion will then be added to the transaction and be unreferenced later, but the key must be unref'ed as the hash table owned a reference on it. This fixes: ==9337== 104 (40 direct, 64 indirect) bytes in 1 blocks are definitely lost in loss record 614 of 678 ==9337== at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==9337== by 0x4EEADE6: g_malloc (gmem.c:97) ==9337== by 0x4F03A22: g_slice_alloc (gslice.c:1007) ==9337== by 0x4EB6954: g_bytes_new_with_free_func (gbytes.c:183) ==9337== by 0x4EB68D5: g_bytes_new_take (gbytes.c:126) ==9337== by 0x4116D4: create_assertion_key (gkm-xdg-trust.c:355) ==9337== by 0x411755: lookup_or_create_assertion_key (gkm-xdg-trust.c:371) ==9337== by 0x41312C: gkm_xdg_trust_replace_assertion (gkm-xdg-trust.c:874) ==9337== by 0x413B62: factory_create_assertion (gkm-xdg-assertion.c:181) ==9337== by 0x4338A3: gkm_session_create_object_for_factory (gkm-session.c:778) ==9337== by 0x433A5B: gkm_session_create_object_for_attributes (gkm-session.c:820) ==9337== by 0x434222: gkm_session_C_CreateObject (gkm-session.c:954) ==9337== by 0x40A1D0: test_create_assertion_twice (test-xdg-trust.c:497) ==9337== by 0x4F0E27A: test_case_run (gtestutils.c:2059) ==9337== by 0x4F0E602: g_test_run_suite_internal (gtestutils.c:2120) ==9337== by 0x4F0E6C4: g_test_run_suite_internal (gtestutils.c:2131) ==9337== by 0x4F0E6C4: g_test_run_suite_internal (gtestutils.c:2131) ==9337== by 0x4F0E847: g_test_run_suite (gtestutils.c:2184) ==9337== by 0x4F0D551: g_test_run (gtestutils.c:1488) ==9337== by 0x4516AD: testing_thread (egg-testing.c:142) ==9337== by 0x4F112F4: g_thread_proxy (gthread.c:764) ==9337== by 0x3B7AE07F34: start_thread (pthread_create.c:309) ==9337== by 0x3B7AAF4C3C: clone (clone.S:111)
Created attachment 288492 [details] [review] test-spawn: Fix leaks of EchoData content EchoData::error and EchoData::output must be freed after use.
Created attachment 288493 [details] [review] GkmMock: Fix handling of CKA_G_CREDENTIAL_TEMPLATE attributes These are special as their value is an array of CK_ATTRIBUTE pointing to allocated memory. Moreover, when gkm_mock_C_SetAttributeValue() is called, this memory is owned by the caller, so it needs to be duplicated as the caller may free it before GkmMock no longer needs it. We also need to make sure this memory we just duplicated is correctly freed when no longer needed. This is achieved by introducing an additional global variable, the_credential_template. This is similar to how this type of attributes is handled in GkmSecretCollection. Without this, running test-login-auto in valgrind causes invalid reads: ==5954== Invalid read of size 1 ==5954== at 0x4123D4: gkm_attributes_find_boolean (gkm-attributes.c:503) ==5954== by 0x408C00: set_unlock_options_on_prompt (gkm-wrap-prompt.c:520) ==5954== by 0x40A3E8: gkm_wrap_prompt_do_credential (gkm-wrap-prompt.c:1055) ==5954== by 0x40641C: auth_C_CreateObject (gkm-wrap-layer.c:764) ==5954== by 0x404D7B: test_unlock_keyring (test-login-auto.c:243) ==5954== by 0x5E04A8B: test_case_run (gtestutils.c:2059) ==5954== by 0x5E04E2D: g_test_run_suite_internal (gtestutils.c:2120) ==5954== by 0x5E04EEF: g_test_run_suite_internal (gtestutils.c:2131) ==5954== by 0x5E04EEF: g_test_run_suite_internal (gtestutils.c:2131) ==5954== by 0x5E0506F: g_test_run_suite (gtestutils.c:2184) ==5954== by 0x5E03D5C: g_test_run (gtestutils.c:1488) ==5954== by 0x410725: testing_thread (egg-testing.c:142) ==5954== by 0x5E07B29: g_thread_proxy (gthread.c:764) ==5954== by 0x3899207529: start_thread (pthread_create.c:310) ==5954== by 0x3898F0077C: clone (clone.S:109) ==5954== Address 0x85bad90 is 0 bytes inside a block of size 1 free'd ==5954== at 0x4A07CE9: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==5954== by 0x5DE1A0D: g_free (gmem.c:190) ==5954== by 0x409E54: gkm_wrap_prompt_finalize (gkm-wrap-prompt.c:933) ==5954== by 0x590293A: g_object_unref (gobject.c:3170) ==5954== by 0x40644A: auth_C_CreateObject (gkm-wrap-layer.c:771) ==5954== by 0x404CD5: test_unlock_keyring (test-login-auto.c:234) ==5954== by 0x5E04A8B: test_case_run (gtestutils.c:2059) ==5954== by 0x5E04E2D: g_test_run_suite_internal (gtestutils.c:2120) ==5954== by 0x5E04EEF: g_test_run_suite_internal (gtestutils.c:2131) ==5954== by 0x5E04EEF: g_test_run_suite_internal (gtestutils.c:2131) ==5954== by 0x5E0506F: g_test_run_suite (gtestutils.c:2184) ==5954== by 0x5E03D5C: g_test_run (gtestutils.c:1488) ==5954== by 0x410725: testing_thread (egg-testing.c:142) ==5954== by 0x5E07B29: g_thread_proxy (gthread.c:764) ==5954== by 0x3899207529: start_thread (pthread_create.c:310) ==5954== by 0x3898F0077C: clone (clone.S:109)
Created attachment 288508 [details] [review] Free GkdSecretService::alias_directory in finalize() This fixes a memory leak reported by valgrind on daemon shutdown (tested by starting a new daemon instance with --replace).
Created attachment 288509 [details] [review] GkdGpgAgent: Unref GIOChannel when no longer needed This fixes a memory leak reported by valgrind on daemon shutdown (tested by starting a new daemon instance with --replace).
Awesome. Will work through these.
Comment on attachment 288443 [details] [review] Fix various leaks in GcrSystemPrompt Attachment 288443 [details] pushed as fd8d55a - Fix various leaks in GcrSystemPrompt
Attachment 288444 [details] pushed as 223c049 - Fix various leaks in test-system-prompt Attachment 288445 [details] pushed as e3a02d3 - Free GcrPkcs11Importer::queue in dispose() Attachment 288446 [details] pushed as b66f3eb - gcr-pkcs11-importer: Unref GcrImporterData::importer Attachment 288447 [details] pushed as fb049a0 - system-prompt: Fix GVariant leak Attachment 288448 [details] pushed as be1ae52 - Free GcrParser::filename in ::finalize() Attachment 288449 [details] pushed as 7a602ee - Don't leak GcrParsing in gcr_parser_parse_stream() Attachment 288450 [details] pushed as a130fee - test-certificate-chain: Add missing cleanup in teardown() Attachment 288451 [details] pushed as 5b54fc5 - system-prompter: Add missing word to debug messages
Comment on attachment 288456 [details] [review] asn1x: Sanitize use of asn1_set_value/asn1_take_value Attachment 288456 [details] pushed as 1034361 - asn1x: Sanitize use of asn1_set_value/asn1_take_value
Attachment 288452 [details] pushed as 04b9de9 - ssh-agent: Fix leak in remove_by_public_key() Attachment 288453 [details] pushed as d177a8b - ssh-agent: Fix leak in search_keys_like_attributes()
Attachment 288454 [details] pushed as 9bfe737 - ssh-agent: Fix leak in op_request_identities Attachment 288455 [details] pushed as f81fa15 - ssh-agent: Fix leak in op_v1_request_identities
Review of attachment 288445 [details] [review]: dispose can get run multiple times. This will cause a double free. Accidentally committed this, will commit a follow up.
Attachment 288457 [details] pushed as 24ff22e - Unref GkmCredential::secret in ::dispose Attachment 288458 [details] pushed as 5a98866 - gkm-gnome2-file: Fix leak in validate_buffer() Attachment 288459 [details] pushed as 04129e8 - gkm-gnome2-file: Fix leaks in create_cipher() Attachment 288460 [details] pushed as 0c9c4a5 - Free GkmSecretItem::schema in ::finalize() Attachment 288461 [details] pushed as 9af2db3 - test-dbus-items: Fix memory leak Attachment 288462 [details] pushed as 8ea8329 - egg-cleanup: Don't leak 'cleanup' on unregister Attachment 288463 [details] pushed as 326a945 - egg/test-dn: Don't leak GBytes created in test_dn_value() Attachment 288464 [details] pushed as c5083ba - gkm-data-asn1: Unref buffer we got from egg_asn1x_get_integer_as_raw() Attachment 288465 [details] pushed as 298d910 - test-data-asn1: Fix memory leaks in test_asn1_integers Attachment 288466 [details] pushed as 0fc832a - test-data-der: Fix various memory leaks Attachment 288467 [details] pushed as 6c2d07a - gkm-gnome2-file: Free keys to 'entries' hash tables Attachment 288468 [details] pushed as 1e18f34 - test-startup: Use g_strfreev to free GStrv variable Attachment 288469 [details] pushed as 610a177 - test-asn1: Don't leak 'asn' in test_create_quark() Attachment 288470 [details] pushed as dbc6af0 - test-padding: Don't leak egg_padding_pkcs1_pad_02 return value Attachment 288471 [details] pushed as 75c1fb8 - test-pam: Fix GError leak in error case Attachment 288472 [details] pushed as 33793e5 - test-secret: Don't leak new secrets in test_equal() Attachment 288473 [details] pushed as 17b429d - test-sexp: Fix 2 leaks Attachment 288474 [details] pushed as 1581f4a - egg-asn1x: Fix memory leak in egg_asn1x_set_any_raw() Attachment 288475 [details] pushed as c2870de - egg-hkdf: Fix gcry_md_ht_t leak in egg_hkdf_perform Attachment 288476 [details] pushed as 9421bf5 - test-import: Don't leak args.pReserved Attachment 288477 [details] pushed as bf96fea - secret-store/test*: Don't leak secret data memory Attachment 288478 [details] pushed as a198426 - gkm-gnome2-storage: Unref GkmGnome2Storage::login in dispose() Attachment 288479 [details] pushed as 750047b - gkm-secret-search: Fix leak in factory_create_search() Attachment 288480 [details] pushed as 165f0e3 - gkm-secret-textual: Fix leak in generate_attribute() Attachment 288481 [details] pushed as ca2b739 - Fix leak in mock_secret_C_CreateObject Attachment 288482 [details] pushed as 670fe30 - gkm-xdg-assertion: Fix leak in factory_create_assertion() Attachment 288485 [details] pushed as bc17247 - Fix leak in gkm_mock_C_SetPIN() Attachment 288486 [details] pushed as bbb9683 - xdg: Don't leak ref in lookup_or_create_assertion_key() Attachment 288487 [details] pushed as 621a86e - test-xdg-module: Fix memory leak Attachment 288488 [details] pushed as a88f4fc - Unref GkmXdgTrust::bytes in finalize() Attachment 288489 [details] pushed as f2e7599 - test-xdg-trust: Fix GChecksum leaks Attachment 288490 [details] pushed as 37c493c - xdg: Remove wrong unref in gkm_xdg_trust_replace_assertion Attachment 288491 [details] pushed as 0db2253 - xdg: Fix ref leak in remove_assertion_from_trust() Attachment 288492 [details] pushed as 87f82c2 - test-spawn: Fix leaks of EchoData content Attachment 288493 [details] pushed as ddcdccb - GkmMock: Fix handling of CKA_G_CREDENTIAL_TEMPLATE attributes Attachment 288508 [details] pushed as 3db6f3b - Free GkdSecretService::alias_directory in finalize() Attachment 288509 [details] pushed as 39aeef6 - GkdGpgAgent: Unref GIOChannel when no longer needed
Thanks for the great work. The remaining patches cause the following compile warnings, and need fixing: ../pkcs11/wrap-layer/gkm-wrap-prompt.c: In function 'credential_prompt_free': ../pkcs11/wrap-layer/gkm-wrap-prompt.c:961:22: error: passing argument 1 of 'egg_secure_strfree' discards 'const' qualifier from pointer target type [-Werror] egg_secure_strfree (data->password); ^ In file included from ../pkcs11/wrap-layer/gkm-wrap-prompt.c:27:0: ../egg/egg-secure-memory.h:110:8: note: expected 'char *' but argument is of type 'const gchar *' void egg_secure_strfree (char *str); ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c: In function 'gkm_wrap_prompt_do_credential': ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1037:22: error: passing argument 1 of 'egg_secure_strfree' discards 'const' qualifier from pointer target type [-Werror] egg_secure_strfree (data->password); ^ In file included from ../pkcs11/wrap-layer/gkm-wrap-prompt.c:27:0: ../egg/egg-secure-memory.h:110:8: note: expected 'char *' but argument is of type 'const gchar *' void egg_secure_strfree (char *str); ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c: In function 'gkm_wrap_prompt_do_init_pin': ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1212:41: error: passing argument 2 of 'gkm_wrap_prompt_set_prompt_data' discards 'const' qualifier from pointer target type [-Werror] gkm_wrap_prompt_set_prompt_data (self, password, NULL); ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:916:1: note: expected 'gpointer' but argument is of type 'const gchar *' gkm_wrap_prompt_set_prompt_data (GkmWrapPrompt *self, ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c: In function 'login_prompt_do_specific': ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1476:46: error: passing argument 2 of 'gkm_wrap_prompt_set_prompt_data' discards 'const' qualifier from pointer target type [-Werror] gkm_wrap_prompt_set_prompt_data (self, password, egg_secure_strfree); ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:916:1: note: expected 'gpointer' but argument is of type 'const gchar *' gkm_wrap_prompt_set_prompt_data (GkmWrapPrompt *self, ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1476:56: error: passing argument 3 of 'gkm_wrap_prompt_set_prompt_data' from incompatible pointer type [-Werror] gkm_wrap_prompt_set_prompt_data (self, password, egg_secure_strfree); ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:916:1: note: expected 'GDestroyNotify' but argument is of type 'void (*)(char *)' gkm_wrap_prompt_set_prompt_data (GkmWrapPrompt *self, ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1488:42: error: passing argument 2 of 'gkm_wrap_prompt_set_prompt_data' discards 'const' qualifier from pointer target type [-Werror] gkm_wrap_prompt_set_prompt_data (self, password, NULL); ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:916:1: note: expected 'gpointer' but argument is of type 'const gchar *' gkm_wrap_prompt_set_prompt_data (GkmWrapPrompt *self, ^ In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9:0, from /usr/include/glib-2.0/glib/gtypes.h:32, from /usr/include/glib-2.0/glib/galloca.h:32, from /usr/include/glib-2.0/glib.h:30, from ../pkcs11/wrap-layer/gkm-wrap-login.h:24, from ../pkcs11/wrap-layer/gkm-wrap-prompt.c:23: ../pkcs11/wrap-layer/gkm-wrap-prompt.c: In function 'login_prompt_done_specific': ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1504:24: error: comparison of distinct pointer types lacks a cast [-Werror] self->destroy_data == egg_secure_strfree); ^ /usr/include/glib-2.0/glib/gmacros.h:318:25: note: in definition of macro 'G_LIKELY' #define G_LIKELY(expr) (expr) ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1503:2: note: in expansion of macro 'g_assert' g_assert (self->destroy_data == NULL || ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c: In function 'login_prompt_do_user': ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1551:43: error: passing argument 2 of 'gkm_wrap_prompt_set_prompt_data' discards 'const' qualifier from pointer target type [-Werror] gkm_wrap_prompt_set_prompt_data (self, password, egg_secure_strfree); ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:916:1: note: expected 'gpointer' but argument is of type 'const gchar *' gkm_wrap_prompt_set_prompt_data (GkmWrapPrompt *self, ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1551:53: error: passing argument 3 of 'gkm_wrap_prompt_set_prompt_data' from incompatible pointer type [-Werror] gkm_wrap_prompt_set_prompt_data (self, password, egg_secure_strfree); ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:916:1: note: expected 'GDestroyNotify' but argument is of type 'void (*)(char *)' gkm_wrap_prompt_set_prompt_data (GkmWrapPrompt *self, ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1563:42: error: passing argument 2 of 'gkm_wrap_prompt_set_prompt_data' discards 'const' qualifier from pointer target type [-Werror] gkm_wrap_prompt_set_prompt_data (self, password, NULL); ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:916:1: note: expected 'gpointer' but argument is of type 'const gchar *' gkm_wrap_prompt_set_prompt_data (GkmWrapPrompt *self, ^ In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9:0, from /usr/include/glib-2.0/glib/gtypes.h:32, from /usr/include/glib-2.0/glib/galloca.h:32, from /usr/include/glib-2.0/glib.h:30, from ../pkcs11/wrap-layer/gkm-wrap-login.h:24, from ../pkcs11/wrap-layer/gkm-wrap-prompt.c:23: ../pkcs11/wrap-layer/gkm-wrap-prompt.c: In function 'login_prompt_done_user': ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1578:24: error: comparison of distinct pointer types lacks a cast [-Werror] self->destroy_data == egg_secure_strfree); ^ /usr/include/glib-2.0/glib/gmacros.h:318:25: note: in definition of macro 'G_LIKELY' #define G_LIKELY(expr) (expr) ^ ../pkcs11/wrap-layer/gkm-wrap-prompt.c:1577:2: note: in expansion of macro 'g_assert' g_assert (self->destroy_data == NULL || ^ cc1: all warnings being treated as errors Makefile:5015: recipe for target 'pkcs11/wrap-layer/libgkm_wrap_layer_la-gkm-wrap-prompt.lo' failed make[2]: *** [pkcs11/wrap-layer/libgkm_wrap_layer_la-gkm-wrap-prompt.lo] Error 1
Ah right, I knew I still had something to fix before sending the patches, but could not remember what /o\ Will fix it, thanks for the quick review on the other patches!
Created attachment 288587 [details] [review] Don't leak password in login_prompt_do_{specific, user) login_prompt_do_specific() and login_prompt_do_user() both set GkmWrapPrompt::prompt_data to either memory which must be freed with egg_secure_memory_strfree (through a call to auto_unlock_lookup_*) or to const memory which must not be freed (through a call to gkm_wrap_prompt_request_password). These methods currently assume the password memory does not need to be freed, which leads to this leak in test-login-auto (line numbers from 3.13.91-2-g45bb5be): ==2190== 5 bytes in 1 blocks are definitely lost in loss record 17 of 1,294 ==2190== at 0x40F8E4: egg_secure_alloc_full (egg-secure-memory.c:1056) ==2190== by 0x417F5E: egg_secure_alloc (gkm-wrap-login.c:42) ==2190== by 0x419157: gkm_wrap_login_lookup_secret (gkm-wrap-login.c:409) ==2190== by 0x407E8D: auto_unlock_lookup_object (gkm-wrap-prompt.c:198) ==2190== by 0x40B9B0: login_prompt_do_specific (gkm-wrap-prompt.c:1453) ==2190== by 0x40C13A: gkm_wrap_prompt_do_login (gkm-wrap-prompt.c:1591) ==2190== by 0x406384: auth_C_Login (gkm-wrap-layer.c:706) ==2190== by 0x40472A: test_specific (test-login-auto.c:156) ==2190== by 0x5E0A27A: test_case_run (gtestutils.c:2059) ==2190== by 0x5E0A602: g_test_run_suite_internal (gtestutils.c:2120) ==2190== by 0x5E0A6C4: g_test_run_suite_internal (gtestutils.c:2131) ==2190== by 0x5E0A6C4: g_test_run_suite_internal (gtestutils.c:2131) ==2190== by 0x5E0A847: g_test_run_suite (gtestutils.c:2184) ==2190== by 0x5E09551: g_test_run (gtestutils.c:1488) ==2190== by 0x410851: testing_thread (egg-testing.c:142) ==2190== by 0x5E0D2F4: g_thread_proxy (gthread.c:764) ==2190== by 0x3B7AE07F34: start_thread (pthread_create.c:309) ==2190== by 0x3B7AAF4C3C: clone (clone.S:111)
Created attachment 288588 [details] [review] Don't leak password data in gkm_wrap_prompt_do_credential Memory returned by auto_unlock_lookup_object() must be freed while memory returned by gkm_wrap_prompt_request_password() must not be freed. Depending on the situation, CredentialPrompt::password will contain one or the other, and currently this field is never freed, causing leaks when the password comes from auto_unlock_lookup_object(). This commit will always free CredentialPrompt::password when it's no longer needed, and will create a copy of the returned string when gkm_wrap_prompt_request_password() is called. This fixes (line numbers from 3.13.91-2-g45bb5be): ==2190== 8 bytes in 1 blocks are definitely lost in loss record 58 of 1,294 ==2190== at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==2190== by 0x5DE6DE6: g_malloc (gmem.c:97) ==2190== by 0x5E024B5: g_memdup (gstrfuncs.c:384) ==2190== by 0x41296C: gkm_template_set (gkm-attributes.c:600) ==2190== by 0x4129F0: gkm_template_set_value (gkm-attributes.c:614) ==2190== by 0x419BCD: mock_secret_C_CreateObject (mock-secret-store.c:174) ==2190== by 0x40646E: wrap_C_CreateObject (gkm-wrap-layer.c:741) ==2190== by 0x418985: find_login_keyring_item (gkm-wrap-login.c:254) ==2190== by 0x4190AF: gkm_wrap_login_lookup_secret (gkm-wrap-login.c:396) ==2190== by 0x407E8D: auto_unlock_lookup_object (gkm-wrap-prompt.c:198) ==2190== by 0x40B9B0: login_prompt_do_specific (gkm-wrap-prompt.c:1453) ==2190== by 0x40C13A: gkm_wrap_prompt_do_login (gkm-wrap-prompt.c:1591) ==2190== by 0x406384: auth_C_Login (gkm-wrap-layer.c:706) ==2190== by 0x40472A: test_specific (test-login-auto.c:156) ==2190== by 0x5E0A27A: test_case_run (gtestutils.c:2059) ==2190== by 0x5E0A602: g_test_run_suite_internal (gtestutils.c:2120) ==2190== by 0x5E0A6C4: g_test_run_suite_internal (gtestutils.c:2131) ==2190== by 0x5E0A6C4: g_test_run_suite_internal (gtestutils.c:2131) ==2190== by 0x5E0A847: g_test_run_suite (gtestutils.c:2184) ==2190== by 0x5E09551: g_test_run (gtestutils.c:1488) ==2190== by 0x410851: testing_thread (egg-testing.c:142) ==2190== by 0x5E0D2F4: g_thread_proxy (gthread.c:764) ==2190== by 0x3B7AE07F34: start_thread (pthread_create.c:309) ==2190== by 0x3B7AAF4C3C: clone (clone.S:111)
Created attachment 288589 [details] [review] daemon: Fix GStrv leak in DBus message handler
'asn1x: Sanitize use of asn1_set_value/asn1_take_value' (commit 1034361 in gcr git) needs to be applied to gnome-keyring as well. 'test-asn1: Don't leak 'asn' in test_create_quark()' 'egg/test-dn: Don't leak GBytes created in test_dn_value()' 'egg-asn1x: Fix memory leak in egg_asn1x_set_any_raw()' (and most likely all the other patches to gnome-keyring/egg/) should be applied to gcr. These 3 ones fix some leaks in gcr tests
Thanks a lot. This is really great work! Attachment 288587 [details] pushed as dc5a7dc - Don't leak password in login_prompt_do_{specific, user) Attachment 288588 [details] pushed as b087539 - Don't leak password data in gkm_wrap_prompt_do_credential Attachment 288589 [details] pushed as 6da13bb - daemon: Fix GStrv leak in DBus message handler