Package: pidgin
Version: 2.10.6-2
Severity: normal
Tags: upstream patch
Hi,
I reviewed the pidgin source for memory leaks. I have used cppcheck
to locate possible leaks and then manually tried to verify them. I am
sending my findings with patches that demonstrate possible ways of
fixing the leaks.
While I believe them to be good, I have not tested them (not even
compiled tested). Nor do I have a thorough understanding of the
internals, so a second review of them would be appreciated.
I suspect there is a leak more in zephyr.c[1] that I did not fix. It
appears in zephyr_join_chat, where classname and instname is mixed
between "must-free" and "must-not-free" strings[2].
The patches are based on top of the Debian git repository (the one
published by the PTS).
Thanks you for considering,
~Niels
[1] libpurple/protocols/zephyr/zephyr.c
[2] Example:
"""
if (!instname || !strlen(instname))
instname = "*";
^^^^^^^^^^^^^^
"""
constant string must not be freed.
"""
if (!g_ascii_strcasecmp(instname,"%host%"))
instname = g_strdup(zephyr->ourhost);
if (!g_ascii_strcasecmp(instname,"%canon%"))
instname = g_strdup(zephyr->ourhostcanon);
"""
^^^^^^^^
Result of g_strdup must be passed to g_free eventually.
>From 934829009f8919cc557a405ca76b8a29ce57a375 Mon Sep 17 00:00:00 2001
From: Niels Thykier <[email protected]>
Date: Wed, 12 Dec 2012 13:04:29 +0100
Subject: [PATCH 1/6] auth_scram.c: Fix memleak in jabber_scram_calc_proofs
Signed-off-by: Niels Thykier <[email protected]>
---
libpurple/protocols/jabber/auth_scram.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/libpurple/protocols/jabber/auth_scram.c b/libpurple/protocols/jabber/auth_scram.c
index a971f09..7263f50 100644
--- a/libpurple/protocols/jabber/auth_scram.c
+++ b/libpurple/protocols/jabber/auth_scram.c
@@ -183,8 +183,13 @@ jabber_scram_calc_proofs(JabberScramData *data, GString *salt, guint iterations)
memset(pass->str, 0, pass->allocated_len);
g_string_free(pass, TRUE);
- if (!salted_password)
+ if (!salted_password) {
+ g_free(server_key);
+ g_free(client_signature);
+ g_free(stored_key);
+ g_free(client_key);
return FALSE;
+ }
/* client_key = HMAC(salted_password, "Client Key") */
hmac(data->hash, client_key, salted_password, "Client Key");
--
1.7.10.4
>From 4d5c9911b4848b6b79dc0c41d1728706fd79fa56 Mon Sep 17 00:00:00 2001
From: Niels Thykier <[email protected]>
Date: Wed, 12 Dec 2012 13:05:59 +0100
Subject: [PATCH 2/6] oob.c: Fix memleak in jabber_oob_parse
Signed-off-by: Niels Thykier <[email protected]>
---
libpurple/protocols/jabber/oob.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/libpurple/protocols/jabber/oob.c b/libpurple/protocols/jabber/oob.c
index 430d17a..0df745a 100644
--- a/libpurple/protocols/jabber/oob.c
+++ b/libpurple/protocols/jabber/oob.c
@@ -211,6 +211,7 @@ void jabber_oob_parse(JabberStream *js, const char *from, JabberIqType type,
jox = g_new0(JabberOOBXfer, 1);
if (!purple_url_parse(url, &jox->address, &jox->port, &jox->page, NULL, NULL)) {
g_free(url);
+ g_free(jox);
return;
}
g_free(url);
--
1.7.10.4
>From c9bf107c484a24ff7de35376ddaa1f322ce2e7fd Mon Sep 17 00:00:00 2001
From: Niels Thykier <[email protected]>
Date: Wed, 12 Dec 2012 13:12:00 +0100
Subject: [PATCH 3/6] myspace.c: Fix memleaks in msim_uri_handler
Signed-off-by: Niels Thykier <[email protected]>
---
libpurple/protocols/myspace/myspace.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/libpurple/protocols/myspace/myspace.c b/libpurple/protocols/myspace/myspace.c
index 9cc52f5..d2f1a2a 100644
--- a/libpurple/protocols/myspace/myspace.c
+++ b/libpurple/protocols/myspace/myspace.c
@@ -3579,7 +3579,10 @@ msim_uri_handler(const gchar *proto, const gchar *cmd, GHashTable *params)
}
session = (MsimSession *)account->gc->proto_data;
- g_return_val_if_fail(session != NULL, FALSE);
+ if (session == NULL) {
+ g_free(cid_str);
+ return FALSE;
+ }
/* Lookup userid to username. TODO: push this down, to IM sending/contact
* adding functions. */
@@ -3596,7 +3599,7 @@ msim_uri_handler(const gchar *proto, const gchar *cmd, GHashTable *params)
g_free(cid_str);
return TRUE;
}
-
+ g_free(cid_str);
return FALSE;
}
--
1.7.10.4
>From d1ecfa2bbdb37701c7fc245a84ae19580d89ae74 Mon Sep 17 00:00:00 2001
From: Niels Thykier <[email protected]>
Date: Wed, 12 Dec 2012 13:17:33 +0100
Subject: [PATCH 4/6] util.c: Fix win32-only leak in silcpurple_check_silc_dir
Signed-off-by: Niels Thykier <[email protected]>
---
libpurple/protocols/silc/util.c | 8 +++++++-
libpurple/protocols/silc10/util.c | 7 +++++++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/libpurple/protocols/silc/util.c b/libpurple/protocols/silc/util.c
index 68eac32..c4f09cf 100644
--- a/libpurple/protocols/silc/util.c
+++ b/libpurple/protocols/silc/util.c
@@ -310,7 +310,13 @@ gboolean silcpurple_check_silc_dir(PurpleConnection *gc)
if (fd != -1)
close(fd);
-
+#ifdef _WIN32
+ /* on win32, we calloc pw so pass it to free
+ * (see the getpwuid code below)
+ * NB: cppcheck seems to get this wrong.
+ */
+ free(pw);
+#endif
return TRUE;
}
diff --git a/libpurple/protocols/silc10/util.c b/libpurple/protocols/silc10/util.c
index 792afde..5b80fc3 100644
--- a/libpurple/protocols/silc10/util.c
+++ b/libpurple/protocols/silc10/util.c
@@ -304,6 +304,13 @@ gboolean silcpurple_check_silc_dir(PurpleConnection *gc)
if (fd != -1)
close(fd);
+#ifdef _WIN32
+ /* on win32, we calloc pw so pass it to free
+ * (see the getpwuid code below)
+ * NB: cppcheck seems to get this wrong.
+ */
+ free(pw);
+#endif
return TRUE;
}
--
1.7.10.4
>From 0c6d721cccf24198d86e308a70a5790014a0173c Mon Sep 17 00:00:00 2001
From: Niels Thykier <[email protected]>
Date: Wed, 12 Dec 2012 13:20:06 +0100
Subject: [PATCH 5/6] yahoo_filexfer.c: Fix memleak in
yahoo_p2p_ft_HEAD_GET_cb
Signed-off-by: Niels Thykier <[email protected]>
---
libpurple/protocols/yahoo/yahoo_filexfer.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/libpurple/protocols/yahoo/yahoo_filexfer.c b/libpurple/protocols/yahoo/yahoo_filexfer.c
index bf87dbe..2d7231c 100644
--- a/libpurple/protocols/yahoo/yahoo_filexfer.c
+++ b/libpurple/protocols/yahoo/yahoo_filexfer.c
@@ -1422,6 +1422,7 @@ static void yahoo_p2p_ft_HEAD_GET_cb(gpointer data, gint source, PurpleInputCond
purple_debug_warning("yahoo","p2p-ft: Wrong HEAD/GET request from peer, disconnecting host\n");
purple_input_remove(xd->input_event);
purple_xfer_cancel_remote(xfer);
+ g_free(url_get);
g_free(url_head);
return;
}
--
1.7.10.4
>From 2e6535012e1856f8db68920e2739f6ae9d7f7466 Mon Sep 17 00:00:00 2001
From: Niels Thykier <[email protected]>
Date: Wed, 12 Dec 2012 13:38:56 +0100
Subject: [PATCH 6/6] zephyr.c: Fix multiple leaks (incomplete)
Fix some leaks in zephyr.c, but the fix is incomplete for
zephyr_action_get_subs_from_server, where there is still a leak
in some casess (see the FIXME).
Signed-off-by: Niels Thykier <[email protected]>
---
libpurple/protocols/zephyr/zephyr.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/libpurple/protocols/zephyr/zephyr.c b/libpurple/protocols/zephyr/zephyr.c
index 69c7113..310cec5 100644
--- a/libpurple/protocols/zephyr/zephyr.c
+++ b/libpurple/protocols/zephyr/zephyr.c
@@ -1495,6 +1495,7 @@ static void process_zsubs(zephyr_account *zephyr)
}
fclose(f);
}
+ g_free(fname);
}
static void process_anyone(PurpleConnection *gc)
@@ -2159,11 +2160,15 @@ static int zephyr_send_message(zephyr_account *zephyr,char* zclass, char* instan
len = strlen(zsendstr);
result = write(zephyr->totzc[ZEPHYR_FD_WRITE], zsendstr, len);
if (result != len) {
+ g_free(tzc_sig);
+ g_free(tzc_body);
g_free(zsendstr);
g_free(html_buf2);
g_free(html_buf);
return errno;
}
+ g_free(tzc_sig);
+ g_free(tzc_body);
g_free(zsendstr);
} else if (use_zeph02(zephyr)) {
ZNotice_t notice;
@@ -2583,19 +2588,24 @@ static PurpleCmdRet zephyr_purple_cmd_msg(PurpleConversation *conv,
const char *cmd, char **args, char **error, void *data)
{
char *recipient;
+ PurpleCmdRet ret;
zephyr_account *zephyr = purple_conversation_get_gc(conv)->proto_data;
if (!g_ascii_strcasecmp(args[0],"*"))
return PURPLE_CMD_RET_FAILED; /* "*" is not a valid argument */
else
recipient = local_zephyr_normalize(zephyr,args[0]);
- if (strlen(recipient) < 1)
+ if (strlen(recipient) < 1) {
+ g_free(recipient);
return PURPLE_CMD_RET_FAILED; /* a null recipient is a chat message, not an IM */
+ }
if (zephyr_send_message(zephyr,"MESSAGE","PERSONAL",recipient,args[1],zephyr_get_signature(),""))
- return PURPLE_CMD_RET_OK;
+ ret = PURPLE_CMD_RET_OK;
else
- return PURPLE_CMD_RET_FAILED;
+ ret = PURPLE_CMD_RET_FAILED;
+ g_free(recipient);
+ return ret;
}
static PurpleCmdRet zephyr_purple_cmd_zlocate(PurpleConversation *conv,
@@ -2795,10 +2805,12 @@ static void zephyr_action_get_subs_from_server(PurplePluginAction *action)
title = g_strdup_printf("Server subscriptions for %s", zephyr->username);
if (zephyr->port == 0) {
+ g_free(title);
purple_debug_error("zephyr", "error while retrieving port\n");
return;
}
if ((retval = ZRetrieveSubscriptions(zephyr->port,&nsubs)) != ZERR_NONE) {
+ g_free(title);
/* XXX better error handling */
purple_debug_error("zephyr", "error while retrieving subscriptions from server\n");
return;
@@ -2807,6 +2819,7 @@ static void zephyr_action_get_subs_from_server(PurplePluginAction *action)
one = 1;
if ((retval = ZGetSubscriptions(&subs,&one)) != ZERR_NONE) {
/* XXX better error handling */
+ g_free(title);
purple_debug_error("zephyr", "error while retrieving individual subscription\n");
return;
}
@@ -2815,6 +2828,9 @@ static void zephyr_action_get_subs_from_server(PurplePluginAction *action)
subs.zsub_recipient);
}
purple_notify_formatted(gc, title, title, NULL, subout->str, NULL, NULL);
+ /* FIXME: subout is leaked, title and subout->str might is as well if
+ * purple_notify_formatted does not "take the it ownership"...
+ */
} else {
/* XXX fix */
purple_notify_error(gc,"","tzc doesn't support this action",NULL);
--
1.7.10.4
