[asterisk-commits] rmudgett: branch 1.8 r348362 - in /branches/1.8: apps/ funcs/ include/asteris...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Fri Dec 16 14:55:22 CST 2011


Author: rmudgett
Date: Fri Dec 16 14:55:17 2011
New Revision: 348362

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=348362
Log:
Fix crash during CDR update.

The ast_cdr_setcid() and ast_cdr_update() were shown in ASTERISK-18836 to
be called by different threads for the same channel.  The channel driver
thread and the PBX thread running dialplan.

* Add lock protection around CDR API calls that access an ast_channel
pointer.

(closes issue ASTERISK-18836)
Reported by: gpluser

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

Modified:
    branches/1.8/apps/app_authenticate.c
    branches/1.8/apps/app_followme.c
    branches/1.8/apps/app_queue.c
    branches/1.8/funcs/func_cdr.c
    branches/1.8/include/asterisk/cdr.h
    branches/1.8/main/channel.c
    branches/1.8/main/features.c
    branches/1.8/main/pbx.c
    branches/1.8/res/res_monitor.c

Modified: branches/1.8/apps/app_authenticate.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/apps/app_authenticate.c?view=diff&rev=348362&r1=348361&r2=348362
==============================================================================
--- branches/1.8/apps/app_authenticate.c (original)
+++ branches/1.8/apps/app_authenticate.c Fri Dec 16 14:55:17 2011
@@ -213,14 +213,20 @@
 						continue;
 					ast_md5_hash(md5passwd, passwd);
 					if (!strcmp(md5passwd, md5secret)) {
-						if (ast_test_flag(&flags,OPT_ACCOUNT))
+						if (ast_test_flag(&flags,OPT_ACCOUNT)) {
+							ast_channel_lock(chan);
 							ast_cdr_setaccount(chan, buf);
+							ast_channel_unlock(chan);
+						}
 						break;
 					}
 				} else {
 					if (!strcmp(passwd, buf)) {
-						if (ast_test_flag(&flags, OPT_ACCOUNT))
+						if (ast_test_flag(&flags, OPT_ACCOUNT)) {
+							ast_channel_lock(chan);
 							ast_cdr_setaccount(chan, buf);
+							ast_channel_unlock(chan);
+						}
 						break;
 					}
 				}
@@ -242,8 +248,11 @@
 	}
 
 	if ((retries < 3) && !res) {
-		if (ast_test_flag(&flags,OPT_ACCOUNT) && !ast_test_flag(&flags,OPT_MULTIPLE))
+		if (ast_test_flag(&flags,OPT_ACCOUNT) && !ast_test_flag(&flags,OPT_MULTIPLE)) {
+			ast_channel_lock(chan);
 			ast_cdr_setaccount(chan, passwd);
+			ast_channel_unlock(chan);
+		}
 		if (!(res = ast_streamfile(chan, "auth-thankyou", chan->language)))
 			res = ast_waitstream(chan, "");
 	} else {

Modified: branches/1.8/apps/app_followme.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/apps/app_followme.c?view=diff&rev=348362&r1=348361&r2=348362
==============================================================================
--- branches/1.8/apps/app_followme.c (original)
+++ branches/1.8/apps/app_followme.c Fri Dec 16 14:55:17 2011
@@ -478,10 +478,12 @@
 
 	if (tmpuser && tmpuser->ochan && tmpuser->state >= 0) {
 		outbound = tmpuser->ochan;
+		ast_channel_lock(outbound);
 		if (!outbound->cdr) {
 			outbound->cdr = ast_cdr_alloc();
-			if (outbound->cdr)
+			if (outbound->cdr) {
 				ast_cdr_init(outbound->cdr, outbound);
+			}
 		}
 		if (outbound->cdr) {
 			char tmp[256];
@@ -492,11 +494,15 @@
 			ast_cdr_start(outbound->cdr);
 			ast_cdr_end(outbound->cdr);
 			/* If the cause wasn't handled properly */
-			if (ast_cdr_disposition(outbound->cdr, outbound->hangupcause))
+			if (ast_cdr_disposition(outbound->cdr, outbound->hangupcause)) {
 				ast_cdr_failed(outbound->cdr);
-		} else
+			}
+		} else {
 			ast_log(LOG_WARNING, "Unable to create Call Detail Record\n");
-		ast_hangup(tmpuser->ochan);
+		}
+		ast_channel_unlock(outbound);
+		ast_hangup(outbound);
+		tmpuser->ochan = NULL;
 	}
 }
 
