From 848acdd1fa31c1b1c253e8f1067a430fb505ef8a Mon Sep 17 00:00:00 2001 From: Steffen Jaeckel Date: Thu, 15 Feb 2024 13:12:36 +0100 Subject: [PATCH 1/3] Improve const correctness Signed-off-by: Steffen Jaeckel --- src/command/cmd_funcs.c | 9 +++------ src/event/client_events.c | 3 +-- src/event/server_events.c | 8 +++----- src/omemo/omemo.c | 3 +-- src/otr/otr.c | 3 +-- src/plugins/plugins.c | 3 +-- src/ui/chatwin.c | 6 ++---- src/ui/console.c | 3 +-- src/xmpp/bookmark.c | 3 +-- src/xmpp/message.c | 6 ++---- src/xmpp/presence.c | 2 +- src/xmpp/session.c | 4 ++-- src/xmpp/xmpp.h | 2 +- tests/unittests/xmpp/stub_xmpp.c | 2 +- 14 files changed, 21 insertions(+), 36 deletions(-) diff --git a/src/command/cmd_funcs.c b/src/command/cmd_funcs.c index 65bfedad..e7b3f63e 100644 --- a/src/command/cmd_funcs.c +++ b/src/command/cmd_funcs.c @@ -943,8 +943,7 @@ _account_set_presence_priority(char* account_name, char* presence, char* priorit jabber_conn_status_t conn_status = connection_get_status(); if (conn_status == JABBER_CONNECTED) { - char* connected_account = session_get_account_name(); - resource_presence_t last_presence = accounts_get_last_presence(connected_account); + resource_presence_t last_presence = accounts_get_last_presence(session_get_account_name()); if (presence_type == last_presence) { cl_ev_presence_send(last_presence, 0); } @@ -3560,8 +3559,7 @@ cmd_join(ProfWin* window, const char* const command, gchar** args) } if (args[0] == NULL) { - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); if (account && account->muc_service) { char* uuid = connection_create_uuid(); auto_gchar gchar* room_str = g_strdup_printf("private-chat-%s@%s", uuid, account->muc_service); @@ -3587,8 +3585,7 @@ cmd_join(ProfWin* window, const char* const command, gchar** args) auto_gchar gchar* room = NULL; char* nick = NULL; char* passwd = NULL; - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); // full room jid supplied (room@server) if (room_arg->localpart) { diff --git a/src/event/client_events.c b/src/event/client_events.c index ee32d320..9b1f0ffd 100644 --- a/src/event/client_events.c +++ b/src/event/client_events.c @@ -119,8 +119,7 @@ cl_ev_presence_send(const resource_presence_t presence_type, const int idle_secs auto_char char* signed_status = NULL; #ifdef HAVE_LIBGPGME - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); if (account->pgp_keyid) { signed_status = p_gpg_sign(connection_get_presence_msg(), account->pgp_keyid); } diff --git a/src/event/server_events.c b/src/event/server_events.c index c577f780..4cab3979 100644 --- a/src/event/server_events.c +++ b/src/event/server_events.c @@ -146,7 +146,7 @@ sv_ev_roster_received(void) ui_show_roster(); } - char* account_name = session_get_account_name(); + const char* account_name = session_get_account_name(); #ifdef HAVE_LIBGPGME // check pgp key valid if specified @@ -275,8 +275,7 @@ sv_ev_room_history(ProfMessage* message) if (prefs_get_boolean(PREF_NOTIFY_ROOM_OFFLINE)) { // check if this message was sent while we were offline. // if so, treat it as a new message rather than a history event. - char* account_name = session_get_account_name(); - auto_gchar gchar* last_activity = accounts_get_last_activity(account_name); + auto_gchar gchar* last_activity = accounts_get_last_activity(session_get_account_name()); int msg_is_new = 0; if (last_activity) { @@ -1302,8 +1301,7 @@ sv_ev_bookmark_autojoin(Bookmark* bookmark) if (bookmark->nick) { nick = strdup(bookmark->nick); } else { - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); nick = strdup(account->muc_nick); account_free(account); } diff --git a/src/omemo/omemo.c b/src/omemo/omemo.c index e995f4cc..da0bfc53 100644 --- a/src/omemo/omemo.c +++ b/src/omemo/omemo.c @@ -1365,8 +1365,7 @@ gboolean omemo_automatic_start(const char* const recipient) { gboolean result = FALSE; - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); prof_omemopolicy_t policy; if (account->omemo_policy) { diff --git a/src/otr/otr.c b/src/otr/otr.c index 73c98f22..af12b209 100644 --- a/src/otr/otr.c +++ b/src/otr/otr.c @@ -633,8 +633,7 @@ otr_get_their_fingerprint(const char* const recipient) prof_otrpolicy_t otr_get_policy(const char* const recipient) { - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); // check contact specific setting if (g_list_find_custom(account->otr_manual, recipient, (GCompareFunc)g_strcmp0)) { account_free(account); diff --git a/src/plugins/plugins.c b/src/plugins/plugins.c index c64dceef..6238525d 100644 --- a/src/plugins/plugins.c +++ b/src/plugins/plugins.c @@ -313,8 +313,7 @@ plugins_unload(const char* const name) caps_reset_ver(); // resend presence to update server's disco info data for this client if (connection_get_status() == JABBER_CONNECTED) { - char* account_name = session_get_account_name(); - resource_presence_t last_presence = accounts_get_last_presence(account_name); + resource_presence_t last_presence = accounts_get_last_presence(session_get_account_name()); cl_ev_presence_send(last_presence, 0); } return TRUE; diff --git a/src/ui/chatwin.c b/src/ui/chatwin.c index 690bfbed..edb8f96a 100644 --- a/src/ui/chatwin.c +++ b/src/ui/chatwin.c @@ -67,8 +67,7 @@ gboolean _pgp_automatic_start(const char* const recipient) { gboolean result = FALSE; - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); if (g_list_find_custom(account->pgp_enabled, recipient, (GCompareFunc)g_strcmp0)) { result = TRUE; @@ -82,8 +81,7 @@ gboolean _ox_automatic_start(const char* const recipient) { gboolean result = FALSE; - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); if (g_list_find_custom(account->ox_enabled, recipient, (GCompareFunc)g_strcmp0)) { result = TRUE; diff --git a/src/ui/console.c b/src/ui/console.c index c090fe88..6a212f42 100644 --- a/src/ui/console.c +++ b/src/ui/console.c @@ -2885,8 +2885,7 @@ cons_privacy_setting(void) } if (connection_get_status() == JABBER_CONNECTED) { - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); if (account->client) { cons_show("Client name (/account set clientid) : %s", account->client); diff --git a/src/xmpp/bookmark.c b/src/xmpp/bookmark.c index dc96a5c0..8721ff50 100644 --- a/src/xmpp/bookmark.c +++ b/src/xmpp/bookmark.c @@ -179,8 +179,7 @@ bookmark_join(const char* jid) return FALSE; } - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); if (!muc_active(bookmark->barejid)) { char* nick = bookmark->nick; if (!nick) { diff --git a/src/xmpp/message.c b/src/xmpp/message.c index 0d153f20..388f5bb6 100644 --- a/src/xmpp/message.c +++ b/src/xmpp/message.c @@ -477,8 +477,7 @@ message_send_chat_pgp(const char* const barejid, const char* const msg, gboolean xmpp_stanza_t* message = NULL; #ifdef HAVE_LIBGPGME - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); if (account->pgp_keyid) { auto_jid Jid* jidp = jid_create(jid); auto_char char* encrypted = p_gpg_encrypt(jidp->barejid, msg, account->pgp_keyid); @@ -540,8 +539,7 @@ message_send_chat_ox(const char* const barejid, const char* const msg, gboolean xmpp_stanza_t* message = NULL; - char* account_name = session_get_account_name(); - ProfAccount* account = accounts_get_account(account_name); + ProfAccount* account = accounts_get_account(session_get_account_name()); message = xmpp_message_new(ctx, STANZA_TYPE_CHAT, jid, id); xmpp_message_set_body(message, "This message is encrypted (XEP-0373: OpenPGP for XMPP)."); diff --git a/src/xmpp/presence.c b/src/xmpp/presence.c index 2a6b3157..22a91abd 100644 --- a/src/xmpp/presence.c +++ b/src/xmpp/presence.c @@ -247,7 +247,7 @@ presence_send(const resource_presence_t presence_type, const int idle, char* sig last = STANZA_TEXT_ONLINE; } - char* account = session_get_account_name(); + const char* account = session_get_account_name(); accounts_set_last_presence(account, last); accounts_set_last_status(account, msg); } diff --git a/src/xmpp/session.c b/src/xmpp/session.c index de6f626a..f6d9c93d 100644 --- a/src/xmpp/session.c +++ b/src/xmpp/session.c @@ -272,7 +272,7 @@ session_process_events(void) } } -char* +const char* session_get_account_name(void) { return saved_account.name; @@ -417,7 +417,7 @@ session_check_autoaway(void) int away_time_ms = away_time * 60000; int xa_time_ms = xa_time * 60000; - char* account = session_get_account_name(); + const char* account = session_get_account_name(); resource_presence_t curr_presence = accounts_get_last_presence(account); auto_char char* curr_status = accounts_get_last_status(account); diff --git a/src/xmpp/xmpp.h b/src/xmpp/xmpp.h index 0e72ef33..755ad273 100644 --- a/src/xmpp/xmpp.h +++ b/src/xmpp/xmpp.h @@ -185,7 +185,7 @@ jabber_conn_status_t session_connect_with_account(const ProfAccount* const accou void session_disconnect(void); void session_shutdown(void); void session_process_events(void); -char* session_get_account_name(void); +const char* session_get_account_name(void); void session_reconnect_now(void); void connection_disconnect(void); diff --git a/tests/unittests/xmpp/stub_xmpp.c b/tests/unittests/xmpp/stub_xmpp.c index bffe976c..1ff25e69 100644 --- a/tests/unittests/xmpp/stub_xmpp.c +++ b/tests/unittests/xmpp/stub_xmpp.c @@ -128,7 +128,7 @@ connection_get_presence_msg(void) return mock_ptr_type(const char*); } -char* +const char* session_get_account_name(void) { return mock_ptr_type(char*); From 9640670f0f1f898c1ed3a60748c92e150b4991ab Mon Sep 17 00:00:00 2001 From: Steffen Jaeckel Date: Thu, 15 Feb 2024 13:12:51 +0100 Subject: [PATCH 2/3] Less GString usage Signed-off-by: Steffen Jaeckel --- src/xmpp/jid.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/xmpp/jid.c b/src/xmpp/jid.c index 565eb698..1f3b1a66 100644 --- a/src/xmpp/jid.c +++ b/src/xmpp/jid.c @@ -164,15 +164,7 @@ char* create_fulljid(const char* const barejid, const char* const resource) { auto_gchar gchar* barejidlower = g_utf8_strdown(barejid, -1); - GString* full_jid = g_string_new(barejidlower); - g_string_append(full_jid, "/"); - g_string_append(full_jid, resource); - - char* result = strdup(full_jid->str); - - g_string_free(full_jid, TRUE); - - return result; + return g_strdup_printf("%s/%s", barejidlower, resource); } /* From 3b099e9403c61d184d325c7b5c7fde30a572cf5c Mon Sep 17 00:00:00 2001 From: Steffen Jaeckel Date: Tue, 20 Feb 2024 15:58:20 +0100 Subject: [PATCH 3/3] Fix potential double free of ratchet identity key libsignal does this properly, so there wouldn't be a real double free, but it will `abort()`. Instead of destroying the identity key on disconnect, already destroy it after it has been put into the libsignal 'ratchet identity key pair'. In the case where the key pair is initially generated, the public and private parts are only `ref()`'ed once in [0]. In the case where the key pair is read from the disk, the public and private parts are `ref()`'ed twice, first when decoded in [1] resp. [2] and a second time in [3]. When `omemo_on_disconnect()` is called we were `unref()`'ing the parts twice, before this patch. First in [4], a second time in [5] resp. [6]. Now we do the second `unref()` already when loading. [0] `signal_protocol_key_helper_generate_identity_key_pair()` [1] `curve_decode_point()` [2] `curve_decode_private_point()` [3] `ratchet_identity_key_pair_create()` [4] `ratchet_identity_key_pair_destroy()` [5] `ec_private_key_destroy()` [6] `ec_public_key_destroy()` Signed-off-by: Steffen Jaeckel --- src/omemo/omemo.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/omemo/omemo.c b/src/omemo/omemo.c index da0bfc53..5155adac 100644 --- a/src/omemo/omemo.c +++ b/src/omemo/omemo.c @@ -271,11 +271,7 @@ omemo_on_disconnect(void) free_keyfile(&omemo_ctx.identity); signal_protocol_store_context_destroy(omemo_ctx.store); - ec_public_key* pub = ratchet_identity_key_pair_get_public(omemo_ctx.identity_key_pair); - ec_private_key* priv = ratchet_identity_key_pair_get_private(omemo_ctx.identity_key_pair); ratchet_identity_key_pair_destroy((signal_type_base*)omemo_ctx.identity_key_pair); - ec_private_key_destroy((signal_type_base*)priv); - ec_public_key_destroy((signal_type_base*)pub); signal_context_destroy(omemo_ctx.signal); memset(&omemo_ctx, 0, sizeof(omemo_ctx)); @@ -1470,6 +1466,8 @@ _load_identity(void) ec_private_key* private_key; curve_decode_private_point(&private_key, identity_key_private, identity_key_private_len, omemo_ctx.signal); ratchet_identity_key_pair_create(&omemo_ctx.identity_key_pair, public_key, private_key); + ec_private_key_destroy((signal_type_base*)private_key); + ec_public_key_destroy((signal_type_base*)public_key); char** keys = NULL; int i;