res_pjsip: make it unloadable (take 2)

Due to the original patch causing memory corruptions it was removed until the
problem could be resolved. This patch is the original patch plus some added
locking around stasis router subcription that was needed to avoid the memory
corruption.

Description of the original problem and patch (still applicable):

The res_pjsip module was previously unloadable. With this patch it can now
be unloaded.

This patch is based off the original patch on the issue (listed below) by Corey
Farrell with a few modifications. Namely, removed a few changes not required to
make the module unloadable and also fixed a bug that would cause asterisk to
crash on unloading.

This patch is the first step (should hopefully be followed by another/others at
some point) in allowing res_pjsip and the modules that depend on it to be
unloadable. At this time, res_pjsip and some of the modules that depend on
res_pjsip cannot be unloaded without causing problems of some sort.

The goal of this patch is to get res_pjsip and only res_pjsip to be able to
unload successfully and/or shutdown without incident (crashes, leaks, etc...).
Other dependent modules may still cause problems on unload.

Basically made sure, with the patch applied, that res_pjsip (with no other
dependent modules loaded) could be succesfully unloaded and Asterisk could
shutdown without any leaks or crashes that pertained directly to res_pjsip.

ASTERISK-24485 #close
Reported by: Corey Farrell
Review: https://reviewboard.asterisk.org/r/4363/
patches:
  pjsip_unload-broken-r1.patch submitted by Corey Farrell (license 5909)
........

Merged revisions 431179 from http://svn.asterisk.org/svn/asterisk/branches/13

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@431180 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/main/stasis_message_router.c b/main/stasis_message_router.c
index a9e458456fcd3c27119bc0ccda677d9877534e02..26df76c53fc3935f7c4af957522ae0ba349043c3 100644 (file)
--- a/main/stasis_message_router.c
+++ b/main/stasis_message_router.c
@@ -255,7 +255,9 @@ void stasis_message_router_unsubscribe(struct stasis_message_router *router)
                return;
        }
 
