[Asterisk-code-review] res pjsip outbound registration.c: Fix handle client state d... (asterisk[master])
Richard Mudgett
asteriskteam at digium.com
Wed Jun 24 10:57:00 CDT 2015
Richard Mudgett has uploaded a new change for review.
https://gerrit.asterisk.org/707
Change subject: res_pjsip_outbound_registration.c: Fix handle_client_state_destruction() refs
......................................................................
res_pjsip_outbound_registration.c: Fix handle_client_state_destruction() refs
* handle_client_state_destruction() must always be passed a ref to
client_state because it will always unref client_state.
handle_registration_response() was not passing a client_state ref.
* Made the final un-REGISTER message get sent normally using the pjproject
register control structure in handle_client_state_destruction(). The
previous code attempted to short circuit the response handling for the
module to unload. That doesn't work for a couple reasons. One,
pjsip_regc_send() may call the registered callback before it returns and
unbalance the client_state ref count. Two, the registered callback
handles any authentication for the un-REGISTER message.
* Removed references to client_state->client outside of the serializer
thread. When handle_client_state_destruction() destroys the pjproject
register control structure that memory is freed and cannot be referenced
anymore. These accesses were to provide information for debug and
off-nominal warning messages.
* In sip_outbound_registration_timer_cb() you should not access entry->id
after unrefing client_state because the passed in entry is normally
pointing to the timer entry in the client_state object.
ASTERISK-24907
Reported by: Kevin Harwell
Change-Id: Ia7b446d8644b6b4550ef5bea49527671de65183f
---
M res/res_pjsip_outbound_registration.c
1 file changed, 63 insertions(+), 40 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/07/707/1
diff --git a/res/res_pjsip_outbound_registration.c b/res/res_pjsip_outbound_registration.c
index e459fa1..d40dfa9 100644
--- a/res/res_pjsip_outbound_registration.c
+++ b/res/res_pjsip_outbound_registration.c
@@ -215,6 +215,8 @@
SIP_REGISTRATION_REJECTED_TEMPORARY,
/*! \brief Registration was rejected, permanently */
SIP_REGISTRATION_REJECTED_PERMANENT,
+ /*! \brief Registration is stopping. */
+ SIP_REGISTRATION_STOPPING,
/*! \brief Registration has been stopped */
SIP_REGISTRATION_STOPPED,
};
@@ -224,6 +226,7 @@
[SIP_REGISTRATION_REGISTERED] = "Registered",
[SIP_REGISTRATION_REJECTED_TEMPORARY] = "Rejected",
[SIP_REGISTRATION_REJECTED_PERMANENT] = "Rejected",
+ [SIP_REGISTRATION_STOPPING] = "Stopped",
[SIP_REGISTRATION_STOPPED] = "Stopped",
};
@@ -268,7 +271,12 @@
struct sip_outbound_registration_client_state {
/*! \brief Current status of this registration */
enum sip_outbound_registration_status status;
- /*! \brief Outbound registration client */
+ /*!
+ * \brief Outbound registration client
+ * \note May only be accessed within the serializer thread
+ * because it might get destroyed and set to NULL for
+ * module unload.
+ */
pjsip_regc *client;
/*! \brief Timer entry for retrying on temporal responses */
pj_timer_entry timer;
@@ -488,8 +496,8 @@
pjsip_regc_get_info(client_state->client, &info);
ast_copy_pj_str(server_uri, &info.server_uri, sizeof(server_uri));
ast_copy_pj_str(client_uri, &info.client_uri, sizeof(client_uri));
- ast_debug(3, "REGISTER attempt %u to '%s' with client '%s'\n",
- client_state->retries + 1, server_uri, client_uri);
+ ast_debug(1, "Outbound REGISTER attempt %u to '%s' with client '%s'\n",
+ client_state->retries + 1, server_uri, client_uri);
if (client_state->support_path) {
pjsip_supported_hdr *hdr;
@@ -518,23 +526,15 @@
static void sip_outbound_registration_timer_cb(pj_timer_heap_t *timer_heap, struct pj_timer_entry *entry)
{
struct sip_outbound_registration_client_state *client_state = entry->user_data;
- pjsip_regc_info info;
- pjsip_regc_get_info(client_state->client, &info);
- ast_debug(1, "Attempting scheduled outbound registration attempt to server '%.*s' from client '%.*s'\n",
- (int) info.server_uri.slen, info.server_uri.ptr,
- (int) info.client_uri.slen, info.client_uri.ptr);
+ entry->id = 0;
ao2_ref(client_state, +1);
if (ast_sip_push_task(client_state->serializer, handle_client_registration, client_state)) {
- ast_log(LOG_WARNING, "Scheduled outbound registration to server '%.*s' from client '%.*s' could not be executed\n",
- (int) info.server_uri.slen, info.server_uri.ptr,
- (int) info.client_uri.slen, info.client_uri.ptr);
+ ast_log(LOG_WARNING, "Scheduled outbound registration could not be executed.\n");
ao2_ref(client_state, -1);
}
ao2_ref(client_state, -1);
-
- entry->id = 0;
}
/*! \brief Helper function which sets up the timer to re-register in a specific amount of time */
@@ -563,35 +563,58 @@
/*! \brief Callback function for unregistering (potentially) and destroying state */
static int handle_client_state_destruction(void *data)
{
- RAII_VAR(struct sip_outbound_registration_client_state *, client_state, data, ao2_cleanup);
+ struct sip_outbound_registration_client_state *client_state = data;
cancel_registration(client_state);
if (client_state->client) {
pjsip_regc_info info;
+ pjsip_tx_data *tdata;
pjsip_regc_get_info(client_state->client, &info);
if (info.is_busy == PJ_TRUE) {
/* If a client transaction is in progress we defer until it is complete */
+ ast_debug(1,
+ "Registration transaction is busy with server '%.*s' from client '%.*s'.\n",
+ (int) info.server_uri.slen, info.server_uri.ptr,
+ (int) info.client_uri.slen, info.client_uri.ptr);
client_state->destroy = 1;
+ ao2_ref(client_state, -1);
return 0;
}
- if (client_state->status != SIP_REGISTRATION_UNREGISTERED
- && client_state->status != SIP_REGISTRATION_REJECTED_PERMANENT) {
- pjsip_tx_data *tdata;
+ switch (client_state->status) {
+ case SIP_REGISTRATION_UNREGISTERED:
+ break;
+ case SIP_REGISTRATION_REGISTERED:
+ ast_debug(1,
+ "Trying to unregister with server '%.*s' from client '%.*s' before destruction.\n",
+ (int) info.server_uri.slen, info.server_uri.ptr,
+ (int) info.client_uri.slen, info.client_uri.ptr);
- if (pjsip_regc_unregister(client_state->client, &tdata) == PJ_SUCCESS) {
- pjsip_regc_send(client_state->client, tdata);
+ client_state->status = SIP_REGISTRATION_STOPPING;
+ client_state->destroy = 1;
+ if (pjsip_regc_unregister(client_state->client, &tdata) == PJ_SUCCESS
+ && registration_client_send(client_state, tdata) == PJ_SUCCESS) {
+ ao2_ref(client_state, -1);
+ return 0;
}
+ break;
+ case SIP_REGISTRATION_REJECTED_TEMPORARY:
+ case SIP_REGISTRATION_REJECTED_PERMANENT:
+ case SIP_REGISTRATION_STOPPING:
+ case SIP_REGISTRATION_STOPPED:
+ break;
}
pjsip_regc_destroy(client_state->client);
+ client_state->client = NULL;
}
client_state->status = SIP_REGISTRATION_STOPPED;
ast_sip_auth_vector_destroy(&client_state->outbound_auths);
+ ao2_ref(client_state, -1);
return 0;
}
@@ -668,20 +691,19 @@
/*! \brief Callback function for handling a response to a registration attempt */
static int handle_registration_response(void *data)
{
- RAII_VAR(struct registration_response *, response, data, ao2_cleanup);
+ struct registration_response *response = data;
pjsip_regc_info info;
char server_uri[PJSIP_MAX_URL_SIZE];
char client_uri[PJSIP_MAX_URL_SIZE];
+ if (response->client_state->status == SIP_REGISTRATION_STOPPED) {
+ ao2_ref(response, -1);
+ return 0;
+ }
+
pjsip_regc_get_info(response->client_state->client, &info);
ast_copy_pj_str(server_uri, &info.server_uri, sizeof(server_uri));
ast_copy_pj_str(client_uri, &info.client_uri, sizeof(client_uri));
-
- if (response->client_state->status == SIP_REGISTRATION_STOPPED) {
- ast_debug(1, "Not handling registration response from server '%s' for client '%s'. Registration already stopped\n",
- server_uri, client_uri);
- return 0;
- }
ast_debug(1, "Processing REGISTER response %d from server '%s' for client '%s'\n",
response->code, server_uri, client_uri);
@@ -694,10 +716,10 @@
response->client_state->auth_attempted = 1;
ast_debug(1, "Sending authenticated REGISTER to server '%s' from client '%s'\n",
server_uri, client_uri);
- if (registration_client_send(response->client_state, tdata) != PJ_SUCCESS) {
- response->client_state->auth_attempted = 0;
+ if (registration_client_send(response->client_state, tdata) == PJ_SUCCESS) {
+ ao2_ref(response, -1);
+ return 0;
}
- return 0;
} else {
ast_log(LOG_WARNING, "Failed to create authenticated REGISTER request to server '%s' from client '%s'\n",
server_uri, client_uri);
@@ -719,6 +741,8 @@
ast_debug(1, "Outbound unregistration to '%s' with client '%s' successful\n", server_uri, client_uri);
response->client_state->status = SIP_REGISTRATION_UNREGISTERED;
}
+ } else if (response->client_state->destroy) {
+ /* We need to deal with the pending destruction instead. */
} else if (response->retry_after) {
/* If we have been instructed to retry after a period of time, schedule it as such */
schedule_retry(response, response->retry_after, server_uri, client_uri);
@@ -759,20 +783,21 @@
ast_system_publish_registry("PJSIP", client_uri, server_uri,
sip_outbound_registration_status_str[response->client_state->status], NULL);
- /* If deferred destruction is in use see if we need to destroy now */
if (response->client_state->destroy) {
+ /* We have a pending deferred destruction to complete now. */
+ ao2_ref(response->client_state, +1);
handle_client_state_destruction(response->client_state);
}
+ ao2_ref(response, -1);
return 0;
}
/*! \brief Callback function for outbound registration client */
static void sip_outbound_registration_response_cb(struct pjsip_regc_cbparam *param)
{
- RAII_VAR(struct sip_outbound_registration_client_state *, client_state, param->token, ao2_cleanup);
+ struct sip_outbound_registration_client_state *client_state = param->token;
struct registration_response *response;
- pjsip_regc_info info;
int *callback_invoked;
callback_invoked = ast_threadstorage_get(®ister_callback_invoked, sizeof(int));
@@ -784,18 +809,16 @@
response = ao2_alloc(sizeof(*response), registration_response_destroy);
if (!response) {
+ ao2_ref(client_state, -1);
return;
}
response->code = param->code;
response->expiration = param->expiration;
+ /* Transfer client_state reference to response. */
response->client_state = client_state;
- ao2_ref(response->client_state, +1);
- pjsip_regc_get_info(client_state->client, &info);
- ast_debug(1, "Received REGISTER response %d(%.*s) from server '%.*s' for client '%.*s\n",
- param->code, (int) param->reason.slen, param->reason.ptr,
- (int) info.server_uri.slen, info.server_uri.ptr,
- (int) info.client_uri.slen, info.client_uri.ptr);
+ ast_debug(1, "Received REGISTER response %d(%.*s)\n",
+ param->code, (int) param->reason.slen, param->reason.ptr);
if (param->rdata) {
struct pjsip_retry_after_hdr *retry_after;
@@ -1053,8 +1076,8 @@
}
}
- if (!state->client_state->client
- && pjsip_regc_create(ast_sip_get_pjsip_endpoint(), state->client_state,
+ ast_assert(state->client_state->client == NULL);
+ if (pjsip_regc_create(ast_sip_get_pjsip_endpoint(), state->client_state,
sip_outbound_registration_response_cb,
&state->client_state->client) != PJ_SUCCESS) {
return -1;
--
To view, visit https://gerrit.asterisk.org/707
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia7b446d8644b6b4550ef5bea49527671de65183f
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
More information about the asterisk-code-review
mailing list