On Fre, 2008-03-07 at 10:39 -0600, Russell Bryant wrote:
> Michael Neuhauser wrote:
> > 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.
> 
> <snip>
> 
> > 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?
> 
> You are absolutely correct that accessing this data without the channel locked
> is not safe.

Thank you for looking into this.

>   The code needs to be fixed to hold the channel lock when
> appropriate, or alternatively, use a lock/private copy/unlock scheme to reduce
> the amount of time the lock is held.  That does mean that the data you copied
> could change while you're still using your private copy, but in most cases that
> is ok.
>
> If you have a patch in mind, post it to bugs.digium.com.  If not, open a bug
> report on there, anyway, and someone will fix it up.

I think an initial lock/make-private-copy/unlock together with
lock/check-if-anything-has-changed-and-update/unlock before every
exist/spawn should do the trick (_softhangup handling might be a little
more complicated). But I haven't implemented anything yet. I will
definitely open a bug report.
-- 
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/