[Asterisk-code-review] res pjsip: Match dialogs on responses better. (asterisk[13])

[Anonymous Coward]

Anonymous Coward #1000019 has submitted this change and it was merged.

Change subject: res_pjsip: Match dialogs on responses better.
......................................................................


res_pjsip: Match dialogs on responses better.

When receiving an incoming response to a dialog-starting INVITE, we were
not matching the response to the INVITE dialog. Since we had not
recorded the to-tag to the dialog structure, the PJSIP-provided method
to find the dialog did not match.

Most of the time, this was not a problem, because there is a fall-back
that makes the response get routed to the same serializer that the
request was sent on. However, in cases where an asynchronous DNS lookup
occurs in the PJSIP core, the thread that sends the INVITE is not
actually a threadpool serializer thread. This means we are unable to
record a serializer to handle the incoming response.

Now, imagine what happens when an INVITE is sent on a non-serialized
thread, and an error response (such as a 486) arrives. The 486 ends up
getting put on some random threadpool thread. Eventually, a hangup task
gets queued on the INVITE dialog serializer. Since the 486 is being
handled on a different thread, the hangup task can execute at the same
time that the 486 is being handled. The hangup task assumes that it is
the sole owner of the INVITE session and channel, so it ends up
potentially freeing the channel and NULLing the session's channel
pointer. The thread handling the 486 can crash as a result.

This change has the incoming response match the INVITE transaction, and
then get the dialog from that transaction. It's the same method we had
been using for matching incoming CANCEL requests. By doing this, we get
the INVITE dialog and can ensure that the 486 response ends up being
handled by the same thread as the hangup, ensuring that the hangup runs
after the 486 has been completely handled.

ASTERISK-25941 #close
Reported by Javier Riveros

Change-Id: I0d4cc5d07e2a8d03e9db704d34bdef2ba60794a0
---
M res/res_pjsip/pjsip_distributor.c
1 file changed, 20 insertions(+), 7 deletions(-)

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



diff --git a/res/res_pjsip/pjsip_distributor.c b/res/res_pjsip/pjsip_distributor.c
index d902ed4..2ab954e 100644
--- a/res/res_pjsip/pjsip_distributor.c
+++ b/res/res_pjsip/pjsip_distributor.c
@@ -231,20 +231,33 @@
 	if (rdata->msg_info.msg->type == PJSIP_RESPONSE_MSG ||
 			pjsip_method_cmp(&rdata->msg_info.msg->line.req.method, &pjsip_cancel_method) ||
 			rdata->msg_info.to->tag.slen != 0) {
-		return pjsip_ua_find_dialog(&rdata->msg_info.cid->id, local_tag,
+		dlg = pjsip_ua_find_dialog(&rdata->msg_info.cid->id, local_tag,
 				remote_tag, PJ_TRUE);
+		if (dlg) {
+			return dlg;
+		}
 	}
 
-	/* Incoming CANCEL without a to-tag can't use same method for finding the
-	 * dialog. Instead, we have to find the matching INVITE transaction and
-	 * then get the dialog from the transaction
+	/*
+	 * There may still be a matching dialog if this is
+	 * 1) an incoming CANCEL request without a to-tag
+	 * 2) an incoming response to a dialog-creating request.
 	 */
-	pjsip_tsx_create_key(rdata->tp_info.pool, &tsx_key, PJSIP_ROLE_UAS,
-			pjsip_get_invite_method(), rdata);
+	if (rdata->msg_info.msg->type == PJSIP_REQUEST_MSG) {
+		/* CANCEL requests will need to match the INVITE we initially received. Any
+		 * other request type will either have been matched already or is not in
+		 * dialog
+		 */
+		pjsip_tsx_create_key(rdata->tp_info.pool, &tsx_key, PJSIP_ROLE_UAS,
+				pjsip_get_invite_method(), rdata);
+	} else {
+		pjsip_tsx_create_key(rdata->tp_info.pool, &tsx_key, PJSIP_ROLE_UAC,
+				&rdata->msg_info.cseq->method, rdata);
+	}
 
 	tsx = pjsip_tsx_layer_find_tsx(&tsx_key, PJ_TRUE);
 	if (!tsx) {
-		ast_log(LOG_ERROR, "Could not find matching INVITE transaction for CANCEL request\n");
+		ast_debug(3, "Could not find matching transaction for %s\n", rdata->msg_info.info);
 		return NULL;
 	}
 

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

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