[asterisk-bugs] [Asterisk 0011196]: RealTime MusicOnHold

[noreply at bugs.digium.com]

A NOTE has been added to this issue. 
====================================================================== 
http://bugs.digium.com/view.php?id=11196 
====================================================================== 
Reported By:                sergee
Assigned To:                
====================================================================== 
Project:                    Asterisk
Issue ID:                   11196
Category:                   Resources/res_musiconhold
Reproducibility:            always
Severity:                   feature
Priority:                   normal
Status:                     new
Asterisk Version:            SVN 
SVN Branch (only for SVN checkouts, not tarball releases):  trunk 
SVN Revision (number only!): 89114 
Disclaimer on File?:        N/A 
Request Review:              
====================================================================== 
Date Submitted:             11-08-2007 12:59 CST
Last Modified:              11-19-2007 16:47 CST
====================================================================== 
Summary:                    RealTime MusicOnHold
Description: 
Some parts of asterisk support RealTime, but others don't. This patch is an
implementation of RT for res_musiconhold. Now you can store MOH classes in
text config and RT engine. Classes from RT engine override classes from
text config. 

To use it, add a line to your extconfig.conf:
musiconhold => mysql,asterisk

Every time MOH starts, search is perfomed first in RT engine, then in
memory (linked list of classes created from musiconhold.conf).

If MOH class is found in RT engine - separate structure is created in
memory. This structure is not added to linked list (mohclasses) and will be
destroyed as soon as MOH will stop.
====================================================================== 

---------------------------------------------------------------------- 
 putnopvut - 11-19-07 16:47  
---------------------------------------------------------------------- 
Okay, valgrind did find a couple of problems, and after inspecting the
code, it should have been much more obvious that the problems existed. I
love valgrind :)

Anyway, here are the issues, starting with the easy one:

1. In local_ast_moh_cleanup, if you are not using cached realtime classes,
then ast_moh_destroy_one is called. In here, the moh class is freed. Upon
return, you immediately check the value of state->class->realtime. Because
the class was just freed, you are reading invalid memory. A simple
rearrangement of if's and the use of an else will easily clear up this
issue.

2. This one is a bit trickier to explain. In local_ast_moh_start, if we
determine that we are using a realtime moh class, we call
ast_pthread_create_background with that class as the argument. Later, if we
determine that the channel already had an moh class associated with it, we
free the moh class which we sent as the argument to monmp3thread, and
"replace" it with the channel's moh class. The problem here is that the 
monmp3thread still refers to the class which was freed, meaning there are
invalid reads and writes depending on timing. Under heavy loads, I could
see this potentially crashing Asterisk. As a test, I moved the check to see
if the channel already had an moh class associated with it to before the
call to ast_pthread_create_background, and I didn't see any more valgrind
problems.


While I'm pointing out problems, I thought I'd also point out a couple of
minor things. One is that the code has a compile warning in
local_ast_moh_stop due to mixed declarations and code. The problem is that
you declare the moh_files_state after calling ast_clear_flag and
ast_deactivate_generator. Another small issue is that if no statically
defined moh class is found, there is a warning printed on the console
saying that the moh class could not be found. This warning should be
changed to state that the moh class wasn't found in memory, so we're going
to check realtime instead. This will help to prevent some possible
confusion.

Keep up the good work! I know I'm probably being a pain, but it's good to
have as many potential bugs cleared up now before people start using it in
production. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
11-19-07 16:47  putnopvut      Note Added: 0074011                          
======================================================================