[asterisk-commits] mmichelson: branch 1.8 r357761 - /branches/1.8/main/channel.c

[SVN commits to the Asterisk project]

Author: mmichelson
Date: Thu Mar  1 18:59:18 2012
New Revision: 357761

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=357761
Log:
Fix race condition that can cause important control frames (such as a hangup) to be missed.

This takes two actions.

1. Move the reading of the alertpipe in __ast_read() to immediately before the
removal of frames from the readq. This means we won't do something silly like
read from the alertpipe, then ignore the fact that there's a frame to get from
the readq since channel's fdno is the AST_TIMING_FD.

2. When ast_settimeout() sets the rate to 0 and the timingfunc to NULL, if the
channel's fdno is the AST_TIMING_FD, then set the fdno to -1. This is because
if the rate is 0 and the timingfunc is NULL, it means that the channel's timing
fd is being invalidated, so any pending reads should not occur.

This may actually solve more issues than the referenced one below, but it's not
known at this time for sure.

(closes issue ASTERISK-19223)
reported by Frank-Michael Wittig

Review: https://reviewboard.asterisk.org/r/1779


Modified:
    branches/1.8/main/channel.c

Modified: branches/1.8/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/channel.c?view=diff&rev=357761&r1=357760&r2=357761
==============================================================================
--- branches/1.8/main/channel.c (original)
+++ branches/1.8/main/channel.c Thu Mar  1 18:59:18 2012
@@ -3507,6 +3507,16 @@
 	c->timingfunc = func;
 	c->timingdata = data;
 
+	if (func == NULL && rate == 0 && c->fdno == AST_TIMING_FD) {
+		/* Clearing the timing func and setting the rate to 0
+		 * means that we don't want to be reading from the timingfd
+		 * any more. Setting c->fdno to -1 means we won't have any
+		 * errant reads from the timingfd, meaning we won't potentially
+		 * miss any important frames.
+		 */
+		c->fdno = -1;
+	}
+
 	ast_channel_unlock(c);
 
 	return res;
@@ -3779,6 +3789,53 @@
 	}
 
 	prestate = chan->_state;
+	if (chan->timingfd > -1 && chan->fdno == AST_TIMING_FD) {
+		enum ast_timer_event res;
+
+		ast_clear_flag(chan, AST_FLAG_EXCEPTION);
+
+		res = ast_timer_get_event(chan->timer);
+
+		switch (res) {
+		case AST_TIMING_EVENT_EXPIRED:
+			ast_timer_ack(chan->timer, 1);
+
+			if (chan->timingfunc) {
+				/* save a copy of func/data before unlocking the channel */
+				int (*func)(const void *) = chan->timingfunc;
+				void *data = chan->timingdata;
+				chan->fdno = -1;
+				ast_channel_unlock(chan);
+				func(data);
+			} else {
+				ast_timer_set_rate(chan->timer, 0);
+				chan->fdno = -1;
+				ast_channel_unlock(chan);
+			}
+
+			/* cannot 'goto done' because the channel is already unlocked */
+			return &ast_null_frame;
+
+		case AST_TIMING_EVENT_CONTINUOUS:
+			if (AST_LIST_EMPTY(&chan->readq) || 
+				!AST_LIST_NEXT(AST_LIST_FIRST(&chan->readq), frame_list)) {
+				ast_timer_disable_continuous(chan->timer);
+			}
+			break;
+		}
+
+	} else if (chan->fds[AST_GENERATOR_FD] > -1 && chan->fdno == AST_GENERATOR_FD) {
+		/* if the AST_GENERATOR_FD is set, call the generator with args
+		 * set to -1 so it can do whatever it needs to.
+		 */
+		void *tmp = chan->generatordata;
+		chan->generatordata = NULL;     /* reset to let ast_write get through */
+		chan->generator->generate(chan, tmp, -1, -1);
+		chan->generatordata = tmp;
+		f = &ast_null_frame;
+		chan->fdno = -1;
+		goto done;
+	}
 
 	/* Read and ignore anything on the alertpipe, but read only
 	   one sizeof(blah) per frame that we send from it */
@@ -3800,53 +3857,6 @@
 		}
 	}
 
-	if (chan->timingfd > -1 && chan->fdno == AST_TIMING_FD) {
-		enum ast_timer_event res;
-
-		ast_clear_flag(chan, AST_FLAG_EXCEPTION);
-
-		res = ast_timer_get_event(chan->timer);
-
-		switch (res) {
-		case AST_TIMING_EVENT_EXPIRED:
-			ast_timer_ack(chan->timer, 1);
-
-			if (chan->timingfunc) {
-				/* save a copy of func/data before unlocking the channel */
-				int (*func)(const void *) = chan->timingfunc;
-				void *data = chan->timingdata;
-				chan->fdno = -1;
-				ast_channel_unlock(chan);
-				func(data);
-			} else {
-				ast_timer_set_rate(chan->timer, 0);
-				chan->fdno = -1;
-				ast_channel_unlock(chan);
-			}
-
-			/* cannot 'goto done' because the channel is already unlocked */
-			return &ast_null_frame;
-
-		case AST_TIMING_EVENT_CONTINUOUS:
-			if (AST_LIST_EMPTY(&chan->readq) || 
-				!AST_LIST_NEXT(AST_LIST_FIRST(&chan->readq), frame_list)) {
-				ast_timer_disable_continuous(chan->timer);
-			}
-			break;
-		}
-
-	} else if (chan->fds[AST_GENERATOR_FD] > -1 && chan->fdno == AST_GENERATOR_FD) {
-		/* if the AST_GENERATOR_FD is set, call the generator with args
-		 * set to -1 so it can do whatever it needs to.
-		 */
-		void *tmp = chan->generatordata;
-		chan->generatordata = NULL;     /* reset to let ast_write get through */
-		chan->generator->generate(chan, tmp, -1, -1);
-		chan->generatordata = tmp;
-		f = &ast_null_frame;
-		chan->fdno = -1;
-		goto done;
-	}
 
 	/* Check for pending read queue */
 	if (!AST_LIST_EMPTY(&chan->readq)) {