Re: [PATCH] Leaks and segfaults in smsbox

Hi,

No opinions or comments at all on this?! Or am I just posting too many 
patches to the mailing list? If so, I apologize. I just feel that it is 
my duty as user of kannel, to share the bugs I find, and their 
corresponding patches - and memory leaks and segmentation faults are 
particular of my concern, as they lower the stability of kannel as a 
whole. I have fought quite a few fights with kannel where servers went 
totally unreachable due to out-of-memory conditions or extreme CPU 
loads, and although the leaks addressed by this patch are minor, it is 
unfortunate when smsbox crashes, no matter how odd the conditions 
required for the crash might be.

Med venlig hilsen / Best regards

Peter Christensen

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

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


Peter Christensen wrote:
> Hi,
> 
> In relation to the thread "Memory leak in smsbox", here's a patch which 
> corrects some of the errors.
> 
> The patch reorganizes the error goto-fields so they are as followed:
> 
> fielderror1:
>  At this point, only receiver have been allocated
> 
> fielderror2:
>  receiver, allowed, and denied have been allocated (allowed and denied 
> may contain elements from receiver)
> 
> fielderror3:
>  receiver, allowed, denied, newfrom, and msg have been allocated 
> (allowed and denied contains elements from receiver)
> 
> error:
>  receiver, allowed, denied, newfrom, and failed_id have been allocated 
> (allowed is empty, and denied and failed_id may contain elements from 
> receiver)
> 
> Besides, the "Number(s) has/have been denied by white- and/or 
> black-lists." error have been moved to before SMS transmission is 
> attempted, and the logic is now gwlist_len(allowed) == 0 (No recipient 
> have been allowed)
> 
> This patch overrules my original patch for the memory leak.
> 
> For the record; While monitoring its memory consumption, I've flooded 
> smsbox with messages for each possible error condition (except 
> conditions where recipients are added to the failed_id list. Don't know 
> how to provoke that condition), and there seems to be no memory leaks, 
> double frees, or segmentation faults.
> 
> 
> ------------------------------------------------------------------------
> 
> diff -Nru gateway/gw/smsbox.c gateway.smsbox/gw/smsbox.c
> --- gateway/gw/smsbox.c	2005-12-09 03:14:31.000000000 +0100
> +++ gateway.smsbox/gw/smsbox.c	2006-01-23 17:51:55.000000000 +0100
> @@ -1990,11 +1990,11 @@
>       */
>      if (udh != NULL && (octstr_len(udh) != octstr_get_char(udh, 0) + 1)) {
>          returnerror = octstr_create("UDH field misformed, rejected");
> -        goto fielderror2;
> +        goto fielderror1;
>      }
>      if (udh != NULL && octstr_len(udh) > MAX_SMS_OCTETS) {
>          returnerror = octstr_create("UDH field is too long, rejected");
> -        goto fielderror2;
> +        goto fielderror1;
>      }
>  
>      /*
> @@ -2108,6 +2108,12 @@
>          del = gwlist_delete_matching(allowed, receiv, octstr_item_match);
>      }
>  
> +    /* have all receivers been denied by list rules?! */
> +    if (gwlist_len(allowed) == 0) {
> +        returnerror = octstr_create("Number(s) has/have been denied by white- and/or black-lists.");
> +        goto fielderror2;
> +    }
> +
>      if (urltrans_faked_sender(t) != NULL) {
>  	/* discard previous from */
>  	newfrom = octstr_duplicate(urltrans_faked_sender(t));
> @@ -2144,7 +2150,7 @@
>  	    msg->sms.account = account ? octstr_duplicate(account) : NULL;
>  	} else {
>  	    returnerror = octstr_create("Account field misformed, rejected");
> -	    goto fielderror;
> +	    goto fielderror3;
>  	}
>      }
>      msg->sms.msgdata = text ? octstr_duplicate(text) : octstr_create("");
> @@ -2156,7 +2162,7 @@
>      if(octstr_len(dlr_url)) {
>  	if(octstr_len(dlr_url) < 8) { /* http(s):// */
>  	    returnerror = octstr_create("DLR-URL field misformed, rejected");
> -	    goto fielderror;
> +	    goto fielderror3;
>  	} else {
>  	    Octstr *tmp;
>  	    tmp = octstr_copy(dlr_url, 0, 7);
> @@ -2168,7 +2174,7 @@
>  		if(octstr_case_compare(tmp, octstr_imm("https://")) != 0) {
>  		    returnerror = octstr_create("DLR-URL field misformed, rejected");
>  		    O_DESTROY(tmp);
> -		    goto fielderror;
> +		    goto fielderror3;
>  		}
>  #ifdef HAVE_LIBSSL
>  		msg->sms.dlr_url = octstr_duplicate(dlr_url);
> @@ -2187,49 +2193,49 @@
>  
>      if ( dlr_mask < -1 || dlr_mask > 31 ) { /* 00011111 */
>  	returnerror = octstr_create("DLR-Mask field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.dlr_mask = dlr_mask;
>      
>      if ( mclass < -1 || mclass > 3 ) {
>  	returnerror = octstr_create("MClass field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.mclass = mclass;
>      
>      if ( pid < -1 || pid > 255 ) {
>  	returnerror = octstr_create("PID field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.pid = pid;
>  
>      if ( rpi < -1 || rpi > 2) {
>  	returnerror = octstr_create("RPI field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.rpi = rpi;
>      
>      if ( alt_dcs < -1 || alt_dcs > 1 ) {
>  	returnerror = octstr_create("Alt-DCS field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.alt_dcs = alt_dcs;
>      
>      if ( mwi < -1 || mwi > 7 ) {
>  	returnerror = octstr_create("MWI field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.mwi = mwi;
>  
>      if ( coding < -1 || coding > 2 ) {
>  	returnerror = octstr_create("Coding field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.coding = coding;
>  
>      if ( compress < -1 || compress > 1 ) {
>  	returnerror = octstr_create("Compress field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.compress = compress;
>  
> @@ -2244,19 +2250,19 @@
>  
>      if ( validity < -1 ) {
>  	returnerror = octstr_create("Validity field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.validity = validity;
>  
>      if ( deferred < -1 ) {
>  	returnerror = octstr_create("Deferred field misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>      msg->sms.deferred = deferred;
>      
>      if (priority != SMS_PARAM_UNDEFINED && (priority < 0 || priority > 3)) {
>          returnerror = octstr_create("Priority field misformed, rejected");
> -        goto fielderror;
> +        goto fielderror3;
>      }
>      msg->sms.priority = priority;
>  
> @@ -2279,7 +2285,7 @@
>  
>      if (charset_processing(charset, msg->sms.msgdata, msg->sms.coding) == -1) {
>  	returnerror = octstr_create("Charset or body misformed, rejected");
> -	goto fielderror;
> +	goto fielderror3;
>      }
>  
>      msg->sms.receiver = NULL;
> @@ -2322,19 +2328,12 @@
>  
>  
>      msg_destroy(msg);
> -    gwlist_destroy(receiver, octstr_destroy_item);
> -    gwlist_destroy(allowed, octstr_destroy_item);
> -
> -    /* have all receivers been denied by list rules?! */
> -    if (no_recv == gwlist_len(denied)) {
> -        returnerror = octstr_create("Number(s) has/have been denied by white- and/or black-lists.");
> -        goto fielderror2;
> -    }
>  
>      if (gwlist_len(failed_id) > 0)
>  	goto error;
>      
> -    gwlist_destroy(failed_id, octstr_destroy_item);
> +    gwlist_destroy(failed_id, NULL);
> +    gwlist_destroy(allowed, NULL);
>      octstr_destroy(newfrom);
>      *status = HTTP_ACCEPTED;
>      returnerror = octstr_create("Sent.");
> @@ -2348,8 +2347,9 @@
>          while ((receiv = gwlist_extract_first(denied)) != NULL) {
>              octstr_format_append(returnerror, " %s", octstr_get_cstr(receiv));
>          }
> -    }               
> -    gwlist_destroy(denied, octstr_destroy_item);  
> +    }
> +    gwlist_destroy(denied, NULL);
> +    gwlist_destroy(receiver, octstr_destroy_item);
>  
>      /*
>       * Append number of splits to returned body. 
> @@ -2361,11 +2361,17 @@
>      return returnerror;
>      
>  
> -fielderror:
> -    octstr_destroy(newfrom);
> +fielderror3:
>      msg_destroy(msg);
> -
> +    octstr_destroy(newfrom);
> +    
>  fielderror2:
> +    gwlist_destroy(allowed, NULL);
> +    gwlist_destroy(denied, NULL);
> +
> +fielderror1:
> +    gwlist_destroy(receiver, octstr_destroy_item);
> +    
>      alog("send-SMS request failed - %s",
>           octstr_get_cstr(returnerror));
>  
> @@ -2374,7 +2380,6 @@
>  
>  error:
>      error(0, "sendsms_request: failed");
> -    octstr_destroy(from);
>      *status = HTTP_INTERNAL_SERVER_ERROR;
>      returnerror = octstr_create("Sending failed.");
>  
> @@ -2389,9 +2394,11 @@
>          }
>      }
>  
> -    octstr_destroy(receiv); 
> -    gwlist_destroy(failed_id, octstr_destroy_item);
> -    gwlist_destroy(denied, octstr_destroy_item);
> +    gwlist_destroy(failed_id, NULL);
> +    gwlist_destroy(allowed, NULL);
> +    gwlist_destroy(denied, NULL);
> +    gwlist_destroy(receiver, octstr_destroy_item);
> +    octstr_destroy(newfrom);
>      return returnerror;
>  }
>