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

[Kevin P. Fleming]

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.

-- 
Kevin P. Fleming
Digium, Inc. | Director of Software Technologies
Jabber: kfleming at digium.com | SIP: kpfleming at digium.com | Skype: kpfleming
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at www.digium.com & www.asterisk.org