[Asterisk-Dev] what can i do on an unlocked ast_channel ?

[Luigi Rizzo]

as a followup to myself... can you people have a look at the last note in
http://bugs.digium.com/view.php?id=4265    (bugnote id 27249)
as a possible deadlock-free solution to channel locking ?

The relevant text is attached below for convenience:
	cheers
	luigi

----------------------

One possibility would be the introduction of refcounts in the
the ast_channel objects, protected by either chan->lock        
or (better) by 'chlock' so that chan->lock can be left to
its intended (i presume) purpose of implementing 'ownership'
of the object.

Then we would have the following interface, a lot simpler and 
without risks of deadlock.

    /* increments the refcount to create a new reference */
    ast_channel_addref(chan) {
        ast_mutex_lock(&chlock);
        chan->refcount++;
        ast_mutex_unlock(&chlock);
    }

    /* decrement the refcount to return a reference */
    ast_channel_remref(chan) {
        ast_mutex_lock(&chlock);
        i = --chan->refcount;
        ast_mutex_unlock(&chlock);
        if (i == 0)  
            ast_channel_free(chan); /* delete on last ref. */
    }

    /* optimized walk function, returns a new reference */
    ast_channel_walk(prev) {
        ast_mutex_lock(&chlock);
        if (prev == NULL) /* we want the first item */
            c = channels;
        else { 
            /* make sure 'prev' still on the list */
            for (c = channels; c && c != prev; c = c->next)  
                ;
            if (c) /* yes, so get the next element */
                c = c->next;
        }
        if (c)  /* the object exists, increment the refcount */
            c->refcount++;
        ast_mutex_unlock(&chlock);
        return c;
    }      

ast_channel_find_by_name(name) is similarly simple.

It is up to the caller to keep or release the reference on 'prev'.
It can release it even before calling ast_channel_walk() because
once again, prev is never dereferenced.

--------------------
On Sat, May 14, 2005 at 07:30:25AM -0700, Luigi Rizzo wrote:
> I was wondering what is the locking scheme used for struct ast_channel.
> I cannot find anything in the source (channel.h, channel.c) except
> figuring out (from the code) that:
> 
>  - chlock protects the two lists 'channels' and 'backends'.
>    So if I hold chlock, nobody else should free() any of the objects
>    on the 'channels' list (not sure about 'backends' though
>    maybe there is no contention on those objects by separate threads.
> 
>  - c->lock "lock a channel for some operations" (from channel.h)
> 
> I have no idea of the meaning of the latter.
> But looking at ast_channel_free() it seems that
> there is a risk that a struct ast_channel can get freed if
> noone holds that mutex.
> 
> The question arises because there is some code in e.g.
> apps/app_chanspy.c::local_channel_walk() that returns a
> pointer to an unlocked channel, and then dereferences it
> without apparently holding any lock. How do we guarantee
> that this does not result in dereferencing an invalid pointer ?
> Perhaps because it is guaranteed that such ops and
> the call to ast_channel_free() cannot occur in different
> threads ?
> 
> 	cheers
> 	luigi
> _______________________________________________
> Asterisk-Dev mailing list
> Asterisk-Dev at lists.digium.com
> http://lists.digium.com/mailman/listinfo/asterisk-dev
> To UNSUBSCRIBE or update options visit:
>    http://lists.digium.com/mailman/listinfo/asterisk-dev