@@ -876,6 +882,7 @@
 					AST_LIST_INSERT_TAIL(findme_user_list, tmpuser, entry);
 				} else {
 					ast_verb(3, "couldn't reach at this number.\n");
+					ast_channel_lock(outbound);
 					if (!outbound->cdr) {
 						outbound->cdr = ast_cdr_alloc();
 					}
@@ -895,6 +902,7 @@
 					} else {
 						ast_log(LOG_ERROR, "Unable to create Call Detail Record\n");
 					}
+					ast_channel_unlock(outbound);
 					ast_hangup(outbound);
 					ast_free(tmpuser);
 				}

Modified: branches/1.8/apps/app_queue.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/apps/app_queue.c?view=diff&rev=348362&r1=348361&r2=348362
==============================================================================
--- branches/1.8/apps/app_queue.c (original)
+++ branches/1.8/apps/app_queue.c Fri Dec 16 14:55:17 2011
@@ -4988,9 +4988,9 @@
 			if ((strcasecmp(cdr->uniqueid, qe->chan->uniqueid)) &&
 			    (strcasecmp(cdr->linkedid, qe->chan->uniqueid)) &&
 			    (newcdr = ast_cdr_dup(cdr))) {
+				ast_channel_lock(qe->chan);
 				ast_cdr_init(newcdr, qe->chan);
 				ast_cdr_reset(newcdr, 0);
-				ast_channel_lock(qe->chan);
 				cdr = ast_cdr_append(cdr, newcdr);
 				cdr = cdr->next;
 				ast_channel_unlock(qe->chan);

Modified: branches/1.8/funcs/func_cdr.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/funcs/func_cdr.c?view=diff&rev=348362&r1=348361&r2=348362
==============================================================================
--- branches/1.8/funcs/func_cdr.c (original)
+++ branches/1.8/funcs/func_cdr.c Fri Dec 16 14:55:17 2011
@@ -197,17 +197,21 @@
 {
 	char *ret;
 	struct ast_flags flags = { 0 };
-	struct ast_cdr *cdr = chan ? chan->cdr : NULL;
+	struct ast_cdr *cdr;
 	AST_DECLARE_APP_ARGS(args,
 			     AST_APP_ARG(variable);
 			     AST_APP_ARG(options);
 	);
 
-	if (ast_strlen_zero(parse))
+	if (ast_strlen_zero(parse) || !chan)
 		return -1;
 
-	if (!cdr)
+	ast_channel_lock(chan);
+	cdr = chan->cdr;
+	if (!cdr) {
+		ast_channel_unlock(chan);
 		return -1;
+	}
 
 	AST_STANDARD_APP_ARGS(args, parse);
 
@@ -255,13 +259,14 @@
 				   ast_test_flag(&flags, OPT_UNPARSED));
 	}
 
