[asterisk-dev] murf: branch 1.4 r152535 - in /branches/1.4: apps/ funcs/ include/asterisk/ res/

[Steve Murphy]

On Thu, 2008-10-30 at 12:27 -0500, Russell Bryant wrote:
> SVN commits to the Digium repositories wrote:
> > Author: murf
> > Date: Tue Oct 28 23:36:32 2008
> > New Revision: 152535
> > 
> > URL: http://svn.digium.com/view/asterisk?view=rev&rev=152535
> > Log:
> > The magic trick to avoid this crash is not to
> > try to find the channel by name in the list,
> > which is slow and resource consuming, but rather
> > to pay attention to the result codes from the
> > ast_bridge_call, to which I added the 
> > AST_PBX_NO_HANGUP_PEER_PARKED value, which
> > now are returned when a channel is parked.
> > 
> > If you get AST_PBX_KEEPALIVE,
> > then don't touch the channel pointer.
> > 
> > If you get AST_PBX_NO_HANGUP_PEER, or
> > AST_PBX_NO_HANGUP_PEER_PARKED, then don't
> > touch the peer pointer.
> > 
> > Updated the several places where the results
> > from a bridge were not being properly obeyed,
> > and fixed some code I had introduced so that
> > the results of the bridge were not overridden 
> > (in trunk).
> > 
> > All the places that previously tested for 
> > AST_PBX_NO_HANGUP_PEER now have to check for
> > both AST_PBX_NO_HANGUP_PEER and AST_PBX_NO_HANGUP_PEER_PARKED.
> 
> In this explanation, you say that that the peer channel pointer should 
> not be touched in the case of NO_HANGUP_PEER and NO_HANGUP_PEER_PARKED. 
>   However, in your code (at least in res_features), you are still 
> accessing the channel in the case of NO_HANGUP_PEER.  You must not touch 
> the channel in this case.
> 
> Also, why do you need the new return code for parked?  The only places 
> that I see handling different from the existing NO_HANGUP_PEER code are 
> where the peer channel is being accessed (when it shouldn't be).
> 


Russell--

It's good to look this situation over. 

The goal was to properly set up channel CDR's in a transfer situation,
so that the next leg had the proper start times; but it occurs to
me now, that it might be possible to move this code (well, at least
the job it wishes to do) from after the  bridge, to the transfer code 
itself. I will investigate this and  report back in this forum.
Hopefully
in the next few days. If I can replicate previous behavior, then this
should make clean up the after-bridge code considerably.



> If for CDR purposes, you _must_ be able to get data off of a channel, 
> then code must be changed so that these return codes aren't used at all. 
>   When code returns this result, it means that another thread has 
> assumed ownership of a channel, and you absolutely must not touch it 
> under any circumstance.  Otherwise, it's still going to be subject to 
> random crashes.

Yes, I agree.

By the way, I found some parking related code in main/pbx.c that I would
assume
needs to be changed to protect against illegal access after the bridge
returns AST_PBX_KEEPALIVE in __ast_pbx_run()...

-       ast_set2_flag(c, autoloopflag, AST_FLAG_IN_AUTOLOOP);
-       ast_clear_flag(c, AST_FLAG_BRIDGE_HANGUP_RUN); /* from one round
to the next, make sure this gets cleared */
-       pbx_destroy(c->pbx);
-       c->pbx = NULL;
-       if (res != AST_PBX_KEEPALIVE)
+       if (res != AST_PBX_KEEPALIVE) {
+               ast_set2_flag(c, autoloopflag, AST_FLAG_IN_AUTOLOOP);
+               ast_clear_flag(c, AST_FLAG_BRIDGE_HANGUP_RUN); /* from
one round to the next, make sure this gets cleared */
+               pbx_destroy(c->pbx);
+               c->pbx = NULL;
                ast_hangup(c);
+       }
        return 0;
 }

The interesting thing is that the present version didn't have problems
in the
various "tired" scenarios, or at least, didn't seem to to; but the
changes 
proposed above seem like a good idea to me. Am I correct?

murf

> 
-- 
Steve Murphy
Software Developer
Digium
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3227 bytes
Desc: not available
Url : http://lists.digium.com/pipermail/asterisk-dev/attachments/20081103/369ef618/attachment.bin