[asterisk-dev] [svn-commits] jrose: branch 1.8 r333265 - /branches/1.8/res/res_jabber.c

[Jonathan Rose]

Looking over this again, it seems to me like the patch was confusing the
variables client->status and client->state, thinking they alluded to the
same thing.  The casting thing isn't necessary at all, instead it looks
like the patch should just be using the state variable instead.

A rudimentary check shows that status and state had the same numerical
value at the time I was getting the segfaults at least.  In the case of
state, this makes sense.  It's AJI_DISCONNECTED.  With the ikshowtype
however it isn't so obvious, it's IKS_SHOW_AVAILABLE, which would imply
to me the opposite of the what's causing the situational problem.

I'm going to go ahead and commit the change from status to to state.

On Mon, 2011-08-29 at 10:20 -0500, Kevin P. Fleming wrote:
> On 08/29/2011 09:08 AM, Jonathan Rose wrote:
> > On Sat, 2011-08-27 at 19:46 -0400, Paul Belanger wrote:
> >> On 11-08-27 05:16 PM, Tzafrir Cohen wrote:
> >>> On Thu, Aug 25, 2011 at 06:47:45PM -0000, SVN commits to the Digium repositories wrote:
> >>>> Author: jrose
> >>>> Date: Thu Aug 25 13:47:42 2011
> >>>> New Revision: 333265
> >>>>
> >>>> URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=333265
> >>>> Log:
> >>>> Segfault when publishing device states via XMPP and not connected
> >>>>
> >>>> When using publishing device state with res_jabber, Asterisk will attempt
> >>>> to send a device state using the unconnected client using iks_send_raw
> >>>> and crash. This patch checks the validity of the connection before
> >>>> attempting to send the device state.
> >>>>
> >>>> (closes issue ASTERISK-18078)
> >>>> Reported by: Michael L. Young
> >>>> Patches:
> >>>>         res_jabber-segfault-pubsub-not-connected2.patch (license #5026) patch uploaded by Michael L. Young
> >>>> Tested by: Jonathan Rose
> >>>>
> >>>>
> >>>> Modified:
> >>>>       branches/1.8/res/res_jabber.c
> >>>>
> >>>> Modified: branches/1.8/res/res_jabber.c
> >>>> URL: http://svnview.digium.com/svn/asterisk/branches/1.8/res/res_jabber.c?view=diff&rev=333265&r1=333264&r2=333265
> >>>> ==============================================================================
> >>>> --- branches/1.8/res/res_jabber.c (original)
> >>>> +++ branches/1.8/res/res_jabber.c Thu Aug 25 13:47:42 2011
> >>>> @@ -1465,7 +1465,15 @@
> >>>>    #endif
> >>>>    	/* If needed, data will be sent unencrypted, and logHook will
> >>>>    	   be called inside iks_send_raw */
> >>>> -	ret = iks_send_raw(client->p, xmlstr);
> >>>> +	if((client->timeout != 0&&   client->status == AJI_CONNECTED) || (client->status == AJI_CONNECTING))
> >>>
> >>> This breaks building in dev-mode. client->status and AJI_CONNECTING /
> >>> AJI_CONNECTED are of different types (two different enum-s). An explicit
> >>> cast will fix this. Any problem with that?
> >>>
> >> Hmm, I'm surprised bamboo did not catch this.  I assume gcc is complaining?
> >>
> >
> > I was working with dev mode while I was testing this, so I assume that
> > means something more elaborate is at play here.  I've tested to make
> > sure it builds on both 32 and 64 bit Ubuntu, so maybe it has something
> > to do with a difference in our versions of libiksemel?  Well,
> > regardless, it's best to make sure this works with your configuration
> > since it was working before and I certainly have no issue with casting
> > to make sure the variables we compare are using the same enumerator
> > types, so I'll fix it.
> >
> > Just a quick confirmation, this should be a change of
> >
> > if((client->timeout != 0&&  client->status == AJI_CONNECTED) ||
> > (client->status == AJI_CONNECTING))
> >
> > to
> >
> > if ((client->timeout != 0&&  client->status == (enum ikshowtype)
> > AJI_CONNECTED || (client->status == (enum ikshowtype) AJI_CONNECTING))
> >
> > Is this correct?  I can't check within 100% certainty on my boxes since
> > it compiles in dev mode regardless (and I'm still slightly new, so
> > confirmation is nice).
> 
> A better question would be 'why is the "status" member of "struct 
> aji_client" of type "enum ikshowtype" when we store values from "enum 
> aji_state" in it?'
> 
> Casting enums does not solve the problem; if the value AJI_CONNECTING is 
> not valid for 'enum ikshowtype', the compiler is still going to 
> complain. The better solution is to only store valid enumerators for a 
> given enum type in a variable of that type.
>