+	ast_channel_unlock(chan);
 	return ret ? 0 : -1;
 }
 
 static int cdr_write(struct ast_channel *chan, const char *cmd, char *parse,
 		     const char *value)
 {
-	struct ast_cdr *cdr = chan ? chan->cdr : NULL;
+	struct ast_cdr *cdr;
 	struct ast_flags flags = { 0 };
 	AST_DECLARE_APP_ARGS(args,
 			     AST_APP_ARG(variable);
@@ -271,8 +276,12 @@
 	if (ast_strlen_zero(parse) || !value || !chan)
 		return -1;
 
-	if (!cdr)
+	ast_channel_lock(chan);
+	cdr = chan->cdr;
+	if (!cdr) {
+		ast_channel_unlock(chan);
 		return -1;
+	}
 
 	AST_STANDARD_APP_ARGS(args, parse);
 
@@ -296,6 +305,7 @@
 		/* No need to worry about the u flag, as all fields for which setting
 		 * 'u' would do anything are marked as readonly. */
 
+	ast_channel_unlock(chan);
 	return 0;
 }
 

Modified: branches/1.8/include/asterisk/cdr.h
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/include/asterisk/cdr.h?view=diff&rev=348362&r1=348361&r2=348362
==============================================================================
--- branches/1.8/include/asterisk/cdr.h (original)
+++ branches/1.8/include/asterisk/cdr.h Fri Dec 16 14:55:17 2011
@@ -211,6 +211,7 @@
  * \param cdr Call Detail Record to use for channel
  * \param chan Channel to bind CDR with
  * Initializes a CDR and associates it with a particular channel
+ * \note The channel should be locked before calling.
  * \return 0 by default
  */
 int ast_cdr_init(struct ast_cdr *cdr, struct ast_channel *chan);
@@ -220,6 +221,7 @@
  * \param cdr Call Detail Record to use for channel
  * \param chan Channel to bind CDR with
  * Initializes a CDR and associates it with a particular channel
+ * \note The channel should be locked before calling.
  * \return 0 by default
  */
 int ast_cdr_setcid(struct ast_cdr *cdr, struct ast_channel *chan);
@@ -403,22 +405,40 @@
  */
 void ast_cdr_merge(struct ast_cdr *to, struct ast_cdr *from);
 
-/*! \brief Set account code, will generate AMI event */
+/*!
+ * \brief Set account code, will generate AMI event
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_setaccount(struct ast_channel *chan, const char *account);
 
-/*! \brief Set the peer account */
+/*!
+ * \brief Set the peer account
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_setpeeraccount(struct ast_channel *chan, const char *account);
 
-/*! \brief Set AMA flags for channel */
+/*!
+ * \brief Set AMA flags for channel
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_setamaflags(struct ast_channel *chan, const char *amaflags);
 
-/*! \brief Set CDR user field for channel (stored in CDR) */
+/*!
+ * \brief Set CDR user field for channel (stored in CDR)
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_setuserfield(struct ast_channel *chan, const char *userfield);
-/*! \brief Append to CDR user field for channel (stored in CDR) */
+/*!
+ * \brief Append to CDR user field for channel (stored in CDR)
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_appenduserfield(struct ast_channel *chan, const char *userfield);
 
 
-/*! Update CDR on a channel */
+/*!
+ * \brief Update CDR on a channel
+ * \note The channel should be locked before calling.
+ */
 int ast_cdr_update(struct ast_channel *chan);
 
 

Modified: branches/1.8/main/channel.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/channel.c?view=diff&rev=348362&r1=348361&r2=348362
==============================================================================
--- branches/1.8/main/channel.c (original)
+++ branches/1.8/main/channel.c Fri Dec 16 14:55:17 2011
@@ -5242,7 +5242,9 @@
 			ast_channel_datastore_inherit(oh->parent_channel, new);
 		}
 		if (oh->account) {
+			ast_channel_lock(new);
 			ast_cdr_setaccount(new, oh->account);
+			ast_channel_lock(new);
 		}
 	} else if (caller) { /* no outgoing helper so use caller if avaliable */
 		ast_channel_update_redirecting(caller, apr, NULL);
@@ -5308,8 +5310,11 @@
 			ast_channel_inherit_variables(oh->parent_channel, chan);
 			ast_channel_datastore_inherit(oh->parent_channel, chan);
 		}
