[asterisk-dev] Locking and unlocking order in chan_local

[Alex Hermann]

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