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

[Russell Bryant]

Tilghman Lesher wrote:
> 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.

The need to track module references makes sense.  I would expect module
references to be incremented and decremented while references exist on
channels.  I still don't understand the need for the new container and a
new thread.

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

The valgrind is there now.  It does appear to be a MOH bug.  The "moh
show classes" command is trying to read from classes that have already
been destroyed.


-- 
Russell Bryant
Digium, Inc. | Engineering Manager, Open Source Software
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org