[Asterisk-code-review] res pjsip outbound registration: Fix double unref on error r... (asterisk[13])

[Joshua Colp]

Joshua Colp has uploaded a new change for review.

  https://gerrit.asterisk.org/306

Change subject: res_pjsip_outbound_registration: Fix double unref on error return.
......................................................................

res_pjsip_outbound_registration: Fix double unref on error return.

When the PJSIP pjsip_regc_send function is invoked and an error
status returned the caller currently decrements the reference count
of the client state that it just incremented, assuming the
registration callback would not have been invoked. In practice
this is not correct. If the failure happens after the transaction
has been set up the callback will still be invoked. This will
cause the reference count to be incorrectly decremented twice, once
by the registration callback and second by the caller of
pjsip_regc_send.

This change makes it so that whether the callback is invoked or
not is known by the caller of pjsip_regc_send. Depending on
this it can know whether it is responsible for decrementing the
reference count of the client state or not.

ASTERISK-25037 #close
Reported by: Joshua Colp

Change-Id: I749dc12f3a22115c49c5d7d95ff42a5fa45319de
---
M res/res_pjsip_outbound_registration.c
1 file changed, 40 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/06/306/1

diff --git a/res/res_pjsip_outbound_registration.c b/res/res_pjsip_outbound_registration.c
index 0547416..a4e4834 100644
--- a/res/res_pjsip_outbound_registration.c
+++ b/res/res_pjsip_outbound_registration.c
@@ -33,6 +33,7 @@
 #include "asterisk/taskprocessor.h"
 #include "asterisk/cli.h"
 #include "asterisk/stasis_system.h"
+#include "asterisk/threadstorage.h"
 #include "res_pjsip/include/res_pjsip_private.h"
 
 /*** DOCUMENTATION
@@ -178,6 +179,9 @@
 		</description>
 	</manager>
  ***/
+
+/*! \brief Some thread local storage used to determine if the running thread invoked the callback */
+AST_THREADSTORAGE(register_callback_invoked);
 
 /*! \brief Amount of buffer time (in seconds) before expiration that we re-register at */
 #define REREGISTER_BUFFER_TIME 10
@@ -365,6 +369,33 @@
 
 static pj_str_t PATH_NAME = { "path", 4 };
 
+/*! \brief Helper function which sends a message and cleans up, if needed, on failure */
+static pj_status_t registration_client_send(struct sip_outbound_registration_client_state *client_state,
+	pjsip_tx_data *tdata)
+{
+	pj_status_t status;
+	int *callback_invoked;
+
+	callback_invoked = ast_threadstorage_get(&register_callback_invoked, sizeof(int));
+	if (!callback_invoked) {
+		return PJ_ENOMEM;
+	}
+	*callback_invoked = 0;
+
+	/* Due to the message going out the callback may now be invoked, so bump the count */
+	ao2_ref(client_state, +1);
+	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 ((status != PJ_SUCCESS) && !(*callback_invoked)) {
+		ao2_ref(client_state, -1);
+	}
+
+	return status;
+}
+
 /*! \brief Callback function for registering */
 static int handle_client_registration(void *data)
 {
@@ -402,11 +433,7 @@
 		pj_strassign(&hdr->values[hdr->count++], &PATH_NAME);
 	}
 
-	/* Due to the registration the callback may now get called, so bump the ref count */
-	ao2_ref(client_state, +1);
-	if (pjsip_regc_send(client_state->client, tdata) != PJ_SUCCESS) {
-		ao2_ref(client_state, -1);
-	}
+	registration_client_send(client_state, tdata);
 
 	return 0;
 }
@@ -586,13 +613,11 @@
 		pjsip_tx_data *tdata;
 		if (!ast_sip_create_request_with_auth_from_old(&response->client_state->outbound_auths,
 				response->rdata, response->old_request, &tdata)) {
-			ao2_ref(response->client_state, +1);
 			response->client_state->auth_attempted = 1;
 			ast_debug(1, "Sending authenticated REGISTER to server '%s' from client '%s'\n",
 					server_uri, client_uri);
-			if (pjsip_regc_send(response->client_state->client, tdata) != PJ_SUCCESS) {
+			if (registration_client_send(response->client_state, tdata) != PJ_SUCCESS) {
 				response->client_state->auth_attempted = 0;
-				ao2_cleanup(response->client_state);
 			}
 			return 0;
 		} else {
@@ -668,8 +693,14 @@
 	RAII_VAR(struct sip_outbound_registration_client_state *, client_state, param->token, ao2_cleanup);
 	struct registration_response *response;
 	pjsip_regc_info info;
+	int *callback_invoked;
 
+	callback_invoked = ast_threadstorage_get(&register_callback_invoked, sizeof(int));
+
+	ast_assert(callback_invoked != NULL);
 	ast_assert(client_state != NULL);
+
+	*callback_invoked = 1;
 
 	response = ao2_alloc(sizeof(*response), registration_response_destroy);
 	if (!response) {
@@ -1121,10 +1152,7 @@
 		return 0;
 	}
 
-	ao2_ref(state->client_state, +1);
-	if (pjsip_regc_send(client, tdata) != PJ_SUCCESS) {
-		ao2_cleanup(state->client_state);
-	}
+	registration_client_send(state->client_state, tdata);
 
 	return 0;
 }

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I749dc12f3a22115c49c5d7d95ff42a5fa45319de
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Joshua Colp <jcolp at digium.com>