[Asterisk-Dev] Re: Fundamental locking (race/deadlock) problem in ast_hangup()

[Tony Mountifield]

In article <200508261545.55792.hselasky at c2i.net>,
Hans Petter Selasky <hselasky at c2i.net> wrote:
> 2) The lock in "struct ast_channel" must go away. Instead there must be:
> 
> struct ast_channel {
>  struct ast_mutex *priv_lock; // the lock that protects the channel driver

The above suggests priv_lock is per-channel...

>  struct ast_mutex *ast_lock; // the lock that is used by Asterisk
>  u_int8_t refcount; // structure reference count
>  u_int8_t present; // set if hangup has not been called
>  ...
> };
> 
> 3) The locking order is defined like this.
> 
>   chan->priv_lock first
>   chan->ast_lock  second
> 
> 
> 4) In the channel driver:
> 
> lock(priv_lock);

But this suggests priv_lock is global to all channels. Which is it?

> chan = lookup ast_channel in list;
> 
>   lock(chan->ast_lock);
> 
>     call a Asterisk callback ;
> 
>     // if this is hangup
>     // {
>     //   chan->present = 0;
>     //   chan->refcount--;
>     // }
>     //
> 
>   refcount = chan->refcount;
> 
>   unlock(chan->ast_lock);
> 
>   if(refcount == 0) 
>   {
>      free(chan);
>   }
> 
> unlock(priv_lock);

As does this.

> Comment to 4):
> 
> In this scheme it is important that the channel driver has its lock locked 
> from it looks up "struct ast_channel", and until after that Asterisk has been 
> called, if that is needed.
> 
> 
> 5) In asterisk:
> 
> lock(ast_lock);
> 
> chan = lookup ast_channel in list;
> 
> chan->refcount++;
> 
> unlock(ast_lock);
> 
> // NOTE: if we sleep here, and 4) calls hangup
> // chan->present will be set to 0 !
> //
> 
> lock(chan->priv_mtx);

What's priv_mtx? A typo for priv_lock?

Cheers
Tony
-- 
Tony Mountifield
Work: tony at softins.co.uk - http://www.softins.co.uk
Play: tony at mountifield.org - http://tony.mountifield.org