[asterisk-commits] russell: branch 1.4 r108135 - in /branches/1.4: apps/ main/

[SVN commits to the Asterisk project]

Author: russell
Date: Wed Mar 12 14:57:42 2008
New Revision: 108135

URL: http://svn.digium.com/view/asterisk?view=rev&rev=108135
Log:
(closes issue #12187, reported by atis, fixed by me after some brainstorming
 on the issue with mmichelson)

- Update copyright info on app_chanspy.

- Fix a race condition that caused app_chanspy to crash.  The issue was that
  the chanspy datastore magic that was used to ensure that spyee channels did
  not disappear out from under the code did not completely solve the problem.
  It was actually possible for chanspy to acquire a channel reference out of
  its datastore to a channel that was in the middle of being destroyed.  That
  was because datastore destruction in ast_channel_free() was done near the
  end.  So, this left the code in app_chanspy accessing a channel that was
  partially, or completely invalid because it was in the process of being free'd
  by another thread.  The following sort of shows the code path where the race 
  occurred:

  =============================================================================
  Thread 1 (PBX thread for spyee chan)  ||   Thread 2 (chanspy)
  --------------------------------------||-------------------------------------
  ast_channel_free()                    ||
    - remove channel from channel list  ||
    - lock/unlock the channel to ensure ||
      that no references retrieved from ||
      the channel list exist.           ||
  --------------------------------------||-------------------------------------
                                        || channel_spy()
    - destroy some channel data         ||  - Lock chanspy datastore
                                        ||  - Retrieve reference to channel
    - destroy channel datastores        ||  - lock channel
       - call chanspy datastore d'tor   ||  - Unlock chanspy datastore
         which NULL's out the ds'       ||
         reference to the channel       ||  
                                        ||  - Operate on the channel ...
    - free the channel                  || 
                                        ||
                                        ||  - unlock the channel
  --------------------------------------||-------------------------------------
  =============================================================================


Modified:
    branches/1.4/apps/app_chanspy.c
    branches/1.4/main/channel.c

Modified: branches/1.4/apps/app_chanspy.c
URL: http://svn.digium.com/view/asterisk/branches/1.4/apps/app_chanspy.c?view=diff&rev=108135&r1=108134&r2=108135
==============================================================================
--- branches/1.4/apps/app_chanspy.c (original)
+++ branches/1.4/apps/app_chanspy.c Wed Mar 12 14:57:42 2008
@@ -2,7 +2,7 @@
  * Asterisk -- An open source telephony toolkit.
  *
  * Copyright (C) 2005 Anthony Minessale II (anthmct at yahoo.com)
- * Copyright (C) 2005 - 2006, Digium, Inc.
+ * Copyright (C) 2005 - 2008, Digium, Inc.
  *
  * A license has been granted to Digium (via disclaimer) for the use of
  * this code.
@@ -23,6 +23,8 @@
  * \brief ChanSpy: Listen in on any channel.
  *
  * \author Anthony Minessale II <anthmct at yahoo.com>
+ * \author Joshua Colp <jcolp at digium.com>
+ * \author Russell Bryant <russell at digium.com>
  *
  * \ingroup applications
  */

Modified: branches/1.4/main/channel.c
URL: http://svn.digium.com/view/asterisk/branches/1.4/main/channel.c?view=diff&rev=108135&r1=108134&r2=108135
==============================================================================
--- branches/1.4/main/channel.c (original)
+++ branches/1.4/main/channel.c Wed Mar 12 14:57:42 2008
@@ -1193,10 +1193,21 @@
 		AST_LIST_UNLOCK(&channels);
 		ast_log(LOG_ERROR, "Unable to find channel in list to free. Assuming it has already been done.\n");
 	}
-	/* Lock and unlock the channel just to be sure nobody
-	   has it locked still */
+	/* Lock and unlock the channel just to be sure nobody has it locked still
+	   due to a reference retrieved from the channel list. */
 	ast_channel_lock(chan);
 	ast_channel_unlock(chan);
+
+	/* Get rid of each of the data stores on the channel */
+	while ((datastore = AST_LIST_REMOVE_HEAD(&chan->datastores, entry)))
+		/* Free the data store */
+		ast_channel_datastore_free(datastore);
+
+	/* Lock and unlock the channel just to be sure nobody has it locked still
+	   due to a reference that was stored in a datastore. (i.e. app_chanspy) */
+	ast_channel_lock(chan);
+	ast_channel_unlock(chan);
+
 	if (chan->tech_pvt) {
 		ast_log(LOG_WARNING, "Channel '%s' may not have been hung up properly\n", chan->name);
 		free(chan->tech_pvt);
@@ -1233,12 +1244,6 @@
 	while ((f = AST_LIST_REMOVE_HEAD(&chan->readq, frame_list)))
 		ast_frfree(f);
 	
-	/* Get rid of each of the data stores on the channel */
-	while ((datastore = AST_LIST_REMOVE_HEAD(&chan->datastores, entry)))
-		/* Free the data store */
-		ast_channel_datastore_free(datastore);
-	AST_LIST_HEAD_INIT_NOLOCK(&chan->datastores);
-
 	/* loop over the variables list, freeing all data and deleting list items */
 	/* no need to lock the list, as the channel is already locked */