From 199162b11afe0e5275f62c65fc82a5bbcc6c9552 Mon Sep 17 00:00:00 2001 From: Michael Vetter Date: Thu, 4 Jul 2019 10:30:56 +0200 Subject: [PATCH 1/6] Add omemo_close function We call omemo_init() when starting profanity and should have an omemo_close() at exit. For now we free the fingerprint autocompleter in there. Fixes valgrind: ``` ==13226== 24 bytes in 1 blocks are definitely lost in loss record 2,855 of 6,958 ==13226== at 0x483677F: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==13226== by 0x48AD39: autocomplete_new (autocomplete.c:57) ==13226== by 0x4AB89F: omemo_init (omemo.c:127) ==13226== by 0x42C283: _init (profanity.c:206) ==13226== by 0x42BFF3: prof_run (profanity.c:98) ==13226== by 0x4B25E6: main (main.c:172) ``` Regards https://github.com/profanity-im/profanity/issues/1131 --- src/omemo/omemo.c | 9 +++++++++ src/omemo/omemo.h | 1 + src/profanity.c | 3 +++ 3 files changed, 13 insertions(+) diff --git a/src/omemo/omemo.c b/src/omemo/omemo.c index 96d2d65a..c813dcf0 100644 --- a/src/omemo/omemo.c +++ b/src/omemo/omemo.c @@ -127,6 +127,15 @@ omemo_init(void) omemo_ctx.fingerprint_ac = autocomplete_new(); } +void +omemo_close(void) +{ + if (omemo_ctx.fingerprint_ac) { + autocomplete_free(omemo_ctx.fingerprint_ac); + omemo_ctx.fingerprint_ac = NULL; + } +} + void omemo_on_connect(ProfAccount *account) { diff --git a/src/omemo/omemo.h b/src/omemo/omemo.h index ae25b5ba..abe21be5 100644 --- a/src/omemo/omemo.h +++ b/src/omemo/omemo.h @@ -56,6 +56,7 @@ typedef struct omemo_key { } omemo_key_t; void omemo_init(void); +void omemo_close(void); void omemo_on_connect(ProfAccount *account); void omemo_on_disconnect(void); void omemo_generate_crypto_materials(ProfAccount *account); diff --git a/src/profanity.c b/src/profanity.c index a9824729..324aa36d 100644 --- a/src/profanity.c +++ b/src/profanity.c @@ -241,6 +241,9 @@ _shutdown(void) #endif #ifdef HAVE_LIBGPGME p_gpg_close(); +#endif +#ifdef HAVE_OMEMO + omemo_close(); #endif chat_log_close(); theme_close(); From d0047c8376f661505e456db64e998a48328f00a5 Mon Sep 17 00:00:00 2001 From: Michael Vetter Date: Thu, 4 Jul 2019 10:38:24 +0200 Subject: [PATCH 2/6] Dont initialize omemo autocompleter twice We already do this in omemo_init() no need to do it again in omemo_on_connect(). --- src/omemo/omemo.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/omemo/omemo.c b/src/omemo/omemo.c index c813dcf0..e62aa795 100644 --- a/src/omemo/omemo.c +++ b/src/omemo/omemo.c @@ -226,8 +226,6 @@ omemo_on_connect(ProfAccount *account) omemo_ctx.device_list_handler = g_hash_table_new_full(g_str_hash, g_str_equal, free, NULL); omemo_ctx.known_devices = g_hash_table_new_full(g_str_hash, g_str_equal, free, (GDestroyNotify)_g_hash_table_free); - omemo_ctx.fingerprint_ac = autocomplete_new(); - char *omemodir = files_get_data_path(DIR_OMEMO); GString *basedir = g_string_new(omemodir); free(omemodir); From 0410802753ec49ecfd699d55c636d41b3d143e01 Mon Sep 17 00:00:00 2001 From: Michael Vetter Date: Thu, 4 Jul 2019 10:47:50 +0200 Subject: [PATCH 3/6] Free omemo_ctx.device_list_handler --- src/omemo/omemo.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/omemo/omemo.c b/src/omemo/omemo.c index e62aa795..02107432 100644 --- a/src/omemo/omemo.c +++ b/src/omemo/omemo.c @@ -305,6 +305,7 @@ omemo_on_disconnect(void) _g_hash_table_free(omemo_ctx.signed_pre_key_store); _g_hash_table_free(omemo_ctx.pre_key_store); + _g_hash_table_free(omemo_ctx.device_list_handler); g_string_free(omemo_ctx.identity_filename, TRUE); g_key_file_free(omemo_ctx.identity_keyfile); From e3443f5c9ae7c79b16d5afeeaf6dd8d5a00437b2 Mon Sep 17 00:00:00 2001 From: Michael Vetter Date: Thu, 4 Jul 2019 11:21:16 +0200 Subject: [PATCH 4/6] Rework omemo_start_device_session_handle_bundle exit In some conditions we just returned without freeing allocated variables. Should fix following valgrind reported leak: ``` ==17941== 19 bytes in 1 blocks are definitely lost in loss record 613 of 3,674 ==17941== at 0x483677F: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==17941== by 0x5BB0DAA: strdup (strdup.c:42) ==17941== by 0x4B1592: omemo_start_device_session_handle_bundle (omemo.c:126) ==17941== by 0x43405E: _iq_handler (iq.c:214) ==17941== by 0x5AF118E: ??? (in /usr/lib64/libmesode.so.0.0.0) ==17941== by 0x5AEDBDA: ??? (in /usr/lib64/libmesode.so.0.0.0) ==17941== by 0x5AFA43E: ??? (in /usr/lib64/libmesode.so.0.0.0) ==17941== by 0x6818AA4: ??? (in /usr/lib64/libexpat.so.1.6.8) ==17941== by 0x681A3AB: ??? (in /usr/lib64/libexpat.so.1.6.8) ==17941== by 0x681D7EB: XML_ParseBuffer (in /usr/lib64/libexpat.so.1.6.8) ==17941== by 0x5AF0A63: xmpp_run_once (in /usr/lib64/libmesode.so.0.0.0) ==17941== by 0x432E5D: connection_check_events (connection.c:104) ==17941== by 0x4323B3: session_process_events (session.c:255) ==17941== by 0x42C097: prof_run (profanity.c:128) ==17941== by 0x4B2610: main (main.c:172) ``` --- src/xmpp/omemo.c | 62 +++++++++++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/src/xmpp/omemo.c b/src/xmpp/omemo.c index cfa3f84c..e0f2a70d 100644 --- a/src/xmpp/omemo.c +++ b/src/xmpp/omemo.c @@ -111,8 +111,13 @@ omemo_bundle_request(const char * const jid, uint32_t device_id, ProfIqCallback int omemo_start_device_session_handle_bundle(xmpp_stanza_t *const stanza, void *const userdata) { + GList *prekeys_list = NULL; + unsigned char *signed_prekey_raw = NULL; + unsigned char *signed_prekey_signature_raw = NULL; char *from = NULL; + const char *from_attr = xmpp_stanza_get_attribute(stanza, STANZA_ATTR_FROM); + if (!from_attr) { Jid *jid = jid_create(connection_get_fulljid()); from = strdup(jid->barejid); @@ -122,54 +127,53 @@ omemo_start_device_session_handle_bundle(xmpp_stanza_t *const stanza, void *cons } if (g_strcmp0(from, userdata) != 0) { - return 1; + goto out; } xmpp_stanza_t *pubsub = xmpp_stanza_get_child_by_ns(stanza, STANZA_NS_PUBSUB); if (!pubsub) { - return 1; + goto out; } xmpp_stanza_t *items = xmpp_stanza_get_child_by_name(pubsub, "items"); if (!items) { - return 1; + goto out; } const char *node = xmpp_stanza_get_attribute(items, "node"); char *device_id_str = strstr(node, ":"); if (!device_id_str) { - return 1; + goto out; } uint32_t device_id = strtoul(++device_id_str, NULL, 10); xmpp_stanza_t *item = xmpp_stanza_get_child_by_name(items, "item"); if (!item) { - return 1; + goto out; } xmpp_stanza_t *bundle = xmpp_stanza_get_child_by_ns(item, STANZA_NS_OMEMO); if (!bundle) { - return 1; + goto out; } xmpp_stanza_t *prekeys = xmpp_stanza_get_child_by_name(bundle, "prekeys"); if (!prekeys) { - return 1; + goto out; } - GList *prekeys_list = NULL; xmpp_stanza_t *prekey; for (prekey = xmpp_stanza_get_children(prekeys); prekey != NULL; prekey = xmpp_stanza_get_next(prekey)) { omemo_key_t *key = malloc(sizeof(omemo_key_t)); const char *prekey_id_text = xmpp_stanza_get_attribute(prekey, "preKeyId"); if (!prekey_id_text) { - return 1; + goto out; } key->id = strtoul(prekey_id_text, NULL, 10); xmpp_stanza_t *prekey_text = xmpp_stanza_get_children(prekey); if (!prekey_text) { - return 1; + goto out; } char *prekey_b64 = xmpp_stanza_get_text(prekey_text); key->data = g_base64_decode(prekey_b64, &key->length); @@ -182,42 +186,44 @@ omemo_start_device_session_handle_bundle(xmpp_stanza_t *const stanza, void *cons xmpp_stanza_t *signed_prekey = xmpp_stanza_get_child_by_name(bundle, "signedPreKeyPublic"); if (!signed_prekey) { - return 1; + goto out; } const char *signed_prekey_id_text = xmpp_stanza_get_attribute(signed_prekey, "signedPreKeyId"); if (!signed_prekey_id_text) { - return 1; + goto out; } + uint32_t signed_prekey_id = strtoul(signed_prekey_id_text, NULL, 10); xmpp_stanza_t *signed_prekey_text = xmpp_stanza_get_children(signed_prekey); if (!signed_prekey_text) { - return 1; + goto out; } + size_t signed_prekey_len; char *signed_prekey_b64 = xmpp_stanza_get_text(signed_prekey_text); - unsigned char *signed_prekey_raw = g_base64_decode(signed_prekey_b64, &signed_prekey_len); + signed_prekey_raw = g_base64_decode(signed_prekey_b64, &signed_prekey_len); free(signed_prekey_b64); xmpp_stanza_t *signed_prekey_signature = xmpp_stanza_get_child_by_name(bundle, "signedPreKeySignature"); if (!signed_prekey_signature) { - return 1; + goto out; } xmpp_stanza_t *signed_prekey_signature_text = xmpp_stanza_get_children(signed_prekey_signature); if (!signed_prekey_signature_text) { - return 1; + goto out; } size_t signed_prekey_signature_len; char *signed_prekey_signature_b64 = xmpp_stanza_get_text(signed_prekey_signature_text); - unsigned char *signed_prekey_signature_raw = g_base64_decode(signed_prekey_signature_b64, &signed_prekey_signature_len); + signed_prekey_signature_raw = g_base64_decode(signed_prekey_signature_b64, &signed_prekey_signature_len); free(signed_prekey_signature_b64); xmpp_stanza_t *identity_key = xmpp_stanza_get_child_by_name(bundle, "identityKey"); if (!identity_key) { - return 1; + goto out; } xmpp_stanza_t *identity_key_text = xmpp_stanza_get_children(identity_key); if (!identity_key_text) { - return 1; + goto out; } size_t identity_key_len; char *identity_key_b64 = xmpp_stanza_get_text(identity_key_text); @@ -228,11 +234,19 @@ omemo_start_device_session_handle_bundle(xmpp_stanza_t *const stanza, void *cons signed_prekey_raw, signed_prekey_len, signed_prekey_signature_raw, signed_prekey_signature_len, identity_key_raw, identity_key_len); - free(from); - g_list_free_full(prekeys_list, (GDestroyNotify)omemo_key_free); - g_free(signed_prekey_raw); g_free(identity_key_raw); - g_free(signed_prekey_signature_raw); + +out: + free(from); + if (signed_prekey_raw) { + g_free(signed_prekey_raw); + } + if (signed_prekey_signature_raw) { + g_free(signed_prekey_signature_raw); + } + if (prekeys_list) { + g_list_free_full(prekeys_list, (GDestroyNotify)omemo_key_free); + } return 1; } @@ -292,12 +306,12 @@ omemo_receive_message(xmpp_stanza_t *const stanza, gboolean *trusted) goto skip; } - const char *rid_text = xmpp_stanza_get_attribute(key_stanza, "rid"); key->device_id = strtoul(rid_text, NULL, 10); if (!key->device_id) { goto skip; } + key->data = g_base64_decode(key_text, &key->length); free(key_text); key->prekey = g_strcmp0(xmpp_stanza_get_attribute(key_stanza, "prekey"), "true") == 0; From 482138feff6a6de0fd3aacd7a00dd0f35a086d41 Mon Sep 17 00:00:00 2001 From: Michael Vetter Date: Thu, 4 Jul 2019 11:55:53 +0200 Subject: [PATCH 5/6] Free key on error in omemo_start_device_session_handle_bundle() Fix: ``` ==20561== 32 bytes in 1 blocks are definitely lost in loss record 1,467 of 3,678 ==20561== at 0x483677F: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==20561== by 0x4B16C9: omemo_start_device_session_handle_bundle (omemo.c:167) ==20561== by 0x43405E: _iq_handler (iq.c:214) ==20561== by 0x5AF118E: ??? (in /usr/lib64/libmesode.so.0.0.0) ==20561== by 0x5AEDBDA: ??? (in /usr/lib64/libmesode.so.0.0.0) ==20561== by 0x5AFA43E: ??? (in /usr/lib64/libmesode.so.0.0.0) ==20561== by 0x6818AA4: ??? (in /usr/lib64/libexpat.so.1.6.8) ==20561== by 0x681A3AB: ??? (in /usr/lib64/libexpat.so.1.6.8) ==20561== by 0x681D7EB: XML_ParseBuffer (in /usr/lib64/libexpat.so.1.6.8) ==20561== by 0x5AF0A63: xmpp_run_once (in /usr/lib64/libmesode.so.0.0.0) ==20561== by 0x432E5D: connection_check_events (connection.c:104) ==20561== by 0x4323B3: session_process_events (session.c:255) ==20561== by 0x42C097: prof_run (profanity.c:128) ==20561== by 0x4B260D: main (main.c:172) ``` --- src/xmpp/omemo.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/xmpp/omemo.c b/src/xmpp/omemo.c index e0f2a70d..e44cc00e 100644 --- a/src/xmpp/omemo.c +++ b/src/xmpp/omemo.c @@ -168,13 +168,18 @@ omemo_start_device_session_handle_bundle(xmpp_stanza_t *const stanza, void *cons const char *prekey_id_text = xmpp_stanza_get_attribute(prekey, "preKeyId"); if (!prekey_id_text) { + omemo_key_free(key); goto out; } + key->id = strtoul(prekey_id_text, NULL, 10); xmpp_stanza_t *prekey_text = xmpp_stanza_get_children(prekey); + if (!prekey_text) { + omemo_key_free(key); goto out; } + char *prekey_b64 = xmpp_stanza_get_text(prekey_text); key->data = g_base64_decode(prekey_b64, &key->length); free(prekey_b64); From 9aad2aa4878fffec098e8b2ecf382be565bd44a7 Mon Sep 17 00:00:00 2001 From: Michael Vetter Date: Thu, 4 Jul 2019 14:25:53 +0200 Subject: [PATCH 6/6] Free iq_id_handlers correctly so far only the key part was freed. We also need to free the actual handler. Fix: ``` ==21171== 1,128 bytes in 47 blocks are definitely lost in loss record 3,476 of 3,670 ==21171== at 0x483677F: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==21171== by 0x434248: iq_id_handler_add (iq.c:265) ==21171== by 0x4B122E: omemo_devicelist_request (omemo.c:46) ==21171== by 0x4AC411: omemo_start_session (omemo.c:409) ==21171== by 0x4AC37C: omemo_start_sessions (omemo.c:396) ==21171== by 0x447881: sv_ev_roster_received (server_events.c:189) ==21171== by 0x444019: roster_result_handler (roster.c:312) ==21171== by 0x433FC2: _iq_handler (iq.c:202) ==21171== by 0x5AF118E: ??? (in /usr/lib64/libmesode.so.0.0.0) ==21171== by 0x5AEDBDA: ??? (in /usr/lib64/libmesode.so.0.0.0) ==21171== by 0x5AFA43E: ??? (in /usr/lib64/libmesode.so.0.0.0) ==21171== by 0x6818AA4: ??? (in /usr/lib64/libexpat.so.1.6.8) ==21171== by 0x681A3AB: ??? (in /usr/lib64/libexpat.so.1.6.8) ==21171== by 0x681D7EB: XML_ParseBuffer (in /usr/lib64/libexpat.so.1.6.8) ==21171== by 0x5AF0A63: xmpp_run_once (in /usr/lib64/libmesode.so.0.0.0) ==21171== by 0x432E5D: connection_check_events (connection.c:104) ==21171== by 0x4323B3: session_process_events (session.c:255) ==21171== by 0x42C097: prof_run (profanity.c:128) ==21171== by 0x4B2627: main (main.c:172) ``` --- src/xmpp/iq.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/xmpp/iq.c b/src/xmpp/iq.c index 91acc212..e31f3269 100644 --- a/src/xmpp/iq.c +++ b/src/xmpp/iq.c @@ -134,6 +134,7 @@ static int _command_exec_response_handler(xmpp_stanza_t *const stanza, void *con static void _iq_free_room_data(ProfRoomInfoData *roominfo); static void _iq_free_affiliation_set(ProfPrivilegeSet *affiliation_set); +static void _iq_id_handler_free(ProfIqHandler *handler); // scheduled static int _autoping_timed_send(xmpp_conn_t *const conn, void *const userdata); @@ -247,7 +248,7 @@ iq_handlers_init(void) g_list_free(keys); g_hash_table_destroy(id_handlers); } - id_handlers = g_hash_table_new_full(g_str_hash, g_str_equal, free, NULL); + id_handlers = g_hash_table_new_full(g_str_hash, g_str_equal, free, (GDestroyNotify)_iq_id_handler_free); rooms_cache = g_hash_table_new_full(g_str_hash, g_str_equal, free, (GDestroyNotify)xmpp_stanza_release); } @@ -259,6 +260,19 @@ iq_handlers_clear() } } +static void +_iq_id_handler_free(ProfIqHandler *handler) +{ + if (handler == NULL) { + return; + } + if (handler->free_func && handler->userdata) { + handler->free_func(handler->userdata); + } + free(handler); + handler = NULL; +} + void iq_id_handler_add(const char *const id, ProfIqCallback func, ProfIqFreeCallback free_func, void *userdata) {