From: George Joseph Date: Fri, 1 Apr 2016 18:30:56 +0000 (-0600) Subject: res_pjsip contact: Lock expiration/addition of contacts X-Git-Tag: certified/13.8-cert1-rc3~20 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=51e45e5ca53b611664e801b84f0f0b65afe5d69c;p=thirdparty%2Fasterisk.git res_pjsip contact: Lock expiration/addition of contacts Contact expiration can occur in several places: res_pjsip_registrar, res_pjsip_registrar_expire, and automatically when anyone calls ast_sip_location_retrieve_aor_contact. At the same time, res_pjsip_registrar may also be attempting to renew or add a contact. Since none of this was locked it was possible for one thread to be renewing a contact and another thread to expire it immediately because it was working off of stale data. This was the casue of intermittent registration/inbound/nominal/multiple_contacts test failures. Now, the new named lock functionality is used to lock the aor during contact expire and add operations and res_pjsip_registrar_expire now checks the expiration with the lock held before deleting the contact. ASTERISK-25885 #close Reported-by: Josh Colp Change-Id: I83d413c46a47796f3ab052ca3b349f21cca47059 --- diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h index f56eb7bb48..acdc9724a8 100644 --- a/include/asterisk/res_pjsip.h +++ b/include/asterisk/res_pjsip.h @@ -998,9 +998,27 @@ struct ast_sip_contact *ast_sip_location_retrieve_first_aor_contact(const struct * * \retval NULL if no contacts available * \retval non-NULL if contacts available + * + * \warning + * Since this function prunes expired contacts before returning, it holds a named write + * lock on the aor. If you already hold the lock, call ast_sip_location_retrieve_aor_contacts_nolock instead. */ struct ao2_container *ast_sip_location_retrieve_aor_contacts(const struct ast_sip_aor *aor); +/*! + * \brief Retrieve all contacts currently available for an AOR without locking the AOR + * \since 13.9.0 + * + * \param aor Pointer to the AOR + * + * \retval NULL if no contacts available + * \retval non-NULL if contacts available + * + * \warning + * This function should only be called if you already hold a named write lock on the aor. + */ +struct ao2_container *ast_sip_location_retrieve_aor_contacts_nolock(const struct ast_sip_aor *aor); + /*! * \brief Retrieve the first bound contact from a list of AORs * @@ -1051,11 +1069,36 @@ struct ast_sip_contact *ast_sip_location_retrieve_contact(const char *contact_na * * \retval -1 failure * \retval 0 success + * + * \warning + * This function holds a named write lock on the aor. If you already hold the lock + * you should call ast_sip_location_add_contact_nolock instead. */ int ast_sip_location_add_contact(struct ast_sip_aor *aor, const char *uri, struct timeval expiration_time, const char *path_info, const char *user_agent, struct ast_sip_endpoint *endpoint); +/*! + * \brief Add a new contact to an AOR without locking the AOR + * \since 13.9.0 + * + * \param aor Pointer to the AOR + * \param uri Full contact URI + * \param expiration_time Optional expiration time of the contact + * \param path_info Path information + * \param user_agent User-Agent header from REGISTER request + * \param endpoint The endpoint that resulted in the contact being added + * + * \retval -1 failure + * \retval 0 success + * + * \warning + * This function should only be called if you already hold a named write lock on the aor. + */ +int ast_sip_location_add_contact_nolock(struct ast_sip_aor *aor, const char *uri, + struct timeval expiration_time, const char *path_info, const char *user_agent, + struct ast_sip_endpoint *endpoint); + /*! * \brief Update a contact * diff --git a/res/res_pjsip/location.c b/res/res_pjsip/location.c index 7ff72e4e9c..636de8bbb2 100644 --- a/res/res_pjsip/location.c +++ b/res/res_pjsip/location.c @@ -27,6 +27,7 @@ #include "include/res_pjsip_private.h" #include "asterisk/res_pjsip_cli.h" #include "asterisk/statsd.h" +#include "asterisk/named_locks.h" #include "asterisk/res_pjproject.h" @@ -177,7 +178,7 @@ struct ast_sip_contact *ast_sip_location_retrieve_first_aor_contact(const struct return contact; } -struct ao2_container *ast_sip_location_retrieve_aor_contacts(const struct ast_sip_aor *aor) +struct ao2_container *ast_sip_location_retrieve_aor_contacts_nolock(const struct ast_sip_aor *aor) { /* Give enough space for ^ at the beginning and ;@ at the end, since that is our object naming scheme */ char regex[strlen(ast_sorcery_object_get_id(aor)) + 4]; @@ -200,6 +201,24 @@ struct ao2_container *ast_sip_location_retrieve_aor_contacts(const struct ast_si return contacts; } +struct ao2_container *ast_sip_location_retrieve_aor_contacts(const struct ast_sip_aor *aor) +{ + struct ao2_container *contacts; + struct ast_named_lock *lock; + + lock = ast_named_lock_get(AST_NAMED_LOCK_TYPE_RWLOCK, "aor", ast_sorcery_object_get_id(aor)); + if (!lock) { + return NULL; + } + + ao2_wrlock(lock); + contacts = ast_sip_location_retrieve_aor_contacts_nolock(aor); + ao2_unlock(lock); + ast_named_lock_put(lock); + + return contacts; +} + void ast_sip_location_retrieve_contact_and_aor_from_list(const char *aor_list, struct ast_sip_aor **aor, struct ast_sip_contact **contact) { @@ -283,7 +302,7 @@ struct ast_sip_contact *ast_sip_location_retrieve_contact(const char *contact_na return ast_sorcery_retrieve_by_id(ast_sip_get_sorcery(), "contact", contact_name); } -int ast_sip_location_add_contact(struct ast_sip_aor *aor, const char *uri, +int ast_sip_location_add_contact_nolock(struct ast_sip_aor *aor, const char *uri, struct timeval expiration_time, const char *path_info, const char *user_agent, struct ast_sip_endpoint *endpoint) { @@ -320,6 +339,27 @@ int ast_sip_location_add_contact(struct ast_sip_aor *aor, const char *uri, return ast_sorcery_create(ast_sip_get_sorcery(), contact); } +int ast_sip_location_add_contact(struct ast_sip_aor *aor, const char *uri, + struct timeval expiration_time, const char *path_info, const char *user_agent, + struct ast_sip_endpoint *endpoint) +{ + int res; + struct ast_named_lock *lock; + + lock = ast_named_lock_get(AST_NAMED_LOCK_TYPE_RWLOCK, "aor", ast_sorcery_object_get_id(aor)); + if (!lock) { + return -1; + } + + ao2_wrlock(lock); + res = ast_sip_location_add_contact_nolock(aor, uri, expiration_time, path_info, user_agent, + endpoint); + ao2_unlock(lock); + ast_named_lock_put(lock); + + return res; +} + int ast_sip_location_update_contact(struct ast_sip_contact *contact) { return ast_sorcery_update(ast_sip_get_sorcery(), contact); diff --git a/res/res_pjsip_registrar.c b/res/res_pjsip_registrar.c index 1ee0d5f8c3..851a5f77d6 100644 --- a/res/res_pjsip_registrar.c +++ b/res/res_pjsip_registrar.c @@ -33,6 +33,7 @@ #include "asterisk/test.h" #include "asterisk/taskprocessor.h" #include "asterisk/manager.h" +#include "asterisk/named_locks.h" #include "asterisk/res_pjproject.h" #include "res_pjsip/include/res_pjsip_private.h" @@ -431,27 +432,21 @@ static int registrar_validate_path(struct rx_task_data *task_data, struct ast_st return -1; } -static int rx_task(void *data) +static int rx_task_core(struct rx_task_data *task_data, struct ao2_container *contacts, + const char *aor_name) { static const pj_str_t USER_AGENT = { "User-Agent", 10 }; - RAII_VAR(struct rx_task_data *, task_data, data, ao2_cleanup); - RAII_VAR(struct ao2_container *, contacts, NULL, ao2_cleanup); - int added = 0, updated = 0, deleted = 0; pjsip_contact_hdr *contact_hdr = NULL; struct registrar_contact_details details = { 0, }; pjsip_tx_data *tdata; - const char *aor_name = ast_sorcery_object_get_id(task_data->aor); RAII_VAR(struct ast_str *, path_str, NULL, ast_free); struct ast_sip_contact *response_contact; char *user_agent = NULL; pjsip_user_agent_hdr *user_agent_hdr; pjsip_expires_hdr *expires_hdr; - /* Retrieve the current contacts, we'll need to know whether to update or not */ - contacts = ast_sip_location_retrieve_aor_contacts(task_data->aor); - /* So we don't count static contacts against max_contacts we prune them out from the container */ ao2_callback(contacts, OBJ_NODATA | OBJ_UNLINK | OBJ_MULTIPLE, registrar_prune_static, NULL); @@ -522,7 +517,7 @@ static int rx_task(void *data) continue; } - if (ast_sip_location_add_contact(task_data->aor, contact_uri, ast_tvadd(ast_tvnow(), + if (ast_sip_location_add_contact_nolock(task_data->aor, contact_uri, ast_tvadd(ast_tvnow(), ast_samp2tv(expiration, 1)), path_str ? ast_str_buffer(path_str) : NULL, user_agent, task_data->endpoint)) { ast_log(LOG_ERROR, "Unable to bind contact '%s' to AOR '%s'\n", @@ -564,7 +559,7 @@ static int rx_task(void *data) if (ast_sip_location_update_contact(contact_update)) { ast_log(LOG_ERROR, "Failed to update contact '%s' expiration time to %d seconds.\n", contact->uri, expiration); - ast_sorcery_delete(ast_sip_get_sorcery(), contact); + ast_sip_location_delete_contact(contact); continue; } ast_debug(3, "Refreshed contact '%s' on AOR '%s' with new expiration of %d seconds\n", @@ -602,15 +597,14 @@ static int rx_task(void *data) ao2_callback(contacts, OBJ_NODATA | OBJ_MULTIPLE, registrar_delete_contact, NULL); } - /* Update the contacts as things will probably have changed */ - ao2_cleanup(contacts); - - contacts = ast_sip_location_retrieve_aor_contacts(task_data->aor); + /* Re-retrieve contacts. Caller will clean up the original container. */ + contacts = ast_sip_location_retrieve_aor_contacts_nolock(task_data->aor); response_contact = ao2_callback(contacts, 0, NULL, NULL); /* Send a response containing all of the contacts (including static) that are present on this AOR */ if (ast_sip_create_response(task_data->rdata, 200, response_contact, &tdata) != PJ_SUCCESS) { ao2_cleanup(response_contact); + ao2_cleanup(contacts); return PJ_TRUE; } ao2_cleanup(response_contact); @@ -619,6 +613,7 @@ static int rx_task(void *data) registrar_add_date_header(tdata); ao2_callback(contacts, 0, registrar_add_contact, tdata); + ao2_cleanup(contacts); if ((expires_hdr = pjsip_msg_find_hdr(task_data->rdata->msg_info.msg, PJSIP_H_EXPIRES, NULL))) { expires_hdr = pjsip_expires_hdr_create(tdata->pool, registrar_get_expiration(task_data->aor, NULL, task_data->rdata)); @@ -630,6 +625,38 @@ static int rx_task(void *data) return PJ_TRUE; } +static int rx_task(void *data) +{ + int res; + struct rx_task_data *task_data = data; + struct ao2_container *contacts = NULL; + struct ast_named_lock *lock; + const char *aor_name = ast_sorcery_object_get_id(task_data->aor); + + lock = ast_named_lock_get(AST_NAMED_LOCK_TYPE_RWLOCK, "aor", aor_name); + if (!lock) { + ao2_cleanup(task_data); + return PJ_TRUE; + } + + ao2_wrlock(lock); + contacts = ast_sip_location_retrieve_aor_contacts_nolock(task_data->aor); + if (!contacts) { + ao2_unlock(lock); + ast_named_lock_put(lock); + ao2_cleanup(task_data); + return PJ_TRUE; + } + + res = rx_task_core(task_data, contacts, aor_name); + ao2_cleanup(contacts); + ao2_unlock(lock); + ast_named_lock_put(lock); + ao2_cleanup(task_data); + + return res; +} + static pj_bool_t registrar_on_rx_request(struct pjsip_rx_data *rdata) { RAII_VAR(struct serializer *, ser, NULL, ao2_cleanup); diff --git a/res/res_pjsip_registrar_expire.c b/res/res_pjsip_registrar_expire.c index 5b2b350eab..cde60ed6db 100644 --- a/res/res_pjsip_registrar_expire.c +++ b/res/res_pjsip_registrar_expire.c @@ -29,6 +29,7 @@ #include "asterisk/res_pjsip.h" #include "asterisk/module.h" #include "asterisk/sched.h" +#include "asterisk/named_locks.h" #define CONTACT_AUTOEXPIRE_BUCKETS 977 @@ -121,13 +122,60 @@ static int contact_expiration_cmp(void *obj, void *arg, int flags) static int contact_expiration_expire(const void *data) { struct contact_expiration *expiration = (void *) data; + struct ast_sip_contact *fresh_contact; + struct ast_named_lock *lock; + int reschedule_time = 0; + + lock = ast_named_lock_get(AST_NAMED_LOCK_TYPE_RWLOCK, "aor", expiration->contact->aor); + if (!lock) { + /* Uh Oh. Just expire the contact and don't be nice about it. */ + expiration->sched = -1; + ast_sip_location_delete_contact(expiration->contact); + ao2_ref(expiration, -1); + return 0; + } - expiration->sched = -1; + /* + * We need to check the expiration again with the aor lock held + * in case another thread is attempting to renew the contact. + */ + ao2_wrlock(lock); + + fresh_contact = ast_sip_location_retrieve_contact( + ast_sorcery_object_get_id(expiration->contact)); + if (fresh_contact) { + int expires; + + expires = ast_tvdiff_ms(fresh_contact->expiration_time, ast_tvnow()); + if (0 < expires) { + /* We need to reschedule for the new expiration time. */ + reschedule_time = expires; + } else { + /* + * Contact is expired. + * + * This will end up invoking the deleted observer callback, + * which will perform the unlinking and such. + */ + expiration->sched = -1; + ast_sip_location_delete_contact(fresh_contact); + ao2_ref(expiration, -1); + } + ao2_ref(fresh_contact, -1); + } else { + /* + * The object no longer exists in sorcery since we + * could not get a fresh copy. + */ + expiration->sched = -1; + ast_sip_location_delete_contact(expiration->contact); + ao2_ref(expiration, -1); + } - /* This will end up invoking the deleted observer callback, which will perform the unlinking and such */ - ast_sorcery_delete(ast_sip_get_sorcery(), expiration->contact); - ao2_ref(expiration, -1); - return 0; + ao2_unlock(lock); + ast_named_lock_put(lock); + + return reschedule_time; } /*! \brief Observer callback for when a contact is created */ @@ -151,7 +199,9 @@ static void contact_expiration_observer_created(const void *object) ao2_ref(expiration->contact, +1); ao2_ref(expiration, +1); - if ((expiration->sched = ast_sched_add(sched, expires, contact_expiration_expire, expiration)) < 0) { + expiration->sched = ast_sched_add_variable(sched, expires, contact_expiration_expire, + expiration, 1); + if (expiration->sched < 0) { ao2_ref(expiration, -1); ast_log(LOG_ERROR, "Scheduled expiration for contact '%s' could not be performed, contact may persist past life\n", ast_sorcery_object_get_id(contact)); @@ -174,8 +224,9 @@ static void contact_expiration_observer_updated(const void *object) return; } - AST_SCHED_REPLACE_UNREF(expiration->sched, sched, expires, contact_expiration_expire, - expiration, ao2_cleanup(expiration), ao2_cleanup(expiration), ao2_ref(expiration, +1)); + AST_SCHED_REPLACE_VARIABLE_UNREF(expiration->sched, sched, expires, + contact_expiration_expire, expiration, 1, + ao2_cleanup(expiration), ao2_cleanup(expiration), ao2_ref(expiration, +1)); ao2_ref(expiration, -1); }