[Asterisk-code-review] res_pjsip_outbound_registration: Fix SRV failover on timeout (asterisk[16])

[George Joseph]

George Joseph has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/13804 )


Change subject: res_pjsip_outbound_registration: Fix SRV failover on timeout
......................................................................

res_pjsip_outbound_registration: Fix SRV failover on timeout

In order to retry outbound registrations for some situations, we
need access to the tdata from the original request.  For instance
for 401/407 responses, we need it to properly construct the
subsequent request with the authentication.  For timeouts, we need
it so we can skip any failed SRV entries.  We used to get the tdata
from the response rdata/transaction but that only worked for the
failures where there was actually a response.  For a timeout
there's no response and therefore no rdata or transaction.  This
wasn't an issue if we we're traversing SRV entries, we just retried
that same server.  If there were SRV entries though, without the
original tdata, we kept doing DNS queries and trying the same
failed server over and over instead of skipping to the next one in
the list.

* Added a "last_tdata" member to the client state structure to keep
  track of the sent tdata.

* Updated registration_client_send() to save the tdata it used into
  the client_state.

* Updated sip_outbound_registration_response_cb() to use the tdata
  saved in client_state instead of getting it from the response
  rdata/transaction.

Change-Id: I199b8274392d17661dd3ce3b4d69a3968368fa06
---
M res/res_pjsip_outbound_registration.c
1 file changed, 37 insertions(+), 6 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/04/13804/1

diff --git a/res/res_pjsip_outbound_registration.c b/res/res_pjsip_outbound_registration.c
index 9ea6054..420c262 100644
--- a/res/res_pjsip_outbound_registration.c
+++ b/res/res_pjsip_outbound_registration.c
@@ -334,6 +334,14 @@
 	 * module unload.
 	 */
 	pjsip_regc *client;
+	/*!
+	 * \brief Last tdata sent
+	 * We need the original tdata to resend a request on auth failure
+	 * or timeout.  On an auth failure, we use the original tdata
+	 * to initialize the new tdata for the authorized response.  On a timeout
+	 * we need it to skip failed SRV entries if any.
+	 */
+	pjsip_tx_data *last_tdata;
 	/*! \brief Timer entry for retrying on temporal responses */
 	pj_timer_entry timer;
 	/*! \brief Optional line parameter placed into Contact */
@@ -544,6 +552,13 @@
 	/* Due to the message going out the callback may now be invoked, so bump the count */
 	ao2_ref(client_state, +1);
 	/*
+	 * We also bump tdata in expectation of saving it to client_state->last_tdata.
+	 * We have to do it BEFORE pjsip_regc_send because if it succeeds, it decrements
+	 * the ref count on its own.
+	 */
+	pjsip_tx_data_add_ref(tdata);
+
+	/*
 	 * Set the transport in case transports were reloaded.
 	 * When pjproject removes the extraneous error messages produced,
 	 * we can check status and only set the transport and resend if there was an error
@@ -552,13 +567,26 @@
 	pjsip_regc_set_transport(client_state->client, &selector);
 	status = pjsip_regc_send(client_state->client, tdata);
 
-	/* If the attempt to send the message failed and the callback was not invoked we need to
-	 * drop the reference we just added
+	/*
+	 * If the attempt to send the message failed and the callback was not invoked we need to
+	 * drop the references we just added
 	 */
 	if ((status != PJ_SUCCESS) && !(*callback_invoked)) {
+		pjsip_tx_data_dec_ref(tdata);
 		ao2_ref(client_state, -1);
+		return status;
 	}
 
+	/*
+	 * Decref the old last_data before replacing it.
+	 * BTW, it's quite possible that last_data == tdata
+	 * if we're trying successive servers in an SRV set.
+	 */
+	if (client_state->last_tdata) {
+		pjsip_tx_data_dec_ref(client_state->last_tdata);
+	}
+	client_state->last_tdata = tdata;
+
 	return status;
 }
 
@@ -1072,17 +1100,17 @@
 
 	if (param->rdata) {
 		struct pjsip_retry_after_hdr *retry_after;
-		pjsip_transaction *tsx;
 
 		retry_after = pjsip_msg_find_hdr(param->rdata->msg_info.msg, PJSIP_H_RETRY_AFTER,
 			NULL);
 		response->retry_after = retry_after ? retry_after->ivalue : 0;
-		tsx = pjsip_rdata_get_tsx(param->rdata);
-		response->old_request = tsx->last_tx;
-		pjsip_tx_data_add_ref(response->old_request);
 		pjsip_rx_data_clone(param->rdata, 0, &response->rdata);
 	}
 
+	/* old_request steals the reference */
+	response->old_request = client_state->last_tdata;
+	client_state->last_tdata = NULL;
+
 	/*
 	 * Transfer response reference to serializer task so the
 	 * nominal path will not dec the response ref in this
@@ -1127,6 +1155,9 @@
 	ast_taskprocessor_unreference(client_state->serializer);
 	ast_free(client_state->transport_name);
 	ast_free(client_state->registration_name);
+	if (client_state->last_tdata) {
+		pjsip_tx_data_dec_ref(client_state->last_tdata);
+	}
 }
 
 /*! \brief Allocator function for registration state */

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13804
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I199b8274392d17661dd3ce3b4d69a3968368fa06
Gerrit-Change-Number: 13804
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200213/4357c05a/attachment-0001.html>