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

[Russell Bryant]

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 ...

-- 
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.