[asterisk-dev] [asterisk-commits] mmichelson: trunk r83350 - in /trunk: ./ apps/app_queue.c

[Luigi Rizzo]

On Thu, Sep 20, 2007 at 09:21:29PM -0000, SVN commits to the Asterisk project wrote:
> Author: mmichelson
> Date: Thu Sep 20 16:21:28 2007
> New Revision: 83350
> 
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=83350
> Log:
> Merging changes from queue_refcount_trunk into trunk. Refcounted queues now in place.

just randomly looking at the diff, there seem to be a couple
of incorrect patterns of usage of refcounts .This change is rather
extensive so i only point out two examples, but i would encourage
you to look at the ordering of ao2_unref/ao2_unlock and ao2_link/ao2_ref
(both wrong: almost surely you need unlock/unref and ref/link) 
in your code.

The first one is shown below (but i have seen at least another instance):
once you release your reference with queue_unref(), you are not
guaranteed that the object exists anymore so you cannot use the
pointer anymore.

> @@ -628,9 +653,9 @@
>  			}
>  			ao2_ref(cur, -1);
>  		}
> -		ast_mutex_unlock(&q->lock);
> -	}
> -	AST_LIST_UNLOCK(&queues);
> +		queue_unref(q);
> +		ao2_unlock(q);
> +	}
>  
>  	return NULL;
>  }

A second problem below: ao2_link() implicitly passes the reference
to the container, so you are not guaranteed access to the object
after that. queue_ref() must be done before the ao2_link()

> @@ -1207,11 +1228,12 @@
>  	if (!q) {
>  		if (!(q = alloc_queue(queuename)))
>  			return NULL;
> -		ast_mutex_lock(&q->lock);
> +		ao2_lock(q);
>  		clear_queue(q);
>  		q->realtime = 1;
>  		init_queue(q);		/* Ensure defaults for all parameters not set explicitly. */
> -		AST_LIST_INSERT_HEAD(&queues, q, list);
> +		ao2_link(queues, q);
> +		queue_ref(q);
>  	}
>  
>  	memset(tmpbuf, 0, sizeof(tmpbuf));


there are probably more instances like these two.

	cheers
	luigi