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

[Mark Michelson]

Russell Bryant wrote:
> Mark Michelson wrote:
>>>> -       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;
>>>>  }
>>>>
> 
>>> You are correct that the existing code is not safe.  However, in the 
>>> existing code, you have the potential for a crash.  In your modified 
>>> version, you have a memory leak, as the ast_pbx isn't getting cleared up 
>>> like it is supposed to.
>>>
>> For my education, where is the memory leak? What else is required besides 
>> pbx_destroy()?
> 
> Previously, when code returned KEEPALIVE, it was expecting the PBX 
> thread to destroy the channel's ast_pbx object.  In this modified code, 
> pbx_destroy() is no longer being called.  The catch is, it's really not 
> safe to touch the channel at all here ...
> 

Ah, my mind had turned != into ==. Thanks.

Mark Michelson