[Asterisk-code-review] chan dahdi.c: Lock private struct for ast write(). (asterisk[11])

[Mark Michelson]

Mark Michelson has submitted this change and it was merged.

Change subject: chan_dahdi.c: Lock private struct for ast_write().
......................................................................


chan_dahdi.c: Lock private struct for ast_write().

There is a window of opportunity for DTMF to not go out if an audio frame
is in the process of being written to DAHDI while another thread starts
sending DTMF.  The thread sending the audio frame could be past the
currently dialing check before being preempted by another thread starting
a DTMF generation request.  When the thread sending the audio frame
resumes it will then cause DAHDI to stop the DTMF tone generation.  The
result is no DTMF goes out.

* Made dahdi_write() lock the private struct before writing to the DAHDI
file descriptor.

ASTERISK-25315
Reported by John Hardin

Change-Id: Ib4e0264cf63305ed5da701188447668e72ec9abb
---
M channels/chan_dahdi.c
1 file changed, 27 insertions(+), 12 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, approved
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, but someone else must approve



diff --git a/channels/chan_dahdi.c b/channels/chan_dahdi.c
index 23494e0..2a92c8e 100644
--- a/channels/chan_dahdi.c
+++ b/channels/chan_dahdi.c
@@ -9590,45 +9590,59 @@
 
 static int dahdi_write(struct ast_channel *ast, struct ast_frame *frame)
 {
-	struct dahdi_pvt *p = ast_channel_tech_pvt(ast);
+	struct dahdi_pvt *p;
 	int res;
 	int idx;
-	idx = dahdi_get_index(ast, p, 0);
-	if (idx < 0) {
-		ast_log(LOG_WARNING, "%s doesn't really exist?\n", ast_channel_name(ast));
-		return -1;
-	}
 
 	/* Write a frame of (presumably voice) data */
 	if (frame->frametype != AST_FRAME_VOICE) {
-		if (frame->frametype != AST_FRAME_IMAGE)
-			ast_log(LOG_WARNING, "Don't know what to do with frame type '%u'\n", frame->frametype);
+		if (frame->frametype != AST_FRAME_IMAGE) {
+			ast_log(LOG_WARNING, "Don't know what to do with frame type '%u'\n",
+				frame->frametype);
+		}
 		return 0;
 	}
 	if ((frame->subclass.format.id != AST_FORMAT_SLINEAR) &&
 		(frame->subclass.format.id != AST_FORMAT_ULAW) &&
 		(frame->subclass.format.id != AST_FORMAT_ALAW)) {
-		ast_log(LOG_WARNING, "Cannot handle frames in %s format\n", ast_getformatname(&frame->subclass.format));
+		ast_log(LOG_WARNING, "Cannot handle frames in %s format\n",
+			ast_getformatname(&frame->subclass.format));
 		return -1;
 	}
+
+	/* Return if it's not valid data */
+	if (!frame->data.ptr || !frame->datalen) {
+		return 0;
+	}
+
+	p = ast_channel_tech_pvt(ast);
+	ast_mutex_lock(&p->lock);
+
+	idx = dahdi_get_index(ast, p, 0);
+	if (idx < 0) {
+		ast_mutex_unlock(&p->lock);
+		ast_log(LOG_WARNING, "%s doesn't really exist?\n", ast_channel_name(ast));
+		return -1;
+	}
+
 	if (p->dialing) {
+		ast_mutex_unlock(&p->lock);
 		ast_debug(5, "Dropping frame since I'm still dialing on %s...\n",
 			ast_channel_name(ast));
 		return 0;
 	}
 	if (!p->owner) {
+		ast_mutex_unlock(&p->lock);
 		ast_debug(5, "Dropping frame since there is no active owner on %s...\n",
 			ast_channel_name(ast));
 		return 0;
 	}
 	if (p->cidspill) {
+		ast_mutex_unlock(&p->lock);
 		ast_debug(5, "Dropping frame since I've still got a callerid spill on %s...\n",
 			ast_channel_name(ast));
 		return 0;
 	}
-	/* Return if it's not valid data */
-	if (!frame->data.ptr || !frame->datalen)
-		return 0;
 
 	if (frame->subclass.format.id == AST_FORMAT_SLINEAR) {
 		if (!p->subs[idx].linear) {
@@ -9648,6 +9662,7 @@
 		}
 		res = my_dahdi_write(p, (unsigned char *)frame->data.ptr, frame->datalen, idx, 0);
 	}
+	ast_mutex_unlock(&p->lock);
 	if (res < 0) {
 		ast_log(LOG_WARNING, "write failed: %s\n", strerror(errno));
 		return -1;

-- 
To view, visit https://gerrit.asterisk.org/1061
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib4e0264cf63305ed5da701188447668e72ec9abb
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>