Merged revisions 152535 via svnmerge from

https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r152535 | murf | 2008-10-28 22:36:32 -0600 (Tue, 28 Oct 2008) | 46 lines

The magic trick to avoid this crash is not to
try to find the channel by name in the list,
which is slow and resource consuming, but rather
to pay attention to the result codes from the
ast_bridge_call, to which I added the 
AST_PBX_NO_HANGUP_PEER_PARKED value, which
now are returned when a channel is parked.
Why? because CDR's aren't generated via parking,
so nothing is needed, but if a transfer occurred,
there are critical things I need.

If you get AST_PBX_KEEPALIVE,
then don't touch the channel pointer.

If you get AST_PBX_NO_HANGUP_PEER, or
AST_PBX_NO_HANGUP_PEER_PARKED, then don't
touch the peer pointer.

Updated the several places where the results
from a bridge were not being properly obeyed,
and fixed some code I had introduced so that
the results of the bridge were not overridden 
(in trunk).

All the places that previously tested for 
AST_PBX_NO_HANGUP_PEER now have to check for
both AST_PBX_NO_HANGUP_PEER and AST_PBX_NO_HANGUP_PEER_PARKED.

I tested this against the 4 common parking
scenarios:


1. A calls B; B answers; A parks B; B hangs up while A is getting the parking
slot announcement, immediately after being put on hold.

2. A calls B; B answers; A parks B; B hangs up after A has been hung up, but
before the park times out.

3. A calls B; B answers; B parks A; A hangs up while B is getting the parking slot announcement, immediately after being put on hold.

4. A calls B; B answers; B parks A; A hangs up after B has been hung up, but before the park times out.


No crash.

I also ran the scenarios above against valgrind, and accesses looked good.



........


git-svn-id: http://svn.digium.com/svn/asterisk/trunk@152536 f38db490-d61c-443f-a65b-d21fe96a405b

1 files changed, 89 insertions, 60 deletions

