[asterisk-commits] rmudgett: trunk r423868 - in /trunk: ./ res/ res/res_pjsip/
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Wed Sep 24 13:35:51 CDT 2014
Author: rmudgett
Date: Wed Sep 24 13:35:47 2014
New Revision: 423868
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=423868
Log:
pjsip_options.c: Fix race condition stopping periodic out of dialog OPTIONS request.
The crash on the issues is a result of an invalid transport configuration
change when asterisk is restarted. The attempt to send the qualify
request fails and we cleaned up. However, the callback is also called
which results in a double unref of the objects involved.
* Put a wrapper around pjsip_endpt_send_request() to detect when the
passed in callback is called because of an error so callers can know to
not cleanup.
* Made send_request_cb() able to handle repeated challenges (Up to 10).
* Fix periodic endpoint qualify OPTIONS sched deletion race by avoiding
it. The sched entry will no longer self stop and must be externally
stopped.
* Added REF_DEBUG description tags to struct sched_data in
pjsip_options.c.
* Fix some off-nominal ref leaks in schedule_qualify(),
qualify_and_schedule().
* Reordered pjsip_options.c module start/stop code to cleanup better on
error.
ASTERISK-24295 #close
Reported by: Rogger Padilla
Review: https://reviewboard.asterisk.org/r/3954/
........
Merged revisions 423866 from http://svn.asterisk.org/svn/asterisk/branches/12
........
Merged revisions 423867 from http://svn.asterisk.org/svn/asterisk/branches/13
Modified:
trunk/ (props changed)
trunk/res/res_pjsip.c
trunk/res/res_pjsip/pjsip_options.c
Propchange: trunk/
------------------------------------------------------------------------------
Binary property 'branch-13-merged' - no diff available.
Modified: trunk/res/res_pjsip.c
URL: http://svnview.digium.com/svn/asterisk/trunk/res/res_pjsip.c?view=diff&rev=423868&r1=423867&r2=423868
==============================================================================
--- trunk/res/res_pjsip.c (original)
+++ trunk/res/res_pjsip.c Wed Sep 24 13:35:47 2014
@@ -2431,24 +2431,35 @@
return pj_stristr(&method, message_method) ? PJ_TRUE : PJ_FALSE;
}
+/*! Maximum number of challenges before assuming that we are in a loop */
+#define MAX_RX_CHALLENGES 10
+
/*! \brief Structure to hold information about an outbound request */
struct send_request_data {
- struct ast_sip_endpoint *endpoint; /*! The endpoint associated with this request */
- void *token; /*! Information to be provided to the callback upon receipt of a response */
- void (*callback)(void *token, pjsip_event *e); /*! The callback to be called upon receipt of a response */
+ /*! The endpoint associated with this request */
+ struct ast_sip_endpoint *endpoint;
+ /*! Information to be provided to the callback upon receipt of a response */
+ void *token;
+ /*! The callback to be called upon receipt of a response */
+ void (*callback)(void *token, pjsip_event *e);
+ /*! Number of challenges received. */
+ unsigned int challenge_count;
};
static void send_request_data_destroy(void *obj)
{
struct send_request_data *req_data = obj;
+
ao2_cleanup(req_data->endpoint);
}
static struct send_request_data *send_request_data_alloc(struct ast_sip_endpoint *endpoint,
void *token, void (*callback)(void *token, pjsip_event *e))
{
- struct send_request_data *req_data = ao2_alloc(sizeof(*req_data), send_request_data_destroy);
-
+ struct send_request_data *req_data;
+
+ req_data = ao2_alloc_options(sizeof(*req_data), send_request_data_destroy,
+ AO2_ALLOC_OPT_LOCK_NOLOCK);
if (!req_data) {
return NULL;
}
@@ -2460,50 +2471,152 @@
return req_data;
}
+struct send_request_wrapper {
+ /*! Information to be provided to the callback upon receipt of a response */
+ void *token;
+ /*! The callback to be called upon receipt of a response */
+ void (*callback)(void *token, pjsip_event *e);
+ /*! Non-zero when the callback is called. */
+ unsigned int cb_called;
+};
+
+static void endpt_send_request_wrapper(void *token, pjsip_event *e)
+{
+ struct send_request_wrapper *req_wrapper = token;
+
+ req_wrapper->cb_called = 1;
+ if (req_wrapper->callback) {
+ req_wrapper->callback(req_wrapper->token, e);
+ }
+ ao2_ref(req_wrapper, -1);
+}
+
+static pj_status_t endpt_send_request(struct ast_sip_endpoint *endpoint,
+ pjsip_tx_data *tdata, pj_int32_t timeout, void *token, pjsip_endpt_send_callback cb)
+{
+ struct send_request_wrapper *req_wrapper;
+ pj_status_t ret_val;
+
+ /* Create wrapper to detect if the callback was actually called on an error. */
+ req_wrapper = ao2_alloc_options(sizeof(*req_wrapper), NULL,
+ AO2_ALLOC_OPT_LOCK_NOLOCK);
+ if (!req_wrapper) {
+ pjsip_tx_data_dec_ref(tdata);
+ return PJ_ENOMEM;
+ }
+ req_wrapper->token = token;
+ req_wrapper->callback = cb;
+
+ ao2_ref(req_wrapper, +1);
+ ret_val = pjsip_endpt_send_request(ast_sip_get_pjsip_endpoint(), tdata, timeout,
+ req_wrapper, endpt_send_request_wrapper);
+ if (ret_val != PJ_SUCCESS) {
+ char errmsg[PJ_ERR_MSG_SIZE];
+
+ /* Complain of failure to send the request. */
+ pj_strerror(ret_val, errmsg, sizeof(errmsg));
+ ast_log(LOG_ERROR, "Error %d '%s' sending %.*s request to endpoint %s\n",
+ (int) ret_val, errmsg, (int) pj_strlen(&tdata->msg->line.req.method.name),
+ pj_strbuf(&tdata->msg->line.req.method.name),
+ endpoint ? ast_sorcery_object_get_id(endpoint) : "<unknown>");
+
+ /* Was the callback called? */
+ if (req_wrapper->cb_called) {
+ /*
+ * Yes so we cannot report any error. The callback
+ * has already freed any resources associated with
+ * token.
+ */
+ ret_val = PJ_SUCCESS;
+ } else {
+ /* No and it is not expected to ever be called. */
+ ao2_ref(req_wrapper, -1);
+ }
+ }
+ ao2_ref(req_wrapper, -1);
+ return ret_val;
+}
+
static void send_request_cb(void *token, pjsip_event *e)
{
- RAII_VAR(struct send_request_data *, req_data, token, ao2_cleanup);
- pjsip_transaction *tsx = e->body.tsx_state.tsx;
- pjsip_rx_data *challenge = e->body.tsx_state.src.rdata;
+ struct send_request_data *req_data = token;
+ pjsip_transaction *tsx;
+ pjsip_rx_data *challenge;
pjsip_tx_data *tdata;
struct ast_sip_supplement *supplement;
-
- AST_RWLIST_RDLOCK(&supplements);
- AST_LIST_TRAVERSE(&supplements, supplement, next) {
- if (supplement->incoming_response && does_method_match(&challenge->msg_info.cseq->method.name, supplement->method)) {
- supplement->incoming_response(req_data->endpoint, challenge);
+ struct ast_sip_endpoint *endpoint;
+ int res;
+
+ switch(e->body.tsx_state.type) {
+ case PJSIP_EVENT_TRANSPORT_ERROR:
+ case PJSIP_EVENT_TIMER:
+ break;
+ case PJSIP_EVENT_RX_MSG:
+ challenge = e->body.tsx_state.src.rdata;
+
+ /*
+ * Call any supplements that want to know about a response
+ * with any received data.
+ */
+ AST_RWLIST_RDLOCK(&supplements);
+ AST_LIST_TRAVERSE(&supplements, supplement, next) {
+ if (supplement->incoming_response
+ && does_method_match(&challenge->msg_info.cseq->method.name,
+ supplement->method)) {
+ supplement->incoming_response(req_data->endpoint, challenge);
+ }
}
- }
- AST_RWLIST_UNLOCK(&supplements);
-
- if ((tsx->status_code == 401 || tsx->status_code == 407)
- && req_data->endpoint
- && !ast_sip_create_request_with_auth(&req_data->endpoint->outbound_auths, challenge, tsx, &tdata)
- && pjsip_endpt_send_request(ast_sip_get_pjsip_endpoint(), tdata, -1, req_data->token, req_data->callback)
- == PJ_SUCCESS) {
- return;
+ AST_RWLIST_UNLOCK(&supplements);
+
+ /* Resend the request with a challenge response if we are challenged. */
+ tsx = e->body.tsx_state.tsx;
+ endpoint = ao2_bump(req_data->endpoint);
+ res = (tsx->status_code == 401 || tsx->status_code == 407)
+ && endpoint
+ && ++req_data->challenge_count < MAX_RX_CHALLENGES /* Not in a challenge loop */
+ && !ast_sip_create_request_with_auth(&endpoint->outbound_auths,
+ challenge, tsx, &tdata)
+ && endpt_send_request(endpoint, tdata, -1, req_data, send_request_cb)
+ == PJ_SUCCESS;
+ ao2_cleanup(endpoint);
+ if (res) {
+ /*
+ * Request with challenge response sent.
+ * Passed our req_data ref to the new request.
+ */
+ return;
+ }
+ break;
+ default:
+ ast_log(LOG_ERROR, "Unexpected PJSIP event %d\n", e->body.tsx_state.type);
+ break;
}
if (req_data->callback) {
req_data->callback(req_data->token, e);
}
+ ao2_ref(req_data, -1);
}
static int send_out_of_dialog_request(pjsip_tx_data *tdata, struct ast_sip_endpoint *endpoint,
void *token, void (*callback)(void *token, pjsip_event *e))
{
struct ast_sip_supplement *supplement;
- struct send_request_data *req_data = send_request_data_alloc(endpoint, token, callback);
- struct ast_sip_contact *contact = ast_sip_mod_data_get(tdata->mod_data, supplement_module.id, MOD_DATA_CONTACT);
-
+ struct send_request_data *req_data;
+ struct ast_sip_contact *contact;
+
+ req_data = send_request_data_alloc(endpoint, token, callback);
if (!req_data) {
pjsip_tx_data_dec_ref(tdata);
return -1;
}
+ contact = ast_sip_mod_data_get(tdata->mod_data, supplement_module.id, MOD_DATA_CONTACT);
+
AST_RWLIST_RDLOCK(&supplements);
AST_LIST_TRAVERSE(&supplements, supplement, next) {
- if (supplement->outgoing_request && does_method_match(&tdata->msg->line.req.method.name, supplement->method)) {
+ if (supplement->outgoing_request
+ && does_method_match(&tdata->msg->line.req.method.name, supplement->method)) {
supplement->outgoing_request(endpoint, contact, tdata);
}
}
@@ -2512,11 +2625,8 @@
ast_sip_mod_data_set(tdata->pool, tdata->mod_data, supplement_module.id, MOD_DATA_CONTACT, NULL);
ao2_cleanup(contact);
- if (pjsip_endpt_send_request(ast_sip_get_pjsip_endpoint(), tdata, -1, req_data, send_request_cb) != PJ_SUCCESS) {
- ast_log(LOG_ERROR, "Error attempting to send outbound %.*s request to endpoint %s\n",
- (int) pj_strlen(&tdata->msg->line.req.method.name),
- pj_strbuf(&tdata->msg->line.req.method.name),
- endpoint ? ast_sorcery_object_get_id(endpoint) : "<unknown>");
+ if (endpt_send_request(endpoint, tdata, -1, req_data, send_request_cb)
+ != PJ_SUCCESS) {
ao2_cleanup(req_data);
return -1;
}
Modified: trunk/res/res_pjsip/pjsip_options.c
URL: http://svnview.digium.com/svn/asterisk/trunk/res/res_pjsip/pjsip_options.c?view=diff&rev=423868&r1=423867&r2=423868
==============================================================================
--- trunk/res/res_pjsip/pjsip_options.c (original)
+++ trunk/res/res_pjsip/pjsip_options.c Wed Sep 24 13:35:47 2014
@@ -113,7 +113,7 @@
/* if the contact is available calculate the rtt as
the diff between the last start time and "now" */
- update->rtt = update->status ?
+ update->rtt = update->status == AVAILABLE ?
ast_tvdiff_us(ast_tvnow(), status->rtt_start) : 0;
update->rtt_start = ast_tv(0, 0);
@@ -240,18 +240,21 @@
/*!
* \internal
- * \brief Receive an response to the qualify contact request.
+ * \brief Receive a response to the qualify contact request.
*/
static void qualify_contact_cb(void *token, pjsip_event *e)
{
struct ast_sip_contact *contact = token;
switch(e->body.tsx_state.type) {
+ default:
+ ast_log(LOG_ERROR, "Unexpected PJSIP event %d\n", e->body.tsx_state.type);
+ /* Fall through */
case PJSIP_EVENT_TRANSPORT_ERROR:
case PJSIP_EVENT_TIMER:
update_contact_status(contact, UNAVAILABLE);
break;
- default:
+ case PJSIP_EVENT_RX_MSG:
update_contact_status(contact, AVAILABLE);
break;
}
@@ -308,6 +311,7 @@
!= PJ_SUCCESS) {
ast_log(LOG_ERROR, "Unable to send request to qualify contact %s\n",
contact->uri);
+ update_contact_status(contact, UNAVAILABLE);
ao2_ref(contact, -1);
return -1;
}
@@ -354,10 +358,12 @@
*/
static struct sched_data *sched_data_create(struct ast_sip_contact *contact)
{
- struct sched_data *data = ao2_alloc(sizeof(*data), sched_data_destructor);
-
+ struct sched_data *data;
+
+ data = ao2_t_alloc(sizeof(*data), sched_data_destructor, contact->uri);
if (!data) {
- ast_log(LOG_ERROR, "Unable to create schedule qualify data\n");
+ ast_log(LOG_ERROR, "Unable to create schedule qualify data for contact %s\n",
+ contact->uri);
return NULL;
}
@@ -392,11 +398,14 @@
ao2_ref(data->contact, +1);
if (ast_sip_push_task(NULL, qualify_contact_task, data->contact)) {
ao2_ref(data->contact, -1);
- ao2_cleanup(data);
- return 0;
- }
-
- return data->contact->qualify_frequency * 1000;
+ }
+
+ /*
+ * Always reschedule rather than have a potential race cleaning
+ * up the data object ref between self deletion and an external
+ * deletion.
+ */
+ return 1;
}
/*!
@@ -405,24 +414,27 @@
*/
static void schedule_qualify(struct ast_sip_contact *contact)
{
- RAII_VAR(struct sched_data *, data, sched_data_create(contact), ao2_cleanup);
-
+ struct sched_data *data;
+
+ data = sched_data_create(contact);
if (!data) {
return;
}
- ao2_ref(data, +1);
- if ((data->id = ast_sched_add_variable(
- sched, contact->qualify_frequency * 1000,
- qualify_contact_sched, data, 1)) < 0) {
-
- ao2_ref(data, -1);
+ ast_assert(contact->qualify_frequency != 0);
+
+ ao2_t_ref(data, +1, "Ref for qualify_contact_sched() scheduler entry");
+ data->id = ast_sched_add_variable(sched, contact->qualify_frequency * 1000,
+ qualify_contact_sched, data, 0);
+ if (data->id < 0) {
+ ao2_t_ref(data, -1, "Cleanup failed scheduler add");
ast_log(LOG_ERROR, "Unable to schedule qualify for contact %s\n",
contact->uri);
- return;
- }
-
- ao2_link(sched_qualifies, data);
+ } else if (!ao2_link(sched_qualifies, data)) {
+ AST_SCHED_DEL_UNREF(sched, data->id,
+ ao2_t_ref(data, -1, "Cleanup scheduler for failed ao2_link"));
+ }
+ ao2_t_ref(data, -1, "Done setting up scheduler entry");
}
/*!
@@ -438,8 +450,9 @@
return;
}
- AST_SCHED_DEL_UNREF(sched, data->id, ao2_cleanup(data));
- ao2_ref(data, -1);
+ AST_SCHED_DEL_UNREF(sched, data->id,
+ ao2_t_ref(data, -1, "Delete scheduler entry ref"));
+ ao2_t_ref(data, -1, "Done with ao2_find ref");
}
/*!
@@ -452,7 +465,9 @@
if (contact->qualify_frequency) {
ao2_ref(contact, +1);
- ast_sip_push_task(NULL, qualify_contact_task, contact);
+ if (ast_sip_push_task(NULL, qualify_contact_task, contact)) {
+ ao2_ref(contact, -1);
+ }
schedule_qualify(contact);
}
@@ -498,23 +513,44 @@
static pj_bool_t options_start(void)
{
- if (!(sched = ast_sched_context_create()) ||
- ast_sched_start_thread(sched)) {
+ sched = ast_sched_context_create();
+ if (!sched) {
return -1;
}
+ if (ast_sched_start_thread(sched)) {
+ ast_sched_context_destroy(sched);
+ sched = NULL;
+ return -1;
+ }
+
+ if (ast_sorcery_observer_add(ast_sip_get_sorcery(), "contact", &contact_observer)) {
+ ast_log(LOG_WARNING, "Unable to add contact observer\n");
+ ast_sched_context_destroy(sched);
+ sched = NULL;
+ return -1;
+ }
return PJ_SUCCESS;
}
+static int sched_qualifies_empty(void *obj, void *arg, int flags)
+{
+ ao2_t_ref(obj, -1, "Release ref held by destroyed scheduler context.");
+ return CMP_MATCH;
+}
+
static pj_bool_t options_stop(void)
{
ast_sorcery_observer_remove(ast_sip_get_sorcery(), "contact", &contact_observer);
-
- ao2_t_ref(sched_qualifies, -1, "Remove scheduled qualifies on module stop");
if (sched) {
ast_sched_context_destroy(sched);
- }
+ sched = NULL;
+ }
+
+ /* Empty the container of scheduling data refs. */
+ ao2_callback(sched_qualifies, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE,
+ sched_qualifies_empty, NULL);
return PJ_SUCCESS;
}
@@ -1022,39 +1058,39 @@
int ast_res_pjsip_init_options_handling(int reload)
{
- const pj_str_t STR_OPTIONS = { "OPTIONS", 7 };
+ static const pj_str_t STR_OPTIONS = { "OPTIONS", 7 };
if (reload) {
qualify_and_schedule_all();
return 0;
}
- if (!(sched_qualifies = ao2_t_container_alloc(
- QUALIFIED_BUCKETS, sched_qualifies_hash_fn, sched_qualifies_cmp_fn,
- "Create container for scheduled qualifies"))) {
+ sched_qualifies = ao2_t_container_alloc(QUALIFIED_BUCKETS,
+ sched_qualifies_hash_fn, sched_qualifies_cmp_fn,
+ "Create container for scheduled qualifies");
+ if (!sched_qualifies) {
return -1;
}
if (pjsip_endpt_register_module(ast_sip_get_pjsip_endpoint(), &options_module) != PJ_SUCCESS) {
- options_stop();
+ ao2_cleanup(sched_qualifies);
+ sched_qualifies = NULL;
return -1;
}
- if (pjsip_endpt_add_capability(ast_sip_get_pjsip_endpoint(), NULL, PJSIP_H_ALLOW, NULL, 1, &STR_OPTIONS) != PJ_SUCCESS) {
+ if (pjsip_endpt_add_capability(ast_sip_get_pjsip_endpoint(), NULL, PJSIP_H_ALLOW,
+ NULL, 1, &STR_OPTIONS) != PJ_SUCCESS) {
pjsip_endpt_unregister_module(ast_sip_get_pjsip_endpoint(), &options_module);
+ ao2_cleanup(sched_qualifies);
+ sched_qualifies = NULL;
return -1;
}
- if (ast_sorcery_observer_add(ast_sip_get_sorcery(), "contact", &contact_observer)) {
- ast_log(LOG_WARNING, "Unable to add contact observer\n");
- return -1;
- }
-
ast_sip_register_endpoint_formatter(&contact_status_formatter);
+ ast_manager_register2("PJSIPQualify", EVENT_FLAG_SYSTEM | EVENT_FLAG_REPORTING, ami_sip_qualify, NULL, NULL, NULL);
+ ast_cli_register_multiple(cli_options, ARRAY_LEN(cli_options));
qualify_and_schedule_all();
- ast_cli_register_multiple(cli_options, ARRAY_LEN(cli_options));
- ast_manager_register2("PJSIPQualify", EVENT_FLAG_SYSTEM | EVENT_FLAG_REPORTING, ami_sip_qualify, NULL, NULL, NULL);
return 0;
}
@@ -1064,4 +1100,8 @@
ast_cli_unregister_multiple(cli_options, ARRAY_LEN(cli_options));
ast_manager_unregister("PJSIPQualify");
ast_sip_unregister_endpoint_formatter(&contact_status_formatter);
-}
+
+ pjsip_endpt_unregister_module(ast_sip_get_pjsip_endpoint(), &options_module);
+ ao2_cleanup(sched_qualifies);
+ sched_qualifies = NULL;
+}
More information about the asterisk-commits
mailing list