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

[Andrew Parisio]

Valgrind output is attached to the new bug report you asked me to post 16388.

If you need anything else let me know and I'll whip it together.  I'd like to get these issues ironed out.

-----Original Message-----
From: asterisk-dev-bounces at lists.digium.com [mailto:asterisk-dev-bounces at lists.digium.com] On Behalf Of Tilghman Lesher
Sent: Friday, December 04, 2009 1:22 PM
To: Asterisk Developers Mailing List
Subject: Re: [asterisk-dev] tilghman: trunk r232660 - in /trunk: include/asterisk/ res/

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

_______________________________________________
--Bandwidth and Colocation Provided by http://www.api-digital.com--

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev