[asterisk-dev] tilghman: trunk r232660 - in /trunk: include/asterisk/ res/

[Tilghman Lesher]

On Friday 04 December 2009 14:37:41 Russell Bryant wrote:
> SVN commits to the Digium repositories wrote:
> > Author: tilghman
> > Date: Wed Dec  2 18:08:55 2009
> > New Revision: 232660
> >
> > URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=232660
> > Log:
> > Fix multiple issues with musiconhold, which led to classes not getting
> > destroyed properly. * Classes are now tracked past removal from the core
> > container, and module removal is actively prevented until all references
> > are freed. * A hanging reference stored in the channel has been removed. 
> > This could have caused a mismatch and the music state not properly
> > cleared, if two or more reloads occurred between MOH being stopped and
> > MOH being restarted. * In certain circumstances, duplicate classes were
> > possible.
> >  * A race existed at reload time between a process being killed and the
> > thread responsible for reading from the related pipe respawning that
> > process. * Several reference counts have also been corrected.  At least
> > one could have caused deleted classes to stick around forever, consuming
> > resources.  This originally manifested as MOH external processes that
> > were not killed at reload time.
> > (closes issue #16279, closes issue #16207)
> >  Reported by: parisioa, dcabot
> >  Patches:
> >        20091202__issue16279__2.diff.txt uploaded by tilghman (license 14)
> >  Tested by: parisioa, tilghman
>
> <snip code>
>
> Could you please expand on the reasoning behind some of these changes?
> Specifically, I don't understand why a new deleted classes container is
> needed, and why a new thread is needed to delete MOH classes.  We use
> reference counted objects from configuration in many places, and nothing
> like this is used anywhere else.

The reporter tried unloading the res_musiconhold module after the classes had
been removed from the container, but before the channels had stopped using
them.  That caused a crash.  So I think it's clear that we need to track those
references and ensure the module cannot be unloaded prior to all references
being deleted.

> Also, based on the latest notes in the bug, it looks like there are
> still problems in this code.  The fact that the astobj2 errors occur
> when the "moh show classes" command runs seems very indicative of a
> problem in the MOH code.

It actually looks like memory corruption, where chan_sip used memory after
it was freed.  However, I'll need Valgrind output to confirm.

-- 
Tilghman Lesher
Digium, Inc. | Senior Software Developer
twitter: Corydon76 | IRC: Corydon76-dig (Freenode)
Check us out at: www.digium.com & www.asterisk.org