Re: [PATCH] Leaks and segfaults in smsbox

Hi Peter,

Peter Christensen schrieb:
> 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.

no problem, thank for patches! Just too much workload last time :(

Just read you patch and I ask me why soo many error labels? It should be 
possible to just initialize e.g. List to NULL and then try to destroy it 
every time (gwlist_destroy accepts NULL). It just a bit strange to 
follow all those labels and return paths.

What do you think about it?

Thanks,
Alex

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