From 1b6df87816bfe0552b7888bf14efd2a82a6c7dbf Mon Sep 17 00:00:00 2001 From: Sungtae Kim Date: Sun, 11 Nov 2018 17:29:10 +0100 Subject: [PATCH] res_pjsip: Patch for res_pjsip_* module load/reload crash The session_supplements for the pjsip makes crashes when the module load/unload. ASTERISK-28157 Change-Id: I5b82be3a75d702cf1933d8d1417f44aa10ad1029 --- include/asterisk/res_pjsip_session.h | 13 ++++- res/res_pjsip/pjsip_message_filter.c | 1 + res/res_pjsip/pjsip_session.c | 85 ++++++++++++++++++++++++---- res/res_pjsip_session.c | 68 +++++++++++++++------- 4 files changed, 136 insertions(+), 31 deletions(-) diff --git a/include/asterisk/res_pjsip_session.h b/include/asterisk/res_pjsip_session.h index ccee8c96d1..fbd1d1659a 100644 --- a/include/asterisk/res_pjsip_session.h +++ b/include/asterisk/res_pjsip_session.h @@ -580,9 +580,13 @@ void ast_sip_session_unregister_sdp_handler(struct ast_sip_session_sdp_handler * * set channel data based on headers in an incoming message. Similarly, * a module could reject an incoming request if desired. * + * \param module Referenced module(NULL safe) * \param supplement The supplement to register */ -void ast_sip_session_register_supplement(struct ast_sip_session_supplement *supplement); +void ast_sip_session_register_supplement_with_module(struct ast_module *module, struct ast_sip_session_supplement *supplement); + +#define ast_sip_session_register_supplement(supplement) \ + ast_sip_session_register_supplement_with_module(AST_MODULE_SELF, supplement) /*! * \brief Unregister a an supplement to SIP session processing @@ -598,6 +602,13 @@ void ast_sip_session_unregister_supplement(struct ast_sip_session_supplement *su */ int ast_sip_session_add_supplements(struct ast_sip_session *session); +/*! + * \brief Remove supplements from a SIP session + * + * \param session The session to remove + */ +void ast_sip_session_remove_supplements(struct ast_sip_session *session); + /*! * \brief Alternative for ast_datastore_alloc() * diff --git a/res/res_pjsip/pjsip_message_filter.c b/res/res_pjsip/pjsip_message_filter.c index f948c4449c..4b0f60dfc9 100644 --- a/res/res_pjsip/pjsip_message_filter.c +++ b/res/res_pjsip/pjsip_message_filter.c @@ -24,6 +24,7 @@ #include "asterisk/res_pjsip.h" #include "asterisk/res_pjsip_session.h" #include "include/res_pjsip_private.h" +#include "asterisk/module.h" #define MOD_DATA_RESTRICTIONS "restrictions" diff --git a/res/res_pjsip/pjsip_session.c b/res/res_pjsip/pjsip_session.c index f3f3a4d878..b9a25f5a43 100644 --- a/res/res_pjsip/pjsip_session.c +++ b/res/res_pjsip/pjsip_session.c @@ -29,22 +29,50 @@ #include "asterisk/lock.h" #include "asterisk/module.h" - AST_RWLIST_HEAD_STATIC(session_supplements, ast_sip_session_supplement); -void ast_sip_session_register_supplement(struct ast_sip_session_supplement *supplement) +/* This structure is used to support module references without + * breaking the ABI of the public struct ast_sip_session_supplement. */ +struct private_sip_session_supplement { + /*! \brief This will be added to the session_supplements list. + * + * This field must be at offset 0 to enable retrieval of this + * structure from session_supplements. + */ + struct ast_sip_session_supplement copy; + /*! Users of this session supplement will hold a reference to the module. */ + struct ast_module *module; + /*! Pointer to the static variable for unregister comparisons. */ + struct ast_sip_session_supplement *original; +}; + +void ast_sip_session_register_supplement_with_module(struct ast_module *module, struct ast_sip_session_supplement *supplement) { struct ast_sip_session_supplement *iter; + struct private_sip_session_supplement *priv; int inserted = 0; SCOPED_LOCK(lock, &session_supplements, AST_RWLIST_WRLOCK, AST_RWLIST_UNLOCK); + ast_assert(supplement != NULL); + if (!supplement->response_priority) { supplement->response_priority = AST_SIP_SESSION_BEFORE_MEDIA; } + priv = ast_calloc(1, sizeof(*priv)); + if (!priv) { + /* Really can't do anything here, just assert and return. */ + ast_assert(priv != NULL); + return; + } + + priv->copy = *supplement; + priv->module = module; + priv->original = supplement; + AST_RWLIST_TRAVERSE_SAFE_BEGIN(&session_supplements, iter, next) { if (iter->priority > supplement->priority) { - AST_RWLIST_INSERT_BEFORE_CURRENT(supplement, next); + AST_RWLIST_INSERT_BEFORE_CURRENT(&priv->copy, next); inserted = 1; break; } @@ -52,7 +80,7 @@ void ast_sip_session_register_supplement(struct ast_sip_session_supplement *supp AST_RWLIST_TRAVERSE_SAFE_END; if (!inserted) { - AST_RWLIST_INSERT_TAIL(&session_supplements, supplement, next); + AST_RWLIST_INSERT_TAIL(&session_supplements, &priv->copy, next); } } @@ -62,17 +90,20 @@ void ast_sip_session_unregister_supplement(struct ast_sip_session_supplement *su SCOPED_LOCK(lock, &session_supplements, AST_RWLIST_WRLOCK, AST_RWLIST_UNLOCK); AST_RWLIST_TRAVERSE_SAFE_BEGIN(&session_supplements, iter, next) { - if (supplement == iter) { + struct private_sip_session_supplement *priv = (void*)iter; + + if (supplement == priv->original) { AST_RWLIST_REMOVE_CURRENT(next); + ast_free(priv); break; } } AST_RWLIST_TRAVERSE_SAFE_END; } -static struct ast_sip_session_supplement *supplement_dup(const struct ast_sip_session_supplement *src) +static struct private_sip_session_supplement *supplement_dup(const struct private_sip_session_supplement *src) { - struct ast_sip_session_supplement *dst = ast_calloc(1, sizeof(*dst)); + struct private_sip_session_supplement *dst = ast_calloc(1, sizeof(*dst)); if (!dst) { return NULL; @@ -89,13 +120,47 @@ int ast_sip_session_add_supplements(struct ast_sip_session *session) SCOPED_LOCK(lock, &session_supplements, AST_RWLIST_RDLOCK, AST_RWLIST_UNLOCK); AST_RWLIST_TRAVERSE(&session_supplements, iter, next) { - struct ast_sip_session_supplement *copy = supplement_dup(iter); + struct private_sip_session_supplement *dup = supplement_dup((void*)iter); - if (!copy) { + if (!dup) { return -1; } - AST_LIST_INSERT_TAIL(&session->supplements, copy, next); + + /* referenced session created. increasing module reference. */ + ast_module_ref(dup->module); + + AST_LIST_INSERT_TAIL(&session->supplements, &dup->copy, next); } return 0; } + +void ast_sip_session_remove_supplements(struct ast_sip_session *session) +{ + struct ast_sip_session_supplement *iter; + + if (!session) { + return; + } + + /* free the supplements */ + while ((iter = AST_LIST_REMOVE_HEAD(&session->supplements, next))) { + struct private_sip_session_supplement *priv = (void*)iter; + if (priv->module) { + /* referenced session closed. decreasing module reference. */ + ast_module_unref(priv->module); + } + + ast_free(iter); + } + + return; +} + +/* This stub is for ABI compatibility. */ +#undef ast_sip_session_register_supplement +void ast_sip_session_register_supplement(struct ast_sip_session_supplement *supplement); +void ast_sip_session_register_supplement(struct ast_sip_session_supplement *supplement) +{ + ast_sip_session_register_supplement_with_module(NULL, supplement); +} diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c index 13c123ae10..f981cd98f1 100644 --- a/res/res_pjsip_session.c +++ b/res/res_pjsip_session.c @@ -58,6 +58,9 @@ #define DEFAULT_NUM_SESSION_MEDIA 2 /* Some forward declarations */ +static void handle_session_begin(struct ast_sip_session *session); +static void handle_session_end(struct ast_sip_session *session); +static void handle_session_destroy(struct ast_sip_session *session); static void handle_incoming_request(struct ast_sip_session *session, pjsip_rx_data *rdata); static void handle_incoming_response(struct ast_sip_session *session, pjsip_rx_data *rdata, enum ast_sip_session_response_priority response_priority); @@ -2088,7 +2091,6 @@ static int datastore_cmp(void *obj, void *arg, int flags) static void session_destructor(void *obj) { struct ast_sip_session *session = obj; - struct ast_sip_session_supplement *supplement; struct ast_sip_session_delayed_request *delay; const char *endpoint_name = session->endpoint ? ast_sorcery_object_get_id(session->endpoint) : ""; @@ -2104,19 +2106,18 @@ static void session_destructor(void *obj) , session->contact ? ast_sorcery_object_get_id(session->contact) : "" ); - while ((supplement = AST_LIST_REMOVE_HEAD(&session->supplements, next))) { - if (supplement->session_destroy) { - supplement->session_destroy(session); - } - ast_free(supplement); - } + /* fire session destroy handler */ + handle_session_destroy(session); + + /* remove all registered supplements */ + ast_sip_session_remove_supplements(session); + AST_LIST_HEAD_DESTROY(&session->supplements); ast_taskprocessor_unreference(session->serializer); ao2_cleanup(session->datastores); ast_sip_session_media_state_free(session->active_media_state); ast_sip_session_media_state_free(session->pending_media_state); - AST_LIST_HEAD_DESTROY(&session->supplements); while ((delay = AST_LIST_REMOVE_HEAD(&session->delayed_requests, next))) { delayed_request_free(delay); } @@ -2165,7 +2166,6 @@ struct ast_sip_session *ast_sip_session_alloc(struct ast_sip_endpoint *endpoint, { RAII_VAR(struct ast_sip_session *, session, NULL, ao2_cleanup); struct ast_sip_session *ret_session; - struct ast_sip_session_supplement *iter; int dsp_features = 0; session = ao2_alloc(sizeof(*session), session_destructor); @@ -2246,11 +2246,9 @@ struct ast_sip_session *ast_sip_session_alloc(struct ast_sip_endpoint *endpoint, ao2_ref(session, -1); return NULL; } - AST_LIST_TRAVERSE(&session->supplements, iter, next) { - if (iter->session_begin) { - iter->session_begin(session); - } - } + + /* Fire seesion begin handlers */ + handle_session_begin(session); /* Avoid unnecessary ref manipulation to return a session */ ret_session = session; @@ -3315,6 +3313,40 @@ static void handle_incoming_request(struct ast_sip_session *session, pjsip_rx_da } } +static void handle_session_begin(struct ast_sip_session *session) +{ + struct ast_sip_session_supplement *iter; + + AST_LIST_TRAVERSE(&session->supplements, iter, next) { + if (iter->session_begin) { + iter->session_begin(session); + } + } +} + +static void handle_session_destroy(struct ast_sip_session *session) +{ + struct ast_sip_session_supplement *iter; + + AST_LIST_TRAVERSE(&session->supplements, iter, next) { + if (iter->session_destroy) { + iter->session_destroy(session); + } + } +} + +static void handle_session_end(struct ast_sip_session *session) +{ + struct ast_sip_session_supplement *iter; + + /* Session is dead. Notify the supplements. */ + AST_LIST_TRAVERSE(&session->supplements, iter, next) { + if (iter->session_end) { + iter->session_end(session); + } + } +} + static void handle_incoming_response(struct ast_sip_session *session, pjsip_rx_data *rdata, enum ast_sip_session_response_priority response_priority) { @@ -3387,17 +3419,13 @@ static void handle_outgoing_response(struct ast_sip_session *session, pjsip_tx_d static int session_end(void *vsession) { struct ast_sip_session *session = vsession; - struct ast_sip_session_supplement *iter; /* Stop the scheduled termination */ sip_session_defer_termination_stop_timer(session); /* Session is dead. Notify the supplements. */ - AST_LIST_TRAVERSE(&session->supplements, iter, next) { - if (iter->session_end) { - iter->session_end(session); - } - } + handle_session_end(session); + return 0; }