[asterisk-dev] Locking and unlocking order in chan_local

[Kevin P. Fleming]

On 01/11/2011 09:41 AM, David Vossel wrote:
>
>
> ----- Original Message -----
>> From: "Alex Hermann"<alex at speakup.nl>
>> To: asterisk-dev at lists.digium.com
>> Cc: "Matthew Nicholson"<mnicholson at digium.com>, "David Vossel"<dvossel at digium.com>
>> Sent: Tuesday, January 11, 2011 9:31:00 AM
>> Subject: Locking and unlocking order in chan_local
>> Hello all,
>>
>>
>> I'm trying to find the cause of bug 15609 reappearing somewhere
>> between
>> 1.6.2.9 and current 1.6.2.svn. I found a suspect patch adding a
>> local_queryoption function to chan_local. I have a few questions on
>> this
>> code.
>>
>> 1) Is the locking code in asterisk designed in such a way that
>> unlocking
>> does not have to be in the reverse order of locking? Or is the code
>> (see below) faulty here?
>>
>> 2) Between an ao2_unlock of a *tech_pvt and a subsequent ao2_lock of
>> that
>> same pvt, couldn't it be the pvt has been freed? Shouldn't the pointer
>> be tested for null?

The structure pointed to by the tech_pvt pointer may have been changed 
while it was not locked by the thread in question, but if that thread 
holds a reference to the object, it will *not* have been freed. That's 
the whole point of astobj2 reference counting; it avoids the constant 
need to hold locks just to ensure that objects don't disappear unexpectedly.

However, that code is definitely buggy, in that is making a copy of a 
pointer to an ao2 object without increasing the reference count, and 
thus it is possible for the object to disappear. The initialization of 
'p' should be done using 'ao2_ref(ast->tech_pvt, 1)', and then 
'ao2_ref(p, -1)' should be called when p is no longer needed (including 
all exit paths from the function).
-- 
Kevin P. Fleming
Digium, Inc. | Director of Software Technologies
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
skype: kpfleming | jabber: kfleming at digium.com
Check us out at www.digium.com & www.asterisk.org