Summary:	ASTERISK-15774: [patch] MusicOnHold produces a crash
Reporter:	Dmitri Anpilogov (dmitri)	Labels:
Date Opened:	2010-03-09 02:29:54.000-0600	Date Closed:	2010-06-09 10:13:55
Priority:	Critical	Regression?	No
Status:	Closed/Complete	Components:	Resources/res_musiconhold
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) moh_crash.diff ( 1) moh_crash2.diff ( 2) res_musiconhold_rtclass.patch ( 3) res_musiconhold_rtclass2.patch
Description:	MusicOnHold produces a crash on hangup when class configured in realtime refers for non existing or an empty directory. And this only happens when using cachertclasses flag in musiconhold.conf. I made a patch for fixing this issue.
Comments:	By: Leif Madsen (lmadsen) 2010-03-10 10:30:50.000-0600 Thanks for the submission! By: Atis Lezdins (atis) 2010-05-07 13:10:37 We have been running this on production for several months now, and realtime music-on-hold is working fine. (Disclaimer: I work at the same company as dmitri) By: Leif Madsen (lmadsen) 2010-05-10 11:27:38 Assigned to pabelanger for review and possible commit. By: Dmitri Anpilogov (dmitri) 2010-05-14 04:38:15 It's about an issue 0016982. Both of bugs is close related to the realtime support in moh. By: David Vossel (dvossel) 2010-05-28 14:34:39 I uploaded a different patch I would like testing with.  If I'm understanding things correctly, I believe my patch will more clearly resolve this in the code.   If the mohclass is present at the end of this function, it has it's refcount decremented.  If MOH_CACHERTCLASSES flag is set it is possible the refcount was never incremented in the first place which could cause a crash. By: Dmitri Anpilogov (dmitri) 2010-06-04 17:19:35 Nope. In fact comment mark "XXX This code is impossible to reach" says true and Your first added line will never been executed. But if impossible becomes possible You just produce a new crash in moh_register(mohclass, 0, DONT_UNREF); cause mohclass is NULL now. Btw only the last case (if MOH_CACHERTCLASSES is not set) Your code works as needed. By: David Vossel (dvossel) 2010-06-04 17:52:30 I am unfamiliar with this code.  Can you explain exactly why your original patch resolve this crash.  It would be nice to see a back trace of what this crash looks like so I can better understand what is being resolved.  Just looking at your patch alone it is not obvious to me what that is supposed to fix.  Thanks for your feedback! By: Dmitri Anpilogov (dmitri) 2010-06-05 08:00:53 Let me explain the logic of this. If we are cashing realtime classes, then we call moh_register function, which adds class to memory container after some validity checks like a filesystem checks etc. If it fails, then we can not find this class by get_mohbyname function, but passed argument (mohclass) is not NULL now. If we are not using cache, then mohclass be checked in subjected function local_ast_moh_start (lines 1347-1404), and will be set to NULL if fails. I said earlier that Your code will work in second case, it's not true. Just because we are not using cache, thus we cannot find this class in memory -- this is always be empty. And I thought it would be cool to avoid unnecessary call of ast_test_flag. So new patch is ready. By: David Vossel (dvossel) 2010-06-07 08:43:42 moh_register returns a -1 if it fails to add the mohclass to the container. Wouldn't checking for that failure and explicitly setting mohclass to NULL make more sense? By: Dmitri Anpilogov (dmitri) 2010-06-07 17:52:25 I'm pretty sure You absolutely right with this! By: David Vossel (dvossel) 2010-06-08 17:57:39 I uploaded a new patch.  Can you verify it resolves the issue? By: Dmitri Anpilogov (dmitri) 2010-06-08 19:43:28 Works like a charm! By: Digium Subversion (svnbot) 2010-06-09 10:11:36 Repository: asterisk Revision: 269271 U   trunk/res/res_musiconhold.c ------------------------------------------------------------------------ r269271 | dvossel | 2010-06-09 10:09:25 -0500 (Wed, 09 Jun 2010) | 15 lines fixes crash in moh when cachertclasses flag is used The result for moh_register was not verified to guarantee the mohclass as added to the container. (closes issue ASTERISK-15774) Reported by: dmitri Patches:      res_musiconhold_rtclass2.patch uploaded by dmitri (license 1001)      moh_crash2.diff uploaded by dvossel (license 671) Tested by: dmitri ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=269271 By: Digium Subversion (svnbot) 2010-06-09 10:13:55 Repository: asterisk Revision: 269272 _U  branches/1.6.2/ U   branches/1.6.2/res/res_musiconhold.c ------------------------------------------------------------------------ r269272 | dvossel | 2010-06-09 10:13:54 -0500 (Wed, 09 Jun 2010) | 20 lines Merged revisions 269271 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ........  r269271 | dvossel | 2010-06-09 10:09:25 -0500 (Wed, 09 Jun 2010) | 15 lines    fixes crash in moh when cachertclasses flag is used    The result for moh_register was not verified to guarantee  the mohclass as added to the container.      (closes issue ASTERISK-15774)  Reported by: dmitri  Patches:        res_musiconhold_rtclass2.patch uploaded by dmitri (license 1001)        moh_crash2.diff uploaded by dvossel (license 671)  Tested by: dmitri ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=269272