Re: [BUG] Smsbox crash with empty messages

Bruno David Rodrigues wrote:
> 
> ----- Original Message -----
> From: "Aarno Syvänen" <[email protected]>
> Cc: "Kannel-devel (E-mail)" <[email protected]>
> Sent: Monday, March 04, 2002 3:53 PM
> Subject: Re: [BUG] Smsbox crash with empty messages
> 
> > Bruno David Rodrigues wrote:
> > >
> > > ----- Original Message -----
> > > From: "Stipe Tolj" <[email protected]>
> > > To: "Bruno David Rodrigues" <[email protected]>
> > > Cc: <[email protected]>
> > > Sent: Monday, March 04, 2002 12:49 PM
> > > Subject: Re: [BUG] Smsbox crash with empty messages
> > >
> > > > Bruno David Rodrigues wrote:
> > > > >
> > > > > smsbox panics on smsbox line 1180 because of a empty text field
> (NULL):
> > > > >
> > > > > http://www.kannel.3glab.org/cgi-bin/viewcvs.cgi/gateway/gw/smsbox.c?
> > > > > annotate=1.174#1180
> > > > >
> > > > > In patch 1.164 at 2002/01/25:
> > > > >  udh == NULL ? ( text == NULL ? "" : octstr_get_cstr(text) ) : "<<
> UDH
> > > >>");
> > > > >
> > > > >  udh == NULL ? ( text == NULL ? "" : octstr_get_cstr(text) ) :
> > > > >                 octstr_get_cstr(text));
> > > > >
> > > > > Why ?!
> > > > > I'm reverting this line
> > > >
> > > > octstr_get_cstr() requires an non-NULL octstr pointer.
> > > >
> > > > So if text == NULL then octstr_get_cstr() throws the assertion error
> > > > and panics the smsbox.
> > > >
> > > > BTW, in the second statement you are not garanteeing that call to
> > > > octstr_get_cstr() does not have text == NULL, it's only handled in the
> > > > case udh == NULL.
> > >
> > > :P
> > >
> > > I asked why did syvanen changed the code like that !
> > >
> > > I know what octstr_get_cstr does ;), that's why I've looked everywhere
> for
> > > lines using it and tried to be careful and inserted the (text == NULL ?
> "" :
> > > octstr_get_cstr(text)) back when I added the possibility of empty
> messages
> > > to smsbox.
> > >
> > > And it this particular line, we've talked about that, in general, when
> you
> > > have
> > > a udh, the text probably is binary (there weren't EMS back then), so it
> > > was not necessary to print the text when exists a udh. I guess syvanen
> > > thought in printing the text even if there is a udh.
> >
> > Actually syvanen did not think *anything* :(( You should *not* commit
> > any-
> > thing like that. And I think that we should add empty message tests to
> > all
> > test scripts. Make check is supposed to catch errors like this.
> >
> > aarno
> 
> What should I NOT commit ?
> I've changed the line to this:
> 
>     info(0, "sendsms sender:<%s:%s> (%s) to:<%s> msg:<%s>",
>          octstr_get_cstr(urltrans_username(t)),
>          octstr_get_cstr(newfrom),
>          octstr_get_cstr(client_ip),
>          octstr_get_cstr(to),
>          ( text == NULL ? "" : octstr_get_cstr(text) ));
> 
> I'm sorry, I'm not acusing you, but it's frustrating to have been working
> for some days
> looking for every piece of code that could crash with empty messages, and
> now,
> at least two month later, the bug reappears.

Sorry being misleading. *You* can commit your proposal, it was *I* that
should
not have committed the bug you kindly corrected. You can even accuse me
for 
doing this kind of thing.

And yes, I will add a test for empty messages, and NULL messages to
check_sendsms
script.

aarno