RE: Does opensmppbox and smsbox have serious memory issues?

Hello Andreas,

 

The smsc_id that is returned by boxc_route_msg_to_smsc is NOT a copy, so it
shouldn't be destroyed afterwards.

But you put the finger on the right spot. There was a memory leak in that
function.

 

I commited attached diff to svn. Please, can somebody test it and see if
this makes _the_ difference for opensmppbox?

 

== Rene

 

 

From: Andreas Fink [mailto:[email protected]] 
Sent: maandag 28 april 2014 12:51
To: Rene Kluwen
Cc: [email protected]; devel Devel; Hanh Le Bich
Subject: Re: Does opensmppbox and smsbox have serious memory issues?

 

 





==31087== 4,986,528 (77,472 direct, 4,909,056 indirect) bytes in 4,842
blocks are definitely lost in loss record 813 of 813
==31087==    at 0x4027434: malloc (vg_replace_malloc.c:291)
==31087==    by 0x80970B3: gw_native_malloc (gwmem-native.c:87)
==31087==    by 0x80A37A1: octstr_create_from_data_real (octstr.c:263)
==31087==    by 0x80A3916: octstr_create_real (octstr.c:245)
==31087==    by 0x80A908E: octstr_format_valist_real (octstr.c:2480)
==31087==    by 0x80A9366: octstr_format (octstr.c:2469)
==31087==    by 0x80534F5: boxc_route_msg_to_smsc (opensmppbox.c:1791)
==31087==    by 0x8057AAE: smpp_to_bearerbox (opensmppbox.c:1638)
==31087==    by 0x80983AE: new_thread (gwthread-pthread.c:385)
==31087==    by 0x46F9C38: start_thread (pthread_create.c:304)
==31087==    by 0x482F78D: clone (clone.S:130)

 

this seems to be a leak in opensmppbox in this code segment (handle_pdu) in
opensmppbox.c around line 1634

 

 

            case submit_sm:
                        msg = pdu_to_msg(box, pdu, &reason);
                        msg2 = msg;
                        if (msg == NULL) {
                                   resp = smpp_pdu_create(generic_nack,
pdu->u.submit_sm.sequence_number);
                                   resp->u.generic_nack.command_status =
SMPP_ESME_RUNKNOWNERR;
                        }
                        else {
                                   Octstr *smsc_id =
boxc_route_msg_to_smsc(box, msg);  <-smsc_id is allocated but never freeed.
                                   check_multipart(box, msg, &msg_to_send,
&msg2, &parts_list);
                                   msg->sms.smsc_id = smsc_id ?
octstr_duplicate(smsc_id) : NULL; /* its duplicated here... */
                                   msg->sms.boxc_id =
octstr_duplicate(box->boxc_id);
                                   msg_dump(msg, 0);
                                   resp = smpp_pdu_create(submit_sm_resp,
pdu->u.submit_sm.sequence_number);
                                   msgid = generate_smppid(msg);
                                   msg->sms.dlr_url =
octstr_duplicate(msgid);
                                   resp->u.submit_sm_resp.message_id =
msgid;
                                   if (msg_to_send) {
                                               if
(DLR_IS_ENABLED(msg2->sms.dlr_mask)) {
                                                           hold_service =
msg2->sms.service;
                                                           msg2->sms.service
= octstr_format("%ld", msg2->sms.time);
                                                           msgid =
generate_smppid(msg2);
                                                           if (parts_list) {
 
msg2->sms.dlr_url = concat_msgids(msgid, parts_list);
                                                           }
 
dlr_add(box->boxc_id, msgid, msg2);
 
octstr_destroy(msgid);
 
octstr_destroy(msg2->sms.service);
                                                           msg2->sms.service
= hold_service;
                                               }
                                               uuid_unparse(msg2->sms.id,
id);
                                               msgid = octstr_create(id);
                                               dict_put(box->msg_acks,
msgid, resp);
                                               octstr_destroy(msgid);
                                               resp = NULL;
 
send_msg(box->bearerbox_connection, box, msg2);
                                               if (parts_list) {
                                                           /* destroy values
*/
 
gwlist_destroy(parts_list, msg_destroy_item);
                                               }
                                   }

                                   octstr_destroy(smsc_id); /* fix for this
leak */
                        }
                        break;

 

This is a leak which would leak a few bytes per SMS.

Index: gw/opensmppbox.c
===================================================================
--- gw/opensmppbox.c	(revision 79)
+++ gw/opensmppbox.c	(working copy)
@@ -1780,7 +1780,7 @@
 
 static Octstr *boxc_route_msg_to_smsc(Boxc *box, Msg *msg)
 {
-	Octstr *os, *smsc_id;
+	Octstr *os = NULL, *smsc_id;
 
 	if (msg->sms.smsc_id != NULL)
 		return msg->sms.smsc_id;
@@ -1788,13 +1788,16 @@
         char *receiver =  octstr_get_cstr(msg->sms.receiver);
         if ( (receiver) && (strlen(receiver) > 0) ) {
                 smsc_id = dict_get(smsc_by_receiver, msg->sms.receiver);
-	        os = octstr_format("receiver:%s", receiver);
-        };
+        }
+	else {
+		receiver = "";
+	}
 
 	if (!smsc_id) {
 	        os = octstr_format("%s:%s", octstr_get_cstr(msg->sms.sender),
 		    octstr_get_cstr(box->boxc_id));
 	        smsc_id = dict_get(smsc_by_sender_smsbox_id, os);
+		octstr_destroy(os);
         };
 	if (!smsc_id)
 		smsc_id = dict_get(smsc_by_sender, msg->sms.sender);
@@ -1805,10 +1808,8 @@
 
 	if (smsc_id)
 		debug("opensmppbox", 0, "routed msg '%s' to smsc '%s'",
-		octstr_get_cstr(os), octstr_get_cstr(smsc_id));
+		receiver, octstr_get_cstr(smsc_id));
 
-	octstr_destroy(os);
-
 	return smsc_id;
 }