[asterisk-commits] rmudgett: branch 10 r334357 - in /branches/10: ./ res/res_musiconhold.c

[SVN commits to the Asterisk project]

Author: rmudgett
Date: Fri Sep  2 16:08:16 2011
New Revision: 334357

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=334357
Log:
Merged revisions 334355 via svnmerge from 
https://origsvn.digium.com/svn/asterisk/branches/1.8

........
  r334355 | rmudgett | 2011-09-02 15:59:49 -0500 (Fri, 02 Sep 2011) | 19 lines
  
  MusicOnHold has extra unref which may lead to memory corruption and crash.
  
  The problem happens when a call is disconnected and you had started a MOH 
  class that does not use the files mode.  If you define REF_DEBUG and 
  recreate the problem, it will announce itself with the following warning: 
  Attempt to unref mohclass 0xb70722e0 (default) when only 1 ref remained, 
  and class is still in a container!  
  
  * Fixed moh_alloc() and moh_release() functions not handling the
  state->class reference consistently.
  
  (closes issue ASTERISK-18346)
  Reported by: Mark Murawski
  Patches:
        jira_asterisk_18346_v1.8.patch (license #5621) patch uploaded by rmudgett
  Tested by: rmudgett, Mark Murawski
  
  Review: https://reviewboard.asterisk.org/r/1404/
........

Modified:
    branches/10/   (props changed)
    branches/10/res/res_musiconhold.c

Propchange: branches/10/
------------------------------------------------------------------------------
Binary property 'branch-1.8-merged' - no diff available.

Modified: branches/10/res/res_musiconhold.c
URL: http://svnview.digium.com/svn/asterisk/branches/10/res/res_musiconhold.c?view=diff&rev=334357&r1=334356&r2=334357
==============================================================================
--- branches/10/res/res_musiconhold.c (original)
+++ branches/10/res/res_musiconhold.c Fri Sep  2 16:08:16 2011
@@ -151,6 +151,7 @@
 static int respawn_time = 20;
 
 struct moh_files_state {
+	/*! Holds a reference to the MOH class. */
 	struct mohclass *class;
 	char name[MAX_MUSICCLASS];
 	struct ast_format origwfmt;
@@ -232,7 +233,7 @@
 {
 	struct mohclass *dup;
 	if ((dup = ao2_find(mohclasses, class, OBJ_POINTER))) {
-		if (_ao2_ref_debug(dup, -1, (char *) tag, (char *) file, line, funcname) == 2) {
+		if (__ao2_ref_debug(dup, -1, (char *) tag, (char *) file, line, funcname) == 2) {
 			FILE *ref = fopen("/tmp/refs", "a");
 			if (ref) {
 				fprintf(ref, "%p =1   %s:%d:%s (%s) BAD ATTEMPT!\n", class, file, line, funcname, tag);
@@ -433,10 +434,13 @@
 		ast_module_ref(ast_module_info->self);
 	} else {
 		state = chan->music_state;
-	}
-
-	if (!state) {
-		return NULL;
+		if (!state) {
+			return NULL;
+		}
+		if (state->class) {
+			mohclass_unref(state->class, "Uh Oh. Restarting MOH with an active class");
+			ast_log(LOG_WARNING, "Uh Oh. Restarting MOH with an active class\n");
+		}
 	}
 
 	/* LOGIC: Comparing an unrefcounted pointer is a really bad idea, because
@@ -936,6 +940,12 @@
 	ast_free(moh);
 
 	if (chan) {
+		struct moh_files_state *state;
+
+		state = chan->music_state;
+		if (state && state->class) {
+			state->class = mohclass_unref(state->class, "Unreffing channel's music class upon deactivation of generator");
+		}
 		if (oldwfmt.id && ast_set_write_format(chan, &oldwfmt)) {
 			ast_log(LOG_WARNING, "Unable to restore channel '%s' to format %s\n",
 					chan->name, ast_getformatname(&oldwfmt));
@@ -954,13 +964,17 @@
 	/* Initiating music_state for current channel. Channel should know name of moh class */
 	if (!chan->music_state && (state = ast_calloc(1, sizeof(*state)))) {
 		chan->music_state = state;
-		state->class = mohclass_ref(class, "Copying reference into state container");
 		ast_module_ref(ast_module_info->self);
-	} else
+	} else {
 		state = chan->music_state;
-	if (state && state->class != class) {
+		if (!state) {
+			return NULL;
+		}
+		if (state->class) {
+			mohclass_unref(state->class, "Uh Oh. Restarting MOH with an active class");
+			ast_log(LOG_WARNING, "Uh Oh. Restarting MOH with an active class\n");
+		}
 		memset(state, 0, sizeof(*state));
-		state->class = class;
 	}
 
 	if ((res = mohalloc(class))) {
@@ -969,6 +983,8 @@
 			ast_log(LOG_WARNING, "Unable to set channel '%s' to format '%s'\n", chan->name, ast_codec2str(&class->format));
 			moh_release(NULL, res);
 			res = NULL;
+		} else {
+			state->class = mohclass_ref(class, "Placing reference into state container");
 		}
 		ast_verb(3, "Started music on hold, class '%s', on channel '%s'\n", class->name, chan->name);
 	}
@@ -1285,7 +1301,10 @@
 
 	if (state) {
 		if (state->class) {
-			state->class = mohclass_unref(state->class, "Channel MOH state destruction");
+			/* This should never happen.  We likely just leaked some resource. */
+			state->class =
+				mohclass_unref(state->class, "Uh Oh. Cleaning up MOH with an active class");
+			ast_log(LOG_WARNING, "Uh Oh. Cleaning up MOH with an active class\n");
 		}
 		ast_free(chan->music_state);
 		chan->music_state = NULL;