[asterisk-dev] [Code Review] [16388] MOH refcount fix

[Russell Bryant]

> On 2009-12-07 17:11:26, Russell Bryant wrote:
> > /trunk/res/res_musiconhold.c, lines 254-281
> > <https://reviewboard.asterisk.org/r/442/diff/1/?file=7530#file7530line254>
> >
> >     I don't think this is the best place to unref the module.  I think it should only be where the state object is destroyed (perhaps just in ast_moh_cleanup()?).
> >     
> >     With the way the code is now, it is possible to leak memory if this module gets unloaded.  Take the case where MOH has been started and stopped.  The state is still on the ast_channel, but the module ref does not exist.  If the module gets unloaded, we will leak the state when the channel goes away.
> 
>  wrote:
>     While that's true, it's really the generator that we're protecting.  That is the part that could potentially cause a crash if the module were unloaded at the wrong time.  While it's clear that we're also dereferencing the class, and the state is left hanging, it's the potential for a crash that is the real reason why we want to protect the module from unload.

If we're going to add module ref counting here, why not do it the way that both prevents crashes and memory leaks?  It's not any more difficult to do.


- Russell


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/442/#review1285
-----------------------------------------------------------


On 2009-12-07 17:45:01, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/442/
> -----------------------------------------------------------
> 
> (Updated 2009-12-07 17:45:01)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> Summary
> -------
> 
> See issue #16388
> 
> 
> This addresses bug 16388.
>     https://issues.asterisk.org/view.php?id=16388
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_musiconhold.c 232854 
> 
> Diff: https://reviewboard.asterisk.org/r/442/diff
> 
> 
> Testing
> -------
> 
> Works for the reporter
> 
> 
> Thanks,
> 
> Tilghman
> 
>