PATCH: Memory leak in Clickatell + Brunet HTTP SMSC
Hi everyone, Attached is a diff for memory leak found in smsc_http.c for Clickatell and Brunet specific implementations. The message ID Octstr's never get destroyed when the dictionary gets destroyed. Thank you! -- Donald Jackson http://www.ddj.co.za/ donaldjster(a)gmail.com smsc_http.c.diff Description: Binary data
Re: PATCH: Memory leak in Clickatell + Brunet HTTP SMSC
Donald Jackson schrieb:
Hi everyone,
Attached is a diff for memory leak found in smsc_http.c for Clickatell
and Brunet specific implementations. The message ID Octstr's never get
destroyed when the dictionary gets destroyed.
-1 on this patch. Reason: we do a double free() on the Octstr pointers and cause
a segfault.
The issue is:
- octstr_split_words() malloc()s the new items in the list.
- then we inject to Dict the pointer refrence, that's the original, not a
copy.
- then we gwlist_destroy() and hence all items of the list are freed, also
the one we put into the dict.
- later on we dict_destroy() _WITH_ the octstr_destroy() callback and hence
we'll try to double free. Bang.
Attached is a revised patch, that takes care we duplicate the items that we put
into the Dict hash, so we have a clean destruction later, as the
gwlist_destroy() took care of the original ones.
Please review and vote.
Stipe
---
Kölner Landstrasse 419
40589 Düsseldorf, NRW, Germany
tolj.org system architecture Kannel Software Foundation (KSF)
http://www.tolj.org/ http://www.kannel.org/
mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org
---
### Eclipse Workspace Patch 1.0
#P gateway-cvs-head
Index: gw/smsc/smsc_http.c
===
RCS file: /home/cvs/gateway/gw/smsc/smsc_http.c,v
retrieving revision 1.56
diff -u -r1.56 smsc_http.c
--- gw/smsc/smsc_http.c 14 Jul 2008 14:50:12 - 1.56
+++ gw/smsc/smsc_http.c 14 Jul 2008 15:09:12 -
@@ -726,16 +726,16 @@
words = octstr_split_words(body);
if ((len = gwlist_len(words)) 1) {
- word = gwlist_extract_first(words);
- if (octstr_compare(word, octstr_imm(ID:)) == 0) {
- value = gwlist_extract_first(words);
- param = dict_create(4, NULL);
- dict_put(param, octstr_imm(ID), value);
- } else if (octstr_compare(word, octstr_imm(ERR:)) == 0) {
- value = gwlist_extract_first(words);
- param = dict_create(4, NULL);
- dict_put(param, octstr_imm(ERR), value);
- }
+word = gwlist_extract_first(words);
+if (octstr_compare(word, octstr_imm(ID:)) == 0) {
+value = gwlist_extract_first(words);
+param = dict_create(4, (void(*)(void *)) octstr_destroy);
+dict_put(param, octstr_imm(ID), octstr_duplicate(value));
+} else if (octstr_compare(word, octstr_imm(ERR:)) == 0) {
+value = gwlist_extract_first(words);
+param = dict_create(4, (void(*)(void *)) octstr_destroy);
+dict_put(param, octstr_imm(ERR), octstr_duplicate(value));
+}
octstr_destroy(word);
}
gwlist_destroy(words, (void(*)(void *)) octstr_destroy);
@@ -1017,13 +1017,13 @@
words = octstr_split_words(body);
if ((len = gwlist_len(words)) 0) {
-param = dict_create(4, NULL);
+param = dict_create(4, (void(*)(void *)) octstr_destroy);
while ((word = gwlist_extract_first(words)) != NULL) {
List *l = octstr_split(word, octstr_imm(=));
Octstr *key = gwlist_extract_first(l);
Octstr *value = gwlist_extract_first(l);
if (octstr_len(key))
-dict_put(param, key, value);
+dict_put(param, key, octstr_duplicate(value));
octstr_destroy(key);
octstr_destroy(word);
gwlist_destroy(l, (void(*)(void *)) octstr_destroy);
RE: PATCH: Memory leak in Clickatell + Brunet HTTP SMSC
Hi Stipe
I must disagree with you (unfortunately).
If you look closely, you'll see that:
word = gwlist_extract_first(words);
and
value = gwlist_extract_first(words);
is used on the list, (i.e. 2 elements removed from the list), and when
gwlist_destroy is called, 'word' and 'value' is not destroyed.
There is an octstr_destroy(word), but there is no octstr_destroy(value), and
because there is no callback for the 'param' dictionary, 'value' never will be
destroyed, resulting in a memory leak.
The patch you provided creates a copy of 'value' which will be destroyed via
the callback when dict_destroy is called, but the original 'value' will still
reside in memory indefinitely. So I would go for the patch provided by Mr.
Jackson, or you should add a octstr_destroy(value) to your patch.
Regards
--
Werner Coetzee
* C Developer: Messaging Engine
* T +27 21 910 7700
* F +27 21 910 7701
[EMAIL PROTECTED]
www.clickatell.com
-Original Message-
From: Stipe Tolj [mailto:[EMAIL PROTECTED]
Sent: 14 July 2008 17:13
Cc: devel@kannel.org
Subject: Re: PATCH: Memory leak in Clickatell + Brunet HTTP SMSC
Donald Jackson schrieb:
Hi everyone,
Attached is a diff for memory leak found in smsc_http.c for Clickatell
and Brunet specific implementations. The message ID Octstr's never get
destroyed when the dictionary gets destroyed.
-1 on this patch. Reason: we do a double free() on the Octstr pointers and cause
a segfault.
The issue is:
- octstr_split_words() malloc()s the new items in the list.
- then we inject to Dict the pointer refrence, that's the original, not a
copy.
- then we gwlist_destroy() and hence all items of the list are freed, also
the one we put into the dict.
- later on we dict_destroy() _WITH_ the octstr_destroy() callback and hence
we'll try to double free. Bang.
Attached is a revised patch, that takes care we duplicate the items that we put
into the Dict hash, so we have a clean destruction later, as the
gwlist_destroy() took care of the original ones.
Please review and vote.
Stipe
---
Kölner Landstrasse 419
40589 Düsseldorf, NRW, Germany
tolj.org system architecture Kannel Software Foundation (KSF)
http://www.tolj.org/ http://www.kannel.org/
mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org
---
Re: PATCH: Memory leak in Clickatell + Brunet HTTP SMSC
Werner Coetzee schrieb:
Hi Stipe
I must disagree with you (unfortunately).
If you look closely, you'll see that:
word = gwlist_extract_first(words);
and
value = gwlist_extract_first(words);
is used on the list, (i.e. 2 elements removed from the list), and when
gwlist_destroy is called, 'word' and 'value' is not destroyed.
There is an octstr_destroy(word), but there is no octstr_destroy(value), and
because there is no callback for the 'param' dictionary, 'value' never will be
destroyed, resulting in a memory leak.
The patch you provided creates a copy of 'value' which will be destroyed via
the callback when dict_destroy is called, but the original 'value' will still
reside in memory indefinitely. So I would go for the patch provided by Mr.
Jackson, or you should add a octstr_destroy(value) to your patch.
yup Werner, thanks for the shot here. You're of course right. I wasn't having my
eyes on the missing octstr_destroy(value) itself. Credits on your eyes here ;)
Stipe
---
Kölner Landstrasse 419
40589 Düsseldorf, NRW, Germany
tolj.org system architecture Kannel Software Foundation (KSF)
http://www.tolj.org/ http://www.kannel.org/
mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org
---
Re: PATCH: Memory leak in Clickatell + Brunet HTTP SMSC
Donald Jackson schrieb:
Hi everyone,
Attached is a diff for memory leak found in smsc_http.c for Clickatell
and Brunet specific implementations. The message ID Octstr's never get
destroyed when the dictionary gets destroyed.
and after Werner's correction to my mail, +1, and committed to CVS:
2008-07-15 Stipe Tolj stolj at kannel.org
* gw/smsc/smsc_http.c: memory leak fix for Clickatell and Brunet HTTP API.
Thanks to Donald Jackson donaldjster at gmail.com for the patch.
[Msg-Id: [EMAIL PROTECTED]]
Stipe
---
Kölner Landstrasse 419
40589 Düsseldorf, NRW, Germany
tolj.org system architecture Kannel Software Foundation (KSF)
http://www.tolj.org/ http://www.kannel.org/
mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org
---