-		if (oh->account)
-			ast_cdr_setaccount(chan, oh->account);	
+		if (oh->account) {
+			ast_channel_lock(chan);
+			ast_cdr_setaccount(chan, oh->account);
+			ast_channel_unlock(chan);
+		}
 	}
 
 	ast_set_callerid(chan, cid_num, cid_name, cid_num);
@@ -5421,21 +5426,27 @@
 		*outstate = AST_CONTROL_ANSWER;
 
 	if (res <= 0) {
-		if ( AST_CONTROL_RINGING == last_subclass ) 
+		ast_channel_lock(chan);
+		if (AST_CONTROL_RINGING == last_subclass) {
 			chan->hangupcause = AST_CAUSE_NO_ANSWER;
-		if (!chan->cdr && (chan->cdr = ast_cdr_alloc()))
+		}
+		if (!chan->cdr && (chan->cdr = ast_cdr_alloc())) {
 			ast_cdr_init(chan->cdr, chan);
+		}
 		if (chan->cdr) {
 			char tmp[256];
+
 			snprintf(tmp, sizeof(tmp), "%s/%s", type, (char *)data);
-			ast_cdr_setapp(chan->cdr,"Dial",tmp);
+			ast_cdr_setapp(chan->cdr, "Dial", tmp);
 			ast_cdr_update(chan);
 			ast_cdr_start(chan->cdr);
 			ast_cdr_end(chan->cdr);
 			/* If the cause wasn't handled properly */
-			if (ast_cdr_disposition(chan->cdr,chan->hangupcause))
+			if (ast_cdr_disposition(chan->cdr, chan->hangupcause)) {
 				ast_cdr_failed(chan->cdr);
-		}
+			}
+		}
+		ast_channel_unlock(chan);
 		ast_hangup(chan);
 		chan = NULL;
 	}

Modified: branches/1.8/main/features.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/features.c?view=diff&rev=348362&r1=348361&r2=348362
==============================================================================
--- branches/1.8/main/features.c (original)
+++ branches/1.8/main/features.c Fri Dec 16 14:55:17 2011
@@ -2303,6 +2303,7 @@
 	pbx_builtin_setvar_helper(transferer, "BLINDTRANSFER", transferee->name);
 	pbx_builtin_setvar_helper(transferee, "BLINDTRANSFER", transferer->name);
 	finishup(transferee);
+	ast_channel_lock(transferer);
 	if (!transferer->cdr) { /* this code should never get called (in a perfect world) */
 		transferer->cdr = ast_cdr_alloc();
 		if (transferer->cdr) {
@@ -2310,6 +2311,7 @@
 			ast_cdr_start(transferer->cdr);
 		}
 	}
+	ast_channel_unlock(transferer);
 	if (transferer->cdr) {
 		struct ast_cdr *swap = transferer->cdr;
 
@@ -3897,11 +3899,15 @@
 	/* copy the userfield from the B-leg to A-leg if applicable */
 	if (chan->cdr && peer->cdr && !ast_strlen_zero(peer->cdr->userfield)) {
 		char tmp[256];
+
+		ast_channel_lock(chan);
 		if (!ast_strlen_zero(chan->cdr->userfield)) {
 			snprintf(tmp, sizeof(tmp), "%s;%s", chan->cdr->userfield, peer->cdr->userfield);
 			ast_cdr_appenduserfield(chan, tmp);
-		} else
+		} else {
 			ast_cdr_setuserfield(chan, peer->cdr->userfield);
+		}
+		ast_channel_unlock(chan);
 		/* Don't delete the CDR; just disable it. */
 		ast_set_flag(peer->cdr, AST_CDR_FLAG_POST_DISABLED);
 		we_disabled_peer_cdr = 1;
@@ -3910,7 +3916,7 @@
 	ast_copy_string(orig_peername,peer->name,sizeof(orig_peername));
 
 	if (!chan_cdr || (chan_cdr && !ast_test_flag(chan_cdr, AST_CDR_FLAG_POST_DISABLED))) {
-		
+		ast_channel_lock_both(chan, peer);
 		if (chan_cdr) {
 			ast_set_flag(chan_cdr, AST_CDR_FLAG_MAIN);
 			ast_cdr_update(chan);
@@ -3925,7 +3931,6 @@
 				ast_copy_string(bridge_cdr->userfield, peer_cdr->userfield, sizeof(bridge_cdr->userfield));
 			}
 			ast_cdr_setaccount(peer, chan->accountcode);
-
 		} else {
 			/* better yet, in a xfer situation, find out why the chan cdr got zapped (pun unintentional) */
 			bridge_cdr = ast_cdr_alloc(); /* this should be really, really rare/impossible? */
@@ -3948,6 +3953,9 @@
 				ast_cdr_start(bridge_cdr);
 			}
 		}
