On Don, 2008-03-06 at 16:03 +0100, Michael Neuhauser wrote:
> While examining the code in __ast_pbx_run(), I've noticed something
> strange and I would really appreciate if someone could comment on this:
> Functions like ast_explicit_goto() that modify the context, exten or
> priority of an ast_channel structure do lock the channel. But it seems
> that __ast_pbx_run() is accessing these values without locking the
> channel - which seems rather hazardous, especially with
> manager-redirects/async-gotos that can happen at any time.

I've did a test that confirms my suspicion that __ast_pbx_run() does not
hold the lock of the channel and that this is a problem because channel
values that are used in this function (context, exten, priority,
_softhangup, ...) can change at any time, e.g., when a redirect is
performed over AMI.

I've added code to __ast_pbx_run() (1.4.19-rc1) to check for changes in
context, exten or priority between checking if the extension exists and
spawning it:
	/* loop on priorities in this context/exten */
        #if 0
	while (ast_exists_extension(c, c->context, c->exten, c->priority, c->cid.cid_num)) {
        #else
	while (1) {
		char context0[AST_MAX_CONTEXT], context1[AST_MAX_CONTEXT];
		char exten0[AST_MAX_EXTENSION], exten1[AST_MAX_EXTENSION];
		int prio0, prio1;
		ast_copy_string(context0, c->context, sizeof(context0));
		ast_copy_string(exten0, c->exten, sizeof(exten0));
		prio0 = c->priority;
		if (!ast_exists_extension(c, context0, exten0, prio0, c->cid.cid_num))
			break;
		/* increase likelihood of being preempted by another */
		/* thread that does an async-goto on this channel */
		usleep(1);
		ast_copy_string(context1, c->context, sizeof(context0));
		ast_copy_string(exten1, c->exten, sizeof(exten0));
		prio1 = c->priority;
		if (strcmp(context0, context1) || strcmp(exten0, exten1) || prio0 != prio1)
			ast_log(LOG_ERROR, "%s.%d@%s -> %s.%d@%s\n", exten0, prio0, context0, exten1, prio1, context1);
        #endif

Using this dialplan
        context default { test => { Answer(); jump s@test0; } }
        context test0 {	s => jump s@test0; }
        context test1 {	s => jump s@test0; }
I called the "test" extension and then did a redirect of the channel to
s.1@test1 and got the following:

[2008-03-07 13:23:41] ERROR[17256]: pbx.c:2423 __ast_pbx_run: s.1@test0 -> s.0@test1
[2008-03-07 13:23:41] NOTICE[17256]: pbx.c:1877 pbx_extension_helper: No such priority 0 in extension 's' in context 'test1'

The async-goto was executed by the manager thread while the pbx-thread
was between ast_exists_extension() and ast_spawn_extension()! I think
this shows that just accessing c->context without
lock(c)/make-private-copy/unlock(c) etc. is not OK in __ast_pbx_run().
Any comments?
-- 
Dr. Michael Neuhauser                              mailto:mike@firmix.at
Firmix Software GmbH                                  sip:mike@firmix.at
Vienna/Austria/Europe                               tel:+43-1-7890849-30
Linux Development and Services                     http://www.firmix.at/