Re: gnokii-0.6.30 patch

[Pawel Kot]

Hi,

On Wed, Oct 19, 2011 at 14:22, Alexander V. Lukyanov <address@hidden> wrote:
> On Wed, Oct 19, 2011 at 12:22:55PM +0200, Pawel Kot wrote:
>> On Wed, Oct 19, 2011 at 11:29, Alexander V. Lukyanov <address@hidden> wrote:
>> > On Wed, Oct 19, 2011 at 10:28:51AM +0200, Pawel Kot wrote:
>> >> --- gnokii-0.6.30/common/phones/nk6510.c      2011-01-23 
>> >> 17:27:32.000000000 +0300
>> >> +++ gnokii-0.6.30+/common/phones/nk6510.c     2011-03-11 
>> >> 14:02:25.000000000 +0300
>> >> +             if(!data->message_center)
>> >> +                     data->message_center = calloc(1, 
>> >> sizeof(data->message_center[0]));
>> >>
>> >> No, I want to avoid it. If (!data->message_center) it should exit with
>> >> error in this place. It is a responsibility of the caller to allocate
>> >> it.
>> >
>> > Ok, then the caller should be fixed.
>>
>> Caller looks good to me:
>>       dt->message_center = calloc (1, sizeof (gn_sms_message_center));
>>       dt->message_center->id = 1;
>>       if ((status = gn_sm_functions (GN_OP_GetSMSCenter, dt, sm)) ==
>> GN_ERR_NONE)
>
> I don't think it is the only caller of that function.

The only one within smsd as far as I can tell.

>> Do you have the backctrace and log when it fails?
>> >>      free (dt->message_center);
>> >> +    dt->message_center = 0;
>> >>
>> >> I'd prefer NULL instead of 0. But that should not matter actually. We
>> >> allocate the structure unconditionally.
>
> I think it is good to zero free'd pointers to avoid subtle problems.

Fair enough. But I'd better see where freed pointers are reused
without proper allocation...

>> >>
>> >>    free (dt->sms->reference);
>> >> +  dt->sms->reference=0;
>> >> +  dt->sms->parts=0;
>> >>    free (dt);
>> >>
>> >> Does not make sense as we free() it right after.
>> >
>> > It's not quite so. dt->sms is not free'd.
>>
>> Right. Because it is static. And no need to zero the parts then. Did
>> you have any problems because of that?
>
> Yes, I've had problems with touching free'd memory. I tracked the bug using
> ElectricFence. The static structure was reused without proper clearing.

Well, what I see is that the full structure is always zeroed before
reusing. But, again, agree that zeroing pointer after freeing does not
hurt.

> I use gnokii-smsd for sending SMS from mysql DB (1000-1500 per month) and
> sometimes I get incoming SMS. Before the patching it failed regularly.

Still, would advice using the git version. I'll submit a subset of
your patch today.

thanks,
-- 
Pawel Kot