diff --git a/main/features.c b/main/features.c
index c5e6d9201..ef07b23db 100644
--- a/main/features.c
+++ b/main/features.c
@@ -777,7 +777,7 @@ static int builtin_parkcall(struct ast_channel *chan, struct ast_channel *peer,
 			res = ast_park_call(parkee, parker, 0, NULL);
 			if (!res) {
 				if (sense == FEATURE_SENSE_CHAN) {
-					res = AST_PBX_NO_HANGUP_PEER;
+					res = AST_PBX_NO_HANGUP_PEER_PARKED;
 				} else {
 					res = AST_PBX_KEEPALIVE;
 				}
@@ -1127,7 +1127,7 @@ static int builtin_blindtransfer(struct ast_channel *chan, struct ast_channel *p
 			   the thread dies -- We have to be careful now though.  We are responsible for 
 			   hanging up the channel, else it will never be hung up! */
 
-			return (transferer == peer) ? AST_PBX_KEEPALIVE : AST_PBX_NO_HANGUP_PEER;
+			return (transferer == peer) ? AST_PBX_KEEPALIVE : AST_PBX_NO_HANGUP_PEER_PARKED;
 		} else {
 			ast_log(LOG_WARNING, "Unable to park call %s\n", transferee->name);
 		}
@@ -1730,6 +1730,8 @@ static int feature_exec_app(struct ast_channel *chan, struct ast_channel *peer,
 	}
 	else if (res == AST_PBX_NO_HANGUP_PEER)
 		return AST_FEATURE_RETURN_NO_HANGUP_PEER;
+	else if (res == AST_PBX_NO_HANGUP_PEER_PARKED)
+		return AST_FEATURE_RETURN_NO_HANGUP_PEER_PARKED;
 	else if (res)
 		return AST_FEATURE_RETURN_SUCCESSBREAK;
 	
@@ -2377,10 +2379,12 @@ int ast_bridge_call(struct ast_channel *chan,struct ast_channel *peer,struct ast
 
 	}
    before_you_go:
-	new_chan_cdr = pick_unlocked_cdr(chan->cdr); /* the proper chan cdr, if there are forked cdrs */
-	new_peer_cdr = pick_unlocked_cdr(peer->cdr); /* the proper chan cdr, if there are forked cdrs */
 
-	if (!ast_test_flag(&(config->features_caller),AST_FEATURE_NO_H_EXTEN) && ast_exists_extension(chan, chan->context, "h", 1, chan->cid.cid_num)) {
+	/* run the hangup exten on the chan object IFF it was NOT involved in a parking situation 
+	 * if it were, then chan belongs to a different thread now, and might have been hung up long
+     * ago.
+	 */
+	if (res != AST_PBX_KEEPALIVE && !ast_test_flag(&(config->features_caller),AST_FEATURE_NO_H_EXTEN) && ast_exists_extension(chan, chan->context, "h", 1, chan->cid.cid_num)) {
 		struct ast_cdr *swapper;
 		char savelastapp[AST_MAX_EXTENSION];
 		char savelastdata[AST_MAX_EXTENSION];
@@ -2424,39 +2428,57 @@ int ast_bridge_call(struct ast_channel *chan,struct ast_channel *peer,struct ast
 		ast_copy_string(bridge_cdr->lastdata, savelastdata, sizeof(bridge_cdr->lastdata));
 	}
 
-	/* obey the NoCDR() wishes. */
- 	if (new_chan_cdr && ast_test_flag(new_chan_cdr, AST_CDR_FLAG_POST_DISABLED) && new_peer_cdr && !ast_test_flag(new_peer_cdr, AST_CDR_FLAG_POST_DISABLED))
- 		ast_set_flag(new_peer_cdr, AST_CDR_FLAG_POST_DISABLED); /* DISABLED is viral-- it will propagate across a bridge */
- 	if (!new_chan_cdr || (new_chan_cdr && !ast_test_flag(new_chan_cdr, AST_CDR_FLAG_POST_DISABLED))) {
- 		struct ast_channel *chan_ptr = NULL;
-		
-		ast_cdr_end(bridge_cdr);
-		
-		ast_cdr_detach(bridge_cdr);
-		
- 		/* do a specialized reset on the beginning channel
- 		   CDR's, if they still exist, so as not to mess up
- 		   issues in future bridges;
-  
- 		   Here are the rules of the game:
- 		   1. The chan and peer channel pointers will not change
- 		      during the life of the bridge.
- 		   2. But, in transfers, the channel names will change.
-		      between the time the bridge is started, and the
-		      time the channel ends. 
-		      Usually, when a channel changes names, it will
-		      also change CDR pointers.
-		   3. Usually, only one of the two channels (chan or peer)
-		      will change names.
-		   4. Usually, if a channel changes names during a bridge,
-		      it is because of a transfer. Usually, in these situations,
-		      it is normal to see 2 bridges running simultaneously, and
-		      it is not unusual to see the two channels that change
-		      swapped between bridges.
- 		   5. After a bridge occurs, we have 2 or 3 channels' CDRs
- 		      to attend to; if the chan or peer changed names,
- 		      we have the before and after attached CDR's.
-		*/
+	/* obey the NoCDR() wishes. -- move the DISABLED flag to the bridge CDR if it was set on the channel during the bridge... */
+	if (res != AST_PBX_KEEPALIVE) {
+		new_chan_cdr = pick_unlocked_cdr(chan->cdr); /* the proper chan cdr, if there are forked cdrs */
+		if (new_chan_cdr && ast_test_flag(new_chan_cdr, AST_CDR_FLAG_POST_DISABLED))
+			ast_set_flag(bridge_cdr, AST_CDR_FLAG_POST_DISABLED);
+	}
+
+	/* we can post the bridge CDR at this point */
+	ast_cdr_end(bridge_cdr);
+	ast_cdr_detach(bridge_cdr);
+	
+	/* do a specialized reset on the beginning channel
+	   CDR's, if they still exist, so as not to mess up
+	   issues in future bridges;
+	   
+	   Here are the rules of the game:
+	   1. The chan and peer channel pointers will not change
+	      during the life of the bridge.
+	   2. But, in transfers, the channel names will change.
+	      between the time the bridge is started, and the
+	      time the channel ends. 
+	      Usually, when a channel changes names, it will
+	      also change CDR pointers.
+	   3. Usually, only one of the two channels (chan or peer)
+	      will change names.
+	   4. Usually, if a channel changes names during a bridge,
+	      it is because of a transfer. Usually, in these situations,
+	      it is normal to see 2 bridges running simultaneously, and
+	      it is not unusual to see the two channels that change
+	      swapped between bridges.
+	   5. After a bridge occurs, we have 2 or 3 channels' CDRs
+	      to attend to; if the chan or peer changed names,
+	      we have the before and after attached CDR's.
+	   6. Parking has to be accounted for in the code:
+	      a. Parking will cause ast_bridge_call to return
+	         either AST_PBX_NO_HANGUP_PEER or AST_PBX_NO_HANGUP_PEER_PARKED;
+			 in the latter case, peer is (most likely) a bad
+			 pointer, you can no longer deref it. If it does still
+			 exist, it is under another's thread control, and
+			 could be destroyed at any time.
+          b. The same applies to AST_PBX_KEEPALIVE, in which
+		     case, the chan ptr cannot be used, as another thread
+			 owns it and may have destroyed the channel.
+	      c. In the former case, you need to check peer to see if it 
+	         still exists before you deref it, and obtain a lock.
+	      d. In neither case should you do an ast_hangup(peer).
+		  e. Do not overwrite the result code from ast_bridge_call.
+	*/
+	
+	if (res != AST_PBX_KEEPALIVE && new_chan_cdr) {
+		struct ast_channel *chan_ptr = NULL;
  
  		if (strcasecmp(orig_channame, chan->name) != 0) { 
  			/* old channel */
@@ -2479,28 +2501,35 @@ int ast_bridge_call(struct ast_channel *chan,struct ast_channel *peer,struct ast
  		} else {
  			ast_cdr_specialized_reset(chan_cdr,0); /* nothing changed, reset the chan_cdr  */
  		}
- 		if (strcasecmp(orig_peername, peer->name) != 0) { 
- 			/* old channel */
- 			chan_ptr = ast_get_channel_by_name_locked(orig_peername);
- 			if (chan_ptr) {
- 				if (!ast_bridged_channel(chan_ptr)) {
- 					struct ast_cdr *cur;
- 					for (cur = chan_ptr->cdr; cur; cur = cur->next) {
+	}
+	
+	if (res != AST_PBX_NO_HANGUP_PEER_PARKED) { /* if the peer was involved in a park, don't even touch it; it's probably gone */
+		struct ast_channel *chan_ptr = NULL;
+		new_peer_cdr = pick_unlocked_cdr(peer->cdr); /* the proper chan cdr, if there are forked cdrs */
+		if (new_chan_cdr && ast_test_flag(new_chan_cdr, AST_CDR_FLAG_POST_DISABLED) && new_peer_cdr && !ast_test_flag(new_peer_cdr, AST_CDR_FLAG_POST_DISABLED))
+			ast_set_flag(new_peer_cdr, AST_CDR_FLAG_POST_DISABLED); /* DISABLED is viral-- it will propagate across a bridge */
+		if (strcasecmp(orig_peername, peer->name) != 0) { 
+			/* old channel */
+			chan_ptr = ast_get_channel_by_name_locked(orig_peername);
+			if (chan_ptr) {
+				if (!ast_bridged_channel(chan_ptr)) {
+					struct ast_cdr *cur;
+					for (cur = chan_ptr->cdr; cur; cur = cur->next) {
 						if (cur == peer_cdr) {
- 							break;
- 						}
- 					}
- 					if (cur)
- 						ast_cdr_specialized_reset(peer_cdr,0);
- 				}
- 				ast_channel_unlock(chan_ptr);
- 			}
- 			/* new channel */
- 			ast_cdr_specialized_reset(new_peer_cdr,0);
- 		} else {
- 			ast_cdr_specialized_reset(peer_cdr,0); /* nothing changed, reset the peer_cdr  */
- 		}
- 	}
+							break;
+						}
+					}
+					if (cur)
+						ast_cdr_specialized_reset(peer_cdr,0);
+				}
+				ast_channel_unlock(chan_ptr);
+			}
+			/* new channel */
+			ast_cdr_specialized_reset(new_peer_cdr,0);
+		} else {
+			ast_cdr_specialized_reset(peer_cdr,0); /* nothing changed, reset the peer_cdr  */
+		}
+	}
 	return res;
 }
 
@@ -2955,7 +2984,7 @@ static int park_exec_full(struct ast_channel *chan, void *data, struct ast_parki
 		ast_cdr_setdestchan(chan->cdr, peer->name);
 
 		/* Simulate the PBX hanging up */
-		if (res != AST_PBX_NO_HANGUP_PEER)
+		if (res != AST_PBX_NO_HANGUP_PEER && res != AST_PBX_NO_HANGUP_PEER_PARKED)
 			ast_hangup(peer);
 		return res;
 	} else {