[asterisk-dev] Locking and unlocking order in chan_local

[David Vossel]

----- 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?
> 
> See comments inline in the code for details.
> 
> 
> 
> static int local_queryoption(struct ast_channel *ast, int option, void
> *data, int *datalen)
> {
> struct local_pvt *p = ast->tech_pvt;
> struct ast_channel *chan, *bridged;
> int res;
> 
> if (!p) {
> return -1;
> }
> 
> if (option != AST_OPTION_T38_STATE) {
> /* AST_OPTION_T38_STATE is the only supported option at this time */
> return -1;
> }
> 
> ao2_lock(p);
> chan = IS_OUTBOUND(ast, p) ? p->owner : p->chan;
> 
> try_again:
> if (!chan) {
> ao2_unlock(p);
> return -1;
> }
> 
> if (ast_channel_trylock(chan)) {
> ao2_unlock(p);
> sched_yield();
> =========== Shouldn't p be tested for null here? ==============
> ao2_lock(p);
> chan = IS_OUTBOUND(ast, p) ? p->owner : p->chan;
> goto try_again;
> }
> 
> bridged = ast_bridged_channel(chan);
> if (!bridged) {
> /* can't query channel unless we are bridged */
> =========== Reverse unlocking ? ==============
> ao2_unlock(p);
> ast_channel_unlock(chan);
> ========================
> return -1;
> }
> 
> if (ast_channel_trylock(bridged)) {
> ast_channel_unlock(chan);
> ao2_unlock(p);
> sched_yield();
> =========== Shouldn't p be tested for null here? ==============
> ao2_lock(p);
> chan = IS_OUTBOUND(ast, p) ? p->owner : p->chan;
> goto try_again;
> }
> 
> res = ast_channel_queryoption(bridged, option, data, datalen, 0);
> =========== Reverse unlocking ? ==============
> ao2_unlock(p);
> ast_channel_unlock(chan);
> ast_channel_unlock(bridged);
> ========================
> return res;
> }
> 
> --
> Greetings,
> 
> Alex Hermann

Unlock order does not matter here.  You are correct about the NULL checks though. 

David Vossel
Digium, Inc. | Software Developer, Open Source Software
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org
The_Boy_Wonder in #asterisk-dev