Re: gnokii-0.6.30 patch

[Alexander V. Lukyanov]

On Wed, Oct 19, 2011 at 02:40:01PM +0200, Pawel Kot wrote:
> 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.

This is the real caller (gsm-statemachine.c:126). The function
NK6510_IncomingSMS is part of incoming_functions. As you see,
message_center is not allocated here.
        /* Pass up the message to the correct phone function, with data if 
necessary */
        c = 0;
        while (state->driver.incoming_functions[c].functions) {
                if (state->driver.incoming_functions[c].message_type == 
messagetype) {
                        dprintf("Received message type %02x\n", messagetype);
                        res = 
state->driver.incoming_functions[c].functions(messagetype, message, 
messagesize, data, state);
                        temp = 0;
                        break;
                }
                c++;
        }
        
> >> 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...

It is easier to find by setting them to zero or some poison.

> > 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.

It is reused in this loop (mysql.c:384):
    do
    {
      error = WriteSMS (&sms);
      sleep (1);
    }
    while ((error == GN_ERR_TIMEOUT || error == GN_ERR_FAILED) && numError++ < 
3);

The same in file.c, pq.c, sqlite.c.

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

I'll try the git version and report the results.

-- 
   Alexander.