[asterisk-dev] tilghman: branch 1.4 r187428 - in /branches/1.4: include/asterisk/ main/

[Russell Bryant]

SVN commits to the Digium repositories wrote:
> Author: tilghman
> Date: Thu Apr  9 13:08:20 2009
> New Revision: 187428
> 
> URL: http://svn.digium.com/svn-view/asterisk?view=rev&rev=187428
> Log:
> Race condition between ast_cli_command() and 'module unload' could cause a deadlock.
> Add lock timeouts to avoid this potential deadlock.
> (closes issue #14705)
>  Reported by: jamessan
>  Patches: 
>        20090320__bug14705.diff.txt uploaded by tilghman (license 14)
>  Tested by: jamessan
> 
> Modified:
>     branches/1.4/include/asterisk/lock.h
>     branches/1.4/main/manager.c


> Modified: branches/1.4/main/manager.c
> URL: http://svn.digium.com/svn-view/asterisk/branches/1.4/main/manager.c?view=diff&rev=187428&r1=187427&r2=187428
> ==============================================================================
> --- branches/1.4/main/manager.c (original)
> +++ branches/1.4/main/manager.c Thu Apr  9 13:08:20 2009
> @@ -2608,8 +2608,12 @@
>  int ast_manager_unregister(char *action) 
>  {
>  	struct manager_action *cur, *prev;
> -
> -	ast_rwlock_wrlock(&actionlock);
> +	struct timespec tv = { 5, };
> +
> +	if (ast_rwlock_timedwrlock(&actionlock, &tv)) {
> +		ast_log(LOG_ERROR, "Could not obtain lock on manager list\n");
> +		return -1;
> +	}
>  	cur = prev = first_action;
>  	while (cur) {
>  		if (!strcasecmp(action, cur->action)) {

I'm actually more in favor of the originally proposed patch by jamesson
on issue 14705.

As it stands, we now have two things in the code dealing with this
potential deadlock.  We have the CLI command blacklist, which jamesson
simply proposed adding "module reload" to, and now we have this timed
lock stuff.

This timed lock handling will result in less deterministic behavior.  In
some cases, module CLI commands will work from the AMI, and in other
cases they won't.  I would much rather just make it clear that it is not
supported at all by sticking with the blacklist approach.

-- 
Russell Bryant
Digium, Inc. | Senior Software Engineer, Open Source Team Lead
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org