+		ast_channel_unlock(chan);
+		ast_channel_unlock(peer);
+
 		ast_debug(4,"bridge answer set, chan answer set\n");
 		/* peer_cdr->answer will be set when a macro runs on the peer;
 		   in that case, the bridge answer will be delayed while the

Modified: branches/1.8/main/pbx.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/main/pbx.c?view=diff&rev=348362&r1=348361&r2=348362
==============================================================================
--- branches/1.8/main/pbx.c (original)
+++ branches/1.8/main/pbx.c Fri Dec 16 14:55:17 2011
@@ -4952,10 +4952,12 @@
 			ast_copy_string(c->context, "default", sizeof(c->context));
 		}
 	}
+	ast_channel_lock(c);
 	if (c->cdr) {
 		/* allow CDR variables that have been collected after channel was created to be visible during call */
 		ast_cdr_update(c);
 	}
+	ast_channel_unlock(c);
 	for (;;) {
 		char dst_exten[256];	/* buffer to accumulate digits */
 		int pos = 0;		/* XXX should check bounds */
@@ -5062,8 +5064,11 @@
 					}
 					/* Call timed out with no special extension to jump to. */
 				}
-				if (c->cdr)
+				ast_channel_lock(c);
+				if (c->cdr) {
 					ast_cdr_update(c);
+				}
+				ast_channel_unlock(c);
 				error = 1;
 				break;
 			}
@@ -5167,10 +5172,12 @@
 					}
 				}
 			}
+			ast_channel_lock(c);
 			if (c->cdr) {
 				ast_verb(2, "CDR updated on %s\n",c->name);
 				ast_cdr_update(c);
 			}
+			ast_channel_unlock(c);
 		}
 	}
 
@@ -9409,7 +9416,9 @@
 static int pbx_builtin_setamaflags(struct ast_channel *chan, const char *data)
 {
 	/* Copy the AMA Flags as specified */
+	ast_channel_lock(chan);
 	ast_cdr_setamaflags(chan, data ? data : "");
+	ast_channel_unlock(chan);
 	return 0;
 }
 

Modified: branches/1.8/res/res_monitor.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.8/res/res_monitor.c?view=diff&rev=348362&r1=348361&r2=348362
==============================================================================
--- branches/1.8/res/res_monitor.c (original)
+++ branches/1.8/res/res_monitor.c Fri Dec 16 14:55:17 2011
@@ -684,9 +684,13 @@
 	if (urlprefix) {
 		snprintf(tmp, sizeof(tmp), "%s/%s.%s", urlprefix, args.fname_base,
 			((strcmp(args.format, "gsm")) ? "wav" : "gsm"));
-		if (!chan->cdr && !(chan->cdr = ast_cdr_alloc()))
+		ast_channel_lock(chan);
+		if (!chan->cdr && !(chan->cdr = ast_cdr_alloc())) {
+			ast_channel_unlock(chan);
 			return -1;
+		}
 		ast_cdr_setuserfield(chan, tmp);
+		ast_channel_unlock(chan);
 	}
 	if (waitforbridge) {
 		/* We must remove the "b" option if listed.  In principle none of




More information about the asterisk-commits mailing list