-       stasis_unsubscribe(router->subscription);
+       ao2_lock(router);
+       router->subscription = stasis_unsubscribe(router->subscription);
+       ao2_unlock(router);
 }
 
 void stasis_message_router_unsubscribe_and_join(

diff --git a/res/res_pjsip.c b/res/res_pjsip.c
index 5a809097e5d4d473f27e9f9ab79d58e8c3d9d237..b2b7d63584a224f3f812e11ef01ca0fc82093801 100644 (file)
--- a/res/res_pjsip.c
+++ b/res/res_pjsip.c
@@ -40,6 +40,8 @@
 /*** MODULEINFO
        <depend>pjproject</depend>
        <depend>res_sorcery_config</depend>
+       <depend>res_sorcery_memory</depend>
+       <depend>res_sorcery_astdb</depend>
        <support_level>core</support_level>
  ***/
 
@@ -1811,7 +1813,7 @@ static pjsip_endpoint *ast_pjsip_endpoint;
 
 static struct ast_threadpool *sip_threadpool;
 
-static int register_service(void *data)
+static int register_service_noref(void *data)
 {
        pjsip_module **module = data;
        if (!ast_pjsip_endpoint) {
@@ -1823,19 +1825,33 @@ static int register_service(void *data)
                return -1;
        }
        ast_debug(1, "Registered SIP service %.*s (%p)\n", (int) pj_strlen(&(*module)->name), pj_strbuf(&(*module)->name), *module);
-       ast_module_ref(ast_module_info->self);
        return 0;
 }
 
+static int register_service(void *data)
+{
+       int res;
+
+       if (!(res = register_service_noref(data))) {
+               ast_module_ref(ast_module_info->self);
+       }
+
+       return res;
+}
+
+int internal_sip_register_service(pjsip_module *module)
+{
+       return ast_sip_push_task_synchronous(NULL, register_service_noref, &module);
+}
+
 int ast_sip_register_service(pjsip_module *module)
 {
        return ast_sip_push_task_synchronous(NULL, register_service, &module);
 }
 
-static int unregister_service(void *data)
+static int unregister_service_noref(void *data)
 {
        pjsip_module **module = data;
-       ast_module_unref(ast_module_info->self);
        if (!ast_pjsip_endpoint) {
                return -1;
        }
@@ -1844,6 +1860,22 @@ static int unregister_service(void *data)
        return 0;
 }
 
+static int unregister_service(void *data)
+{
+       int res;
+
+       if (!(res = unregister_service_noref(data))) {
+               ast_module_unref(ast_module_info->self);
+       }
+
+       return res;
+}
+
+int internal_sip_unregister_service(pjsip_module *module)
+{
+       return ast_sip_push_task_synchronous(NULL, unregister_service_noref, &module);
+}
+
 void ast_sip_unregister_service(pjsip_module *module)
 {
        ast_sip_push_task_synchronous(NULL, unregister_service, &module);
@@ -1990,26 +2022,38 @@ struct ast_sip_endpoint *ast_sip_identify_endpoint(pjsip_rx_data *rdata)
 
 AST_RWLIST_HEAD_STATIC(endpoint_formatters, ast_sip_endpoint_formatter);
 
-int ast_sip_register_endpoint_formatter(struct ast_sip_endpoint_formatter *obj)
+void internal_sip_register_endpoint_formatter(struct ast_sip_endpoint_formatter *obj)
 {
        SCOPED_LOCK(lock, &endpoint_formatters, AST_RWLIST_WRLOCK, AST_RWLIST_UNLOCK);
        AST_RWLIST_INSERT_TAIL(&endpoint_formatters, obj, next);
+}
+
+int ast_sip_register_endpoint_formatter(struct ast_sip_endpoint_formatter *obj)
+{
+       internal_sip_register_endpoint_formatter(obj);
        ast_module_ref(ast_module_info->self);
        return 0;
 }
 
-void ast_sip_unregister_endpoint_formatter(struct ast_sip_endpoint_formatter *obj)
+int internal_sip_unregister_endpoint_formatter(struct ast_sip_endpoint_formatter *obj)
 {
        struct ast_sip_endpoint_formatter *i;
        SCOPED_LOCK(lock, &endpoint_formatters, AST_RWLIST_WRLOCK, AST_RWLIST_UNLOCK);
        AST_RWLIST_TRAVERSE_SAFE_BEGIN(&endpoint_formatters, i, next) {
                if (i == obj) {
                        AST_RWLIST_REMOVE_CURRENT(next);
-                       ast_module_unref(ast_module_info->self);
                        break;
                }
        }
        AST_RWLIST_TRAVERSE_SAFE_END;
+       return i == obj ? 0 : -1;
+}
+
+void ast_sip_unregister_endpoint_formatter(struct ast_sip_endpoint_formatter *obj)
+{
+       if (!internal_sip_unregister_endpoint_formatter(obj)) {
+               ast_module_unref(ast_module_info->self);
+       }
 }
 
 int ast_sip_format_endpoint_ami(struct ast_sip_endpoint *endpoint,
@@ -3234,7 +3278,7 @@ static int load_module(void)
                return AST_MODULE_LOAD_DECLINE;
        }
 
-       if (ast_sip_register_service(&supplement_module)) {
+       if (internal_sip_register_service(&supplement_module)) {
                ast_log(LOG_ERROR, "Failed to initialize supplement hooks. Aborting load\n");
                ast_sip_destroy_distributor();
                ast_res_pjsip_destroy_configuration();
@@ -3249,9 +3293,9 @@ static int load_module(void)
                return AST_MODULE_LOAD_DECLINE;
        }
 
-       if (ast_sip_initialize_outbound_authentication()) {
+       if (internal_sip_initialize_outbound_authentication()) {
                ast_log(LOG_ERROR, "Failed to initialize outbound authentication. Aborting load\n");
-               ast_sip_unregister_service(&supplement_module);
+               internal_sip_unregister_service(&supplement_module);
                ast_sip_destroy_distributor();
                ast_res_pjsip_destroy_configuration();
                ast_sip_destroy_global_headers();
@@ -3267,8 +3311,6 @@ static int load_module(void)
 
        ast_res_pjsip_init_options_handling(0);
 
-       ast_module_ref(ast_module_info->self);
-
        return AST_MODULE_LOAD_SUCCESS;
 }
 
@@ -3282,9 +3324,41 @@ static int reload_module(void)
        return 0;
 }
 
+static int unload_pjsip(void *data)
+{
+       if (memory_pool) {
+               pj_pool_release(memory_pool);
+               memory_pool = NULL;
+       }
+       if (ast_pjsip_endpoint) {
+               pjsip_endpt_destroy(ast_pjsip_endpoint);
+               ast_pjsip_endpoint = NULL;
+       }
+       pj_caching_pool_destroy(&caching_pool);
+       pj_shutdown();
+       return 0;
+}
+
 static int unload_module(void)
 {
-       /* This will never get called as this module can't be unloaded */
+       ast_res_pjsip_cleanup_options_handling();
+       internal_sip_destroy_outbound_authentication();
+       ast_sip_destroy_distributor();
+       ast_res_pjsip_destroy_configuration();
+       ast_sip_destroy_system();
+       ast_sip_destroy_global_headers();
+       internal_sip_unregister_service(&supplement_module);
+       if (monitor_thread) {
+               stop_monitor_thread();
+       }
+       /* The thread this is called from cannot call PJSIP/PJLIB functions,
+        * so we have to push the work to the threadpool to handle
+        */
+       ast_sip_push_task_synchronous(NULL, unload_pjsip, NULL);
+
+       ast_threadpool_shutdown(sip_threadpool);
+
+       ast_sip_destroy_cli();
        return 0;
 }
 

diff --git a/res/res_pjsip/config_auth.c b/res/res_pjsip/config_auth.c
index fcab6a84b4f2cdc25bbdc404a8ca2c8e78726e16..773889c7a7df6d0d855df74c6f62b7ccbd5f9aa1 100644 (file)
--- a/res/res_pjsip/config_auth.c
+++ b/res/res_pjsip/config_auth.c
@@ -312,7 +312,7 @@ int ast_sip_initialize_sorcery_auth(void)
        ast_sorcery_object_field_register_custom(sorcery, SIP_SORCERY_AUTH_TYPE, "auth_type",
                        "userpass", auth_type_handler, auth_type_to_str, NULL, 0, 0);
 
-       ast_sip_register_endpoint_formatter(&endpoint_auth_formatter);
+       internal_sip_register_endpoint_formatter(&endpoint_auth_formatter);
 
        cli_formatter = ao2_alloc(sizeof(struct ast_sip_cli_formatter_entry), NULL);
        if (!cli_formatter) {
@@ -337,6 +337,7 @@ int ast_sip_destroy_sorcery_auth(void)
 {
        ast_cli_unregister_multiple(cli_commands, ARRAY_LEN(cli_commands));
        ast_sip_unregister_cli_formatter(cli_formatter);
+       internal_sip_unregister_endpoint_formatter(&endpoint_auth_formatter);
 
        return 0;
 }

diff --git a/res/res_pjsip/config_transport.c b/res/res_pjsip/config_transport.c
index ce170da36e1c10ca3349db56ea66815b91a6f645..9c7298b981c920ab9489673fee6567360c638fba 100644 (file)
--- a/res/res_pjsip/config_transport.c
+++ b/res/res_pjsip/config_transport.c
@@ -769,7 +769,7 @@ int ast_sip_initialize_sorcery_transport(void)
        ast_sorcery_object_field_register(sorcery, "transport", "cos", "0", OPT_UINT_T, 0, FLDSET(struct ast_sip_transport, cos));
        ast_sorcery_object_field_register(sorcery, "transport", "websocket_write_timeout", AST_DEFAULT_WEBSOCKET_WRITE_TIMEOUT_STR, OPT_INT_T, PARSE_IN_RANGE, FLDSET(struct ast_sip_transport, write_timeout), 1, INT_MAX);
 
-       ast_sip_register_endpoint_formatter(&endpoint_transport_formatter);
+       internal_sip_register_endpoint_formatter(&endpoint_transport_formatter);
 
        cli_formatter = ao2_alloc(sizeof(struct ast_sip_cli_formatter_entry), NULL);
        if (!cli_formatter) {
@@ -795,5 +795,7 @@ int ast_sip_destroy_sorcery_transport(void)
        ast_cli_unregister_multiple(cli_commands, ARRAY_LEN(cli_commands));
        ast_sip_unregister_cli_formatter(cli_formatter);
 
+       internal_sip_unregister_endpoint_formatter(&endpoint_transport_formatter);
+
        return 0;
 }

diff --git a/res/res_pjsip/include/res_pjsip_private.h b/res/res_pjsip/include/res_pjsip_private.h
index fa37c8c4beb7b7024b15428e86bf4cd768bd53df..b3d66dbb5b57db1ea042a6f59ce4ce6360edfda5 100644 (file)
--- a/res/res_pjsip/include/res_pjsip_private.h
+++ b/res/res_pjsip/include/res_pjsip_private.h
@@ -57,7 +57,15 @@ int ast_res_pjsip_init_contact_transports(void);
  * \retval 0 Success
  * \retval non-zero Failure
  */
-int ast_sip_initialize_outbound_authentication(void);
+int internal_sip_initialize_outbound_authentication(void);
+
+/*!
+ * \brief Destroy outbound authentication support
+ *
+ * \retval 0 Success
+ * \retval non-zero Failure
+ */
+void internal_sip_destroy_outbound_authentication(void);
 
 /*!
  * \brief Initialize system configuration
@@ -112,4 +120,24 @@ char *ast_sip_global_default_outbound_endpoint(void);
 int ast_sip_initialize_cli(void);
 void ast_sip_destroy_cli(void);
 
+/*!
+ * \internal \brief Used by res_pjsip.so to register a service without adding a self reference
+ */
+int internal_sip_register_service(pjsip_module *module);
+
+/*!
+ * \internal \brief Used by res_pjsip.so to unregister a service without removing a self reference
+ */
+int internal_sip_unregister_service(pjsip_module *module);
+
+/*!
+ * \internal \brief Used by res_pjsip.so to register an endpoint formatter without adding a self reference
+ */
+void internal_sip_register_endpoint_formatter(struct ast_sip_endpoint_formatter *obj);
+
+/*!
+ * \internal \brief Used by res_pjsip.so to unregister a endpoint formatter without removing a self reference
+ */
+int internal_sip_unregister_endpoint_formatter(struct ast_sip_endpoint_formatter *obj);
+
 #endif /* RES_PJSIP_PRIVATE_H_ */

diff --git a/res/res_pjsip/location.c b/res/res_pjsip/location.c
index f3623516167f319a9c5c7098ccb0e9189725f406..73ffdca0e1f3a00dd349a65ef20fdac0e2a6404f 100644 (file)
--- a/res/res_pjsip/location.c
+++ b/res/res_pjsip/location.c
@@ -870,7 +870,7 @@ int ast_sip_initialize_sorcery_location(void)
        ast_sorcery_object_field_register(sorcery, "aor", "outbound_proxy", "", OPT_STRINGFIELD_T, 0, STRFLDSET(struct ast_sip_aor, outbound_proxy));
        ast_sorcery_object_field_register(sorcery, "aor", "support_path", "no", OPT_BOOL_T, 1, FLDSET(struct ast_sip_aor, support_path));
 
-       ast_sip_register_endpoint_formatter(&endpoint_aor_formatter);
+       internal_sip_register_endpoint_formatter(&endpoint_aor_formatter);
 
        contact_formatter = ao2_alloc(sizeof(struct ast_sip_cli_formatter_entry), NULL);
        if (!contact_formatter) {
@@ -911,6 +911,8 @@ int ast_sip_destroy_sorcery_location(void)
        ast_sip_unregister_cli_formatter(contact_formatter);
        ast_sip_unregister_cli_formatter(aor_formatter);
 
+       internal_sip_unregister_endpoint_formatter(&endpoint_aor_formatter);
+
        return 0;
 }
 

diff --git a/res/res_pjsip/pjsip_configuration.c b/res/res_pjsip/pjsip_configuration.c
index 7dad58c1531a6f61c0b01bc244db5082d3d567fa..d818361a4a044dab9a641ccd31dc2582ed9030b1 100644 (file)
--- a/res/res_pjsip/pjsip_configuration.c
+++ b/res/res_pjsip/pjsip_configuration.c
@@ -1670,7 +1670,9 @@ int ast_res_pjsip_initialize_configuration(const struct ast_module_info *ast_mod
                return -1;
        }
 
-       ast_sorcery_internal_object_register(sip_sorcery, "nat_hook", sip_nat_hook_alloc, NULL, NULL);
+       if (ast_sorcery_internal_object_register(sip_sorcery, "nat_hook", sip_nat_hook_alloc, NULL, NULL)) {
+               ast_log(LOG_ERROR, "Failed to register nat_hook\n");
+       }
 
        ast_sorcery_object_field_register(sip_sorcery, "endpoint", "type", "", OPT_NOOP_T, 0, 0);
        ast_sorcery_object_field_register(sip_sorcery, "endpoint", "context", "default", OPT_STRINGFIELD_T, 0, STRFLDSET(struct ast_sip_endpoint, context));
@@ -1849,6 +1851,7 @@ void ast_res_pjsip_destroy_configuration(void)
        ast_sip_unregister_cli_formatter(endpoint_formatter);
        ast_sip_unregister_cli_formatter(channel_formatter);
        ast_sorcery_unref(sip_sorcery);
+       ao2_cleanup(persistent_endpoints);
 }
 
 int ast_res_pjsip_reload_configuration(void)

diff --git a/res/res_pjsip/pjsip_distributor.c b/res/res_pjsip/pjsip_distributor.c
index 90744fdf0995ee223e3f0f285da56403f38af947..e32f02833f7dc94561c556e58fa1430a07ad6d31 100644 (file)
--- a/res/res_pjsip/pjsip_distributor.c
+++ b/res/res_pjsip/pjsip_distributor.c
@@ -21,6 +21,7 @@
 #include <pjsip.h>
 
 #include "asterisk/res_pjsip.h"
+#include "include/res_pjsip_private.h"
 
 static int distribute(void *data);
 static pj_bool_t distributor(pjsip_rx_data *rdata);
@@ -222,7 +223,7 @@ struct ast_sip_auth *ast_sip_get_artificial_auth(void)
        return artificial_auth;
 }
 
-static struct ast_sip_endpoint *artificial_endpoint;
+static struct ast_sip_endpoint *artificial_endpoint = NULL;
 
 static int create_artificial_endpoint(void)
 {
@@ -236,7 +237,7 @@ static int create_artificial_endpoint(void)
         * the proper size of the vector is returned. This value is
         * not actually used anywhere
         */
-       AST_VECTOR_APPEND(&artificial_endpoint->inbound_auths, "artificial-auth");
+       AST_VECTOR_APPEND(&artificial_endpoint->inbound_auths, ast_strdup("artificial-auth"));
        return 0;
 }
 
@@ -373,13 +374,13 @@ int ast_sip_initialize_distributor(void)
                return -1;
        }
 
-       if (ast_sip_register_service(&distributor_mod)) {
+       if (internal_sip_register_service(&distributor_mod)) {
                return -1;
        }
-       if (ast_sip_register_service(&endpoint_mod)) {
+       if (internal_sip_register_service(&endpoint_mod)) {
                return -1;
        }
-       if (ast_sip_register_service(&auth_mod)) {
+       if (internal_sip_register_service(&auth_mod)) {
                return -1;
        }
 
@@ -388,9 +389,9 @@ int ast_sip_initialize_distributor(void)
 
 void ast_sip_destroy_distributor(void)
 {
-       ast_sip_unregister_service(&distributor_mod);
-       ast_sip_unregister_service(&endpoint_mod);
-       ast_sip_unregister_service(&auth_mod);
+       internal_sip_unregister_service(&distributor_mod);
+       internal_sip_unregister_service(&endpoint_mod);
+       internal_sip_unregister_service(&auth_mod);
 
        ao2_cleanup(artificial_auth);
        ao2_cleanup(artificial_endpoint);

diff --git a/res/res_pjsip/pjsip_global_headers.c b/res/res_pjsip/pjsip_global_headers.c
index 0fcc3a1398d0d951e413d389c610f01f0d79da64..057b0c3d092545d5a41a8f0c8a5ad93e7c53a6d7 100644 (file)
--- a/res/res_pjsip/pjsip_global_headers.c
+++ b/res/res_pjsip/pjsip_global_headers.c
@@ -23,6 +23,7 @@
 
 #include "asterisk/res_pjsip.h"
 #include "asterisk/linkedlists.h"
+#include "include/res_pjsip_private.h"
 
 static pj_status_t add_request_headers(pjsip_tx_data *tdata);
 static pj_status_t add_response_headers(pjsip_tx_data *tdata);
@@ -152,7 +153,7 @@ void ast_sip_initialize_global_headers(void)
        AST_RWLIST_HEAD_INIT(&request_headers);
        AST_RWLIST_HEAD_INIT(&response_headers);
 
-       ast_sip_register_service(&global_header_mod);
+       internal_sip_register_service(&global_header_mod);
 }
 
 static void destroy_headers(struct header_list *headers)
@@ -169,4 +170,6 @@ void ast_sip_destroy_global_headers(void)
 {
        destroy_headers(&request_headers);
        destroy_headers(&response_headers);
+
+       internal_sip_unregister_service(&global_header_mod);
 }

diff --git a/res/res_pjsip/pjsip_options.c b/res/res_pjsip/pjsip_options.c
index 92755523029cd5bd7edef560064fc233a458c0c5..cf55f4dd64f093ef4bf2d3951d8743438f447e27 100644 (file)
--- a/res/res_pjsip/pjsip_options.c
+++ b/res/res_pjsip/pjsip_options.c
@@ -1123,7 +1123,7 @@ int ast_res_pjsip_init_options_handling(int reload)
                return -1;
        }
 
-       ast_sip_register_endpoint_formatter(&contact_status_formatter);
+       internal_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));
 
@@ -1136,7 +1136,7 @@ void ast_res_pjsip_cleanup_options_handling(void)
 {
        ast_cli_unregister_multiple(cli_options, ARRAY_LEN(cli_options));
        ast_manager_unregister("PJSIPQualify");
-       ast_sip_unregister_endpoint_formatter(&contact_status_formatter);
+       internal_sip_unregister_endpoint_formatter(&contact_status_formatter);
 
        pjsip_endpt_unregister_module(ast_sip_get_pjsip_endpoint(), &options_module);
        ao2_cleanup(sched_qualifies);

diff --git a/res/res_pjsip/pjsip_outbound_auth.c b/res/res_pjsip/pjsip_outbound_auth.c
index 28ca3ec8a208f2c921077b87db5ebebb242aa4fc..1f754227a476d667f23382e55d49a7378b73599e 100644 (file)
--- a/res/res_pjsip/pjsip_outbound_auth.c
+++ b/res/res_pjsip/pjsip_outbound_auth.c
@@ -91,6 +91,11 @@ int ast_sip_dialog_setup_outbound_authentication(pjsip_dialog *dlg, const struct
        return 0;
 }
 
-int ast_sip_initialize_outbound_authentication(void) {
-       return ast_sip_register_service(&outbound_auth_mod);
+int internal_sip_initialize_outbound_authentication(void) {
+       return internal_sip_register_service(&outbound_auth_mod);
+}
+
+
+void internal_sip_destroy_outbound_authentication(void) {
+       internal_sip_unregister_service(&outbound_auth_mod);
 }