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

[Tilghman Lesher]

> On 2009-12-07 17:11:26, Russell Bryant wrote:
> > /trunk/res/res_musiconhold.c, lines 235-236
> > <https://reviewboard.asterisk.org/r/442/diff/1/?file=7530#file7530line235>
> >
> >     If the only reference was the reference owned by the container, wouldn't you want to check for a return value of 2 here?  ao2_ref() returns the _previous_ value of the reference count.  In this error case, I would expect 2: 1 for the container, and 1 that was returned by the ao2_find().

Fixed.


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

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.


- Tilghman


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


On 2009-12-07 16:55:33, Tilghman Lesher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/442/
> -----------------------------------------------------------
> 
> (Updated 2009-12-07 16:55:33)
> 
> 
> 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
> 
>