Re: Memory leak in smsbox

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
@ : [email protected]
www: www.coolsystems.dk