Re: Memory leak in smsbox

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Memory leak in smsbox

Alejandro Guerrieri
Peter,

I'll definitely try it. I've been struggling to find the cause of a
misterious memory-eating smsbox for weeks.

Since I don't master the "debugging skills" I've ended up using
tcptools (the "supervise" program from DJB) and softlimit to deny the
smsbox more memory, so the offending process get killed and restarted,
but I know it's a temporary solution. I think your patch may come in
handy :)

I'll try your patch and let you know if the problem is fixed on my setup.

Best regards,

On 1/20/06, Peter Christensen <[hidden email]> wrote:

> Hi,
>
> Earlier today I noticed a significant increase of memory consumption by
> smsbox. Over a period of 5½ hour it had gone from around 1.8MB to
> 209MB. When I examined the log of smsbox, two messages were rejected by
> smsbox due to bad character formatting, but a minute later the client
> attempted to transfer both messages again and again etc.
> Judging from this there must be a memory leak in smsbox which is only
> triggered upon errors.
>
> I went through some memory dumps and smsbox_req_handle and eventually
> concluded that upon errors the allowed, denied, and receiver lists were
> not freed.
>
> I've attached a patch which will free the lists on errors in between the
> creation and destruction of the lists.
>
> As far as I could see, the elements within the three lists are actually
> the same (that is, everything in allowed and denied are definitely also
> in received), and since we have not extracted the elements from the
> receiver yet, this patch only free the elements in the receiver list.
> Not the allowed and denied lists. Is this assumption correct? One thing
> is certain, freeing both receiver and allowed with gwlist_destroy(list,
> octstr_destroy_item) ended in a PANIC since the first destruction freed
> the elements in the other list.
>
>
> A brute spamming with invalid messages made an unpatched newly started
> kannel go from 1.3MB to 10MB in two seconds. The same thing with a
> patched kannel went from 1.3MB to 1.5MB where it stayed (I assume that
> the first bunch was first-time single-shot allocations). The memory
> sizes are RSS.
>
>
> Another unrelated thing... I've been bombing the mailing list with
> patches and stuff the last few weeks, but what exactly is the preferred
> way of getting patches? In the mailing list, or by creating a bug report
> with an attached patch?
>
> --
> Med venlig hilsen / Best regards
>
> Peter Christensen
>
> Developer
> ------------------
> Cool Systems ApS
>
> Tel: +45 2888 1600
> @ : [hidden email]
> www: www.coolsystems.dk
>
>
>


--
Alejandro Guerrieri
Magicom
http://www.magicom-bcn.net/

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Memory leak in smsbox

Stipe Tolj
Alejandro Guerrieri wrote:

> Peter,
>
> I'll definitely try it. I've been struggling to find the cause of a
> misterious memory-eating smsbox for weeks.
>
> Since I don't master the "debugging skills" I've ended up using
> tcptools (the "supervise" program from DJB) and softlimit to deny the
> smsbox more memory, so the offending process get killed and restarted,
> but I know it's a temporary solution. I think your patch may come in
> handy :)

yep, agree'ing... BTW, debugging memory leaks may be done with valgrind. Which
is our favorite approach to identify these kind of issues.

> I'll try your patch and let you know if the problem is fixed on my setup.

good, we approtiate any support in reviewing patchsets by users. Please do so
and drop a vote on the list.

I'll do the same.

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
-------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Memory leak in smsbox

Stipe Tolj
In reply to this post by Alejandro Guerrieri
Peter Christensen wrote:

> Another unrelated thing... I've been bombing the mailing list with
> patches and stuff the last few weeks, but what exactly is the preferred
> way of getting patches? In the mailing list, or by creating a bug report
> with an attached patch?

prefered way is the mailing list, IMO. Where you prefix the subject with
"[PATCH] foobar patch", so we have a fast indication scanning the topics and
attaching the patch in unified diff format as you did for this.

This is mostly for "new features", or "behaviour changes".

If you patch _DOES_ fix an obvious bug, I'd suggest to go with the bug tracking
way, so we keep track of the things that have been fixed. Hence fill in a bug
report and provide the patch as attachement there.

Developers who wish to follow the development stream will hence be subscribed to
devel@ for general purposes and discussion _AND_ have a mantis account receiving
and new issues from there.

This would be my suggested approach... others?

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
-------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Memory leak in smsbox

Stipe Tolj
In reply to this post by Alejandro Guerrieri
Peter Christensen wrote:

> diff -Nru gateway/gw/smsbox.c gateway.leak/gw/smsbox.c
> --- gateway/gw/smsbox.c 2005-12-09 03:14:31.000000000 +0100
> +++ gateway.leak/gw/smsbox.c 2006-01-20 21:30:57.000000000 +0100
> @@ -2119,7 +2119,7 @@
>   newfrom = octstr_duplicate(global_sender);
>      } else {
>   returnerror = octstr_create("Sender missing and no global set, rejected");
> - goto fielderror2;
> + goto fielderror3;
>      }

can we revise this to keep the present goto labels instead of introducing new
ones? The flow-control-logic in that function get's too complicated.

>      info(0, "sendsms sender:<%s:%s> (%s) to:<%s> msg:<%s>",
> @@ -2365,6 +2365,11 @@
>      octstr_destroy(newfrom);
>      msg_destroy(msg);
>  
> +fielderror3:
> +    gwlist_destroy(allowed, NULL);
> +    gwlist_destroy(denied, NULL);
> +    gwlist_destroy(receiver, octstr_destroy_item);
> +

why don't we gwlist_destroy([allowed|denied], octstr_destroy_item) too?


>  fielderror2:
>      alog("send-SMS request failed - %s",
>           octstr_get_cstr(returnerror));


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
-------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Memory leak in smsbox

Peter Christensen-3


Stipe Tolj wrote:

> Peter Christensen wrote:
>
>> diff -Nru gateway/gw/smsbox.c gateway.leak/gw/smsbox.c
>> --- gateway/gw/smsbox.c    2005-12-09 03:14:31.000000000 +0100
>> +++ gateway.leak/gw/smsbox.c    2006-01-20 21:30:57.000000000 +0100
>> @@ -2119,7 +2119,7 @@
>>      newfrom = octstr_duplicate(global_sender);
>>      } else {
>>      returnerror = octstr_create("Sender missing and no global set,
>> rejected");
>> -    goto fielderror2;
>> +    goto fielderror3;
>>      }
>
> can we revise this to keep the present goto labels instead of
> introducing new ones? The flow-control-logic in that function get's too
> complicated.
>
>>      info(0, "sendsms sender:<%s:%s> (%s) to:<%s> msg:<%s>",
>> @@ -2365,6 +2365,11 @@
>>      octstr_destroy(newfrom);
>>      msg_destroy(msg);
>>  
>> +fielderror3:
>> +    gwlist_destroy(allowed, NULL);
>> +    gwlist_destroy(denied, NULL);
>> +    gwlist_destroy(receiver, octstr_destroy_item);
>> +
>
> why don't we gwlist_destroy([allowed|denied], octstr_destroy_item) too?
>

At this point some elements are present in both the allowed or denied
and the receiver list. Here's how I interpret the flow (reduced list):

1.  At first, we have a list of recipients (receiver)
2.  Empty allowed and denied lists are created
3.  Each recipient is added to either allowed or denied depending on
blacklisting. (The lists contains only pointers to the Octstr, so all
pointers in allowed and denied are also present in receiver)
4.  Every recipient both present in denied and allowed are removed from
allowed
5.  Empty failed_id list is created
6.  Each recipient is extracted from allowed and sent to bearerbox with
message. Failed recipients are added to failed_id list.
7.  With each recipient removed from allowed, reipients are now only
present in both receiver and denied or receiver and failed_id at worst)
8.  Message structure is freed
9.  Receiver is freed with octstr_destroy_item
10. Now, every recipient in denied (if any) are actually invalid as they
have been freed.
11. Allowed is freed with octstr_destroy_item
12. Now, every recipient in failed_id (if any) are also invalid
13. If all recipients was denied (no_recv == gwlist_len(denied)),
terminate with error
14. If failed_id list is not empty, goto error (no. 18)
15. failed_id is freed with octstr_destroy_item (no need to use
octstr_destroy_item here, as failed_id is ALWAYS empty at this point. If
there WAS any element in the list, they would have been freed with
allowed was freed, and thus generate an error)
16. Each element in denied is extracted and appended to output (but at
this point, every string in the list is invalid. I foresee a
segmentation fault here)
17. denied is freed with octstr_destroy_item (Again, no need. Is always
empty at this point)
18. Terminate

19. Each failed recipient is extracted from failed_id and appended to
output (But every element is invalid and we'll get a segmentation fault)
20. failed_id is freed with octstr_destroy_item (but since list is
empty, octstr_destroy_item is unnecessary)
21. denied is freed with octstr_destroy_item (double free, as every
element have already been freed)
22. Terminate


Just as expected... Did a little test to prove whether my segfault
theory was accurate. I've added "234" to my black list:

http://localhost:13003/cgi-bin/sendsms?username=testuser&password=testpass&from=123&to=123
smsbox says "Sent." (denied and failed_id is empty)

http://localhost:13003/cgi-bin/sendsms?username=testuser&password=testpass&from=123&to=123%20123
smsbox says "Sent." (denied and failed_id is empty)

http://localhost:13003/cgi-bin/sendsms?username=testuser&password=testpass&from=123&to=234
smsbox says "Number(s) has/have been denied by white- and/or
black-lists." (denied have one item and failed_id is empty. Since ALL
recipients failed, they are not shown in output and we get no segfault.
In fact denied, failed_id, and newfrom are not freed. Slow, but
potential memory leak?)

http://localhost:13003/cgi-bin/sendsms?username=testuser&password=testpass&from=123&to=123%20234
smsbox dies with no reply. (smsbox attempts to print out that 234 have
failed, but this string have been freed although it is present in the
denied list)

http://localhost:13003/cgi-bin/sendsms?username=testuser&password=testpass&from=123&to=234%20234
smsbox dies with no reply. (Both recipients are rejected, but as they
are identical, no_recv is 2, but denied have only one element. Therefore
smsbox will fail as it attempts to return a list of denied recipients.

http://localhost:13003/cgi-bin/sendsms?username=testuser&password=testpass&from=123&to=234%20123A
smsbox says "Number(s) has/have been denied by white- and/or
black-lists." (At this point the two rejected numbers are NOT identical
and no_recv becomes gwlist_len(denied) and we won't crash)


So, smsbox definitely doesn't handle errors well when there are multiple
recipients.


>
>>  fielderror2:
>>      alog("send-SMS request failed - %s",
>>           octstr_get_cstr(returnerror));
>
>
> 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
> -------------------------------------------------------------------

--
Med venlig hilsen / Best regards

Peter Christensen

Developer
------------------
Cool Systems ApS

Tel: +45 2888 1600
@ : [hidden email]
www: www.coolsystems.dk

Loading...