]> git.ipfire.org Git - thirdparty/asterisk.git/commitdiff
res_pjsip contact: Lock expiration/addition of contacts 25/2525/5
authorGeorge Joseph <george.joseph@fairview5.com>
Fri, 1 Apr 2016 18:30:56 +0000 (12:30 -0600)
committerJoshua Colp <jcolp@digium.com>
Mon, 11 Apr 2016 18:00:14 +0000 (13:00 -0500)
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

include/asterisk/res_pjsip.h
res/res_pjsip/location.c
res/res_pjsip_registrar.c
res/res_pjsip_registrar_expire.c

index fbb9bdcbe54f5c717da97d4e5d4b1c11b438043a..3a9d61e4cd04c2109cc13a93baed1981efedf9cf 100644 (file)
@@ -1004,9 +1004,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
  *
@@ -1057,11 +1075,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
  *
index 735b18f46754047739ff0af43542a28aba174446..3413708e9364fd44f28e35a9280b2aa1f38835b3 100644 (file)
@@ -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"
 
 /*! \brief Destructor for AOR */
 static void aor_destroy(void *obj)
@@ -173,7 +174,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];
@@ -196,6 +197,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)
 {
@@ -279,7 +298,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)
 {
@@ -316,6 +335,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);
index fb2b9dabac71aacd100a15308916604ebf5594b9..4a42102dc2f6819b16703c30395ce3e6b776dfc3 100644 (file)
@@ -32,6 +32,7 @@
 #include "asterisk/test.h"
 #include "asterisk/taskprocessor.h"
 #include "asterisk/manager.h"
+#include "asterisk/named_locks.h"
 #include "res_pjsip/include/res_pjsip_private.h"
 
 /*** DOCUMENTATION
@@ -412,27 +413,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);
 
@@ -503,7 +498,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",
@@ -545,7 +540,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",
@@ -584,15 +579,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);
@@ -601,6 +595,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));
@@ -612,6 +607,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);
index 97f2ca1c01d9c4b7587c5e7b3e62229af03f84bc..0fddbda9103cee71fc7d45cfee414f7f907ca539 100644 (file)
@@ -30,6 +30,7 @@
 
 #include "asterisk/res_pjsip.h"
 #include "asterisk/module.h"
+#include "asterisk/named_locks.h"
 
 /*! \brief Thread keeping things alive */
 static pthread_t check_thread = AST_PTHREADT_NULL;
@@ -41,8 +42,23 @@ static unsigned int check_interval;
 static int expire_contact(void *obj, void *arg, int flags)
 {
        struct ast_sip_contact *contact = obj;
+       struct ast_named_lock *lock;
 
-       ast_sorcery_delete(ast_sip_get_sorcery(), contact);
+       lock = ast_named_lock_get(AST_NAMED_LOCK_TYPE_RWLOCK, "aor", contact->aor);
+       if (!lock) {
+               return 0;
+       }
+
+       /*
+        * 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);
+       if (ast_tvdiff_ms(ast_tvnow(), contact->expiration_time) > 0) {
+               ast_sip_location_delete_contact(contact);
+       }
+       ao2_unlock(lock);
+       ast_named_lock_put(lock);
 
        return 0;
 }