[Asterisk-Dev] Signal timing can bring down Asterisk

[Hans Petter Selasky]

On Tuesday 16 August 2005 09:02, Kristian Nielsen wrote:
> Hans Petter Selasky <hselasky at c2i.net> writes:
> > Hi,
> >
> > I'm writing a channel driver for asterisk and I have hit some problems.
> >
> > 1)
> >
> > I want to have my channel driver under a BSD license, but I see no
> > ASTERISK_BSD_KEY. Must all code be under GPL or is BSD also allowed?
>
> The question is whether your code could be considered to be a derived
> work of Asterisk. If it can, then to distribute it you must have the
> permission from Digium, which you can have either by distributing under
> the GPL, or by special agreement. This also depends on how copyright
> works in your country, of course.
>
> One could argue that any channel driver is so derived from Asterisk
> because of the tight integration (struct ast_channel_tech ...). The
> ASTERISK_GPL_KEY also makes a strong case for being derived work (though
> you would be free to distribute a GPL Asterisk without the
> ASTERISK_GPL_KEY along with your module).
>
> So as I see it, _if_ your code is a derived work of Asterisk, you can't
> use BSD license without special permission from Digium. I doubt anyone
> will be able to give you a definitive answer as to whether it is so
> derived or not.
>
> > What happens if two different threads call "ast_hangup()" at the same
> > time on the same "ast_channel" ?
>
> I believe this won't happen. I think the ast_hangup() call is meant to
> free the channel, being dual to ast_call(). User hangup events are
> signalled by passing a hangup event to ast_write(), or by returning one
> from ast_read(). Check f.ex. the source for the "Dial" application, that
> should show you how it is used exactly.

I see some channel drivers using "ast_softhangup()".

This function has no safe entrypoint into "ast_channel".

int ast_softhangup(struct ast_channel *chan, int cause)
{
        int res;
        ast_mutex_lock(&chan->lock);
        res = ast_softhangup_nolock(chan, cause);
        ast_mutex_unlock(&chan->lock);
        return res;
}

Maybe something like this would be better, though not complete:

int ast_softhangup(struct ast_channel *chan, struct ast_mutex *mtx, int cause)
{
        int res;
        ast_mutex_lock(mtx);
#if 0
        Hence one mutex is only protecting one structure, why not
        put the pointer to that structure into the mutex structure? Then
        we save passing that parameter. Or maybe we can check that
        "chan == mtx->priv_sc". That would be a resonable check, though 
        not perfect. Another thing is that we can put a refcount in 
        "struct ast_mutex". Then a copy of that refcount is passed to this
        function from the caller, and when we get here, we just compare that
        the refcount is right. Then instead of passing "ast_channel" we pass a
        mutex and a copy of that refcount ?

        chan = mtx->priv_sc;
        if(refcount == mtx->priv_refcount) OK else WRONG;

        Checking a refcount is much faster than doing a linear search 
        for this structure, and involves no second lock !
#endif

        if( xxx == yyy)
        {
            res = ast_softhangup_nolock(chan, cause);
        }
        ast_mutex_unlock(mtx);
        return res;
}

But, even though the hangup is returned as a NULL frame, from "ast_read" that 
won't solve the problem, because there are more signals than that.

Consider this for example. I have a separate structure "struct call_desc" 
associated with every "struct ast_channel". Now, at first I thought that I 
could use "pch->lock" but that lock is destroyed, and I have no entry point 
into my "struct call_desc". So I have put this lock under its own lock, 
"cd->mtx".

The problem is that Asterisk calls the callbacks in the channel drivers with 
"pch->lock" locked. Then, in my callback, I have to lock "cd->mtx" to be able 
to read/write this structure. You probably know that the locking order cannot 
be reversed, hence one will be going for a deadlock. So when my driver is 
calling Asterisk, the code has to look like this:


lock(cd->mtx);
xxx code;
pch = cd->pch;
unlock(cd->mtx);

Now, what happens here if "pch->lock" is already contested. Won't this code 
sleep, and if Asterisk is doing a hangup, what will happen then? Well one 
solution is to hold "cd->mtx" while reading the value of "alertpipe" and make 
sure that this value is only changed when both "cd->mtx" and "pch->lock" is 
held. Or maybe I can store this value in "struct call_desc". That would work.

/* wakeup read thread */

lock(pch->lock);

(void) write(pch->pvt->alertpipe[1], "", 1);

unlock(pch->lock);






Here is another example where I cannot hold "cd->mtx":

    case EV_CONNECTED:
      pch = cd->pch;

      ast_mutex_unlock(&cd->mtx);
      cd = NULL;

      Now, what happens here if "pch->lock" is contested and 
      Asterisk is doing a hangup.

      ast_mutex_lock(&pch->lock);
      ast_setstate(pch, AST_STATE_UP);
      ast_mutex_unlock(&pch->lock);



So what is missing is a safe entrypoint into "struct ast_channel". This means 
that the mutex must be static, and there must be some static refcount, per 
struct ast_channel, which value can be checked before continuing. Else one 
ends up with a single-locked system. The pointer to the mutex, refcount and a 
copy of its value, must be stored in a structure like my "struct call_desc".


Can the example above be done in some other way, so that I avoid these 
problems?


Any comments ?


--HPS