res/res_pjsip: Fix crash due to misuse of session->media between threads.

This patch makes sure that thread running ast_taskprocessor_execute
cannot suddenly dispose the session->media object making the other
threads (running pbx_thread / bridge_channel_ind_thread) crash when they
try to access the pointer to invalid memory. We were experiencing a crash due
to a misuse of session->media container between threads running
(bridge_channel_ind_thread/pbx_thread) and the thread running
ast_taskprocessor_execute. Depending on the SIP flow (during a disconnection)
and the threads' code path, the session->media container was being destroyed
(and set to NULL) by the thread running ast_taskprocessor_execute while the
thread running t38_framehook_read was still referring to it.
Now res_pjsip_t38 is referring a session_media in a datastore.

ASTERISK-28156

Change-Id: Ia92e2389b8d804bf205473e92ec06217e87ce237

diff --git a/res/res_pjsip_t38.c b/res/res_pjsip_t38.c
index cff625f76a0bff6179167c5ade580773f9a3dc0d..cd225d4b111b5cfcc9ee362e1bf7d160d30fc70b 100644 (file)
--- a/res/res_pjsip_t38.c
+++ b/res/res_pjsip_t38.c
@@ -79,6 +79,17 @@ static const struct ast_datastore_info t38_datastore = {
        .destroy = t38_state_destroy,
 };
 
+static void session_media_dec(void *obj)
+{
+       ao2_cleanup(obj);
+}
+
+/*! \brief Datastore for T.38 session_media information */
+static const struct ast_datastore_info session_media_datastore = {
+       .type = "t38_session_media",
+       .destroy = session_media_dec,
+};
+
 /*! \brief Structure for T.38 parameters task data */
 struct t38_parameters_task_data {
        /*! \brief Session itself */
@@ -120,6 +131,36 @@ static struct t38_parameters_task_data *t38_parameters_task_data_alloc(struct as
        return data;
 }
 
+/*! \brief Helper function which retrieves a T.38 session_media from datastore */
+static struct ast_sip_session_media *session_media_from_datastore_addref(struct ast_sip_session *session)
+{
+       struct ast_datastore *datastore = NULL;
+       struct ast_sip_session_media *session_media = NULL;
+
+       if ((datastore = ast_sip_session_get_datastore(session, "t38_session_media"))) {
+               session_media = datastore->data;
+               ao2_ref(session_media, +1);
+               ao2_ref(datastore, -1);
+       }
+
+       return session_media;
+}
+
+/*! \brief Helper function which allocates datastore with a session media */
+static struct ast_datastore *create_datastore_session_media(struct ast_sip_session *session, struct ast_sip_session_media *session_media)
+{
+       struct ast_datastore *datastore = NULL;
+
+       if (!(datastore = ast_sip_session_alloc_datastore(&session_media_datastore, "t38_session_media"))
+               || ast_sip_session_add_datastore(session, datastore)) {
+               return NULL;
+       }
+
+       datastore->data = session_media;
+       ao2_bump(session_media);
+       return datastore;
+}
+
 /*! \brief Helper function for changing the T.38 state */
 static void t38_change_state(struct ast_sip_session *session, struct ast_sip_session_media *session_media,
        struct t38_state *state, enum ast_sip_session_t38state new_state)
@@ -198,7 +239,7 @@ static int t38_automatic_reject(void *obj)
 {
        RAII_VAR(struct ast_sip_session *, session, obj, ao2_cleanup);
        RAII_VAR(struct ast_datastore *, datastore, ast_sip_session_get_datastore(session, "t38"), ao2_cleanup);
-       RAII_VAR(struct ast_sip_session_media *, session_media, ao2_find(session->media, "image", OBJ_KEY), ao2_cleanup);
+       RAII_VAR(struct ast_sip_session_media *, session_media, session_media_from_datastore_addref(session), ao2_cleanup);
 
        if (!datastore) {
                return 0;
@@ -255,6 +296,10 @@ static int t38_initialize_session(struct ast_sip_session *session, struct ast_si
                return 0;
        }
 
+       if (!create_datastore_session_media(session, session_media)) {
+               return -1;
+       }
+
        if (!(session_media->udptl = ast_udptl_new_with_bindaddr(NULL, NULL, 0, &address))) {
                return -1;
        }
@@ -292,7 +337,8 @@ static int t38_reinvite_response_cb(struct ast_sip_session *session, pjsip_rx_da
 {
        struct pjsip_status_line status = rdata->msg_info.msg->line.status;
        struct t38_state *state;
-       struct ast_sip_session_media *session_media = NULL;
+       RAII_VAR(struct ast_sip_session_media *, session_media,
+               session_media_from_datastore_addref(session), ao2_cleanup);
 
        if (status.code / 100 <= 1) {
                /* Ignore any non-final responses (1xx) */
@@ -300,7 +346,7 @@ static int t38_reinvite_response_cb(struct ast_sip_session *session, pjsip_rx_da
        }
 
        if (!session->channel || !(state = t38_state_get_or_alloc(session)) ||
-               !(session_media = ao2_find(session->media, "image", OBJ_KEY))) {
+               !session_media) {
                ast_log(LOG_WARNING, "Received %d response to T.38 re-invite on '%s' but state unavailable\n",
                        status.code,
                        session->channel ? ast_channel_name(session->channel) : "unknown channel");
@@ -311,7 +357,6 @@ static int t38_reinvite_response_cb(struct ast_sip_session *session, pjsip_rx_da
        t38_change_state(session, session_media, state,
                (status.code / 100 == 2) ? T38_ENABLED : T38_REJECTED);
 
-       ao2_cleanup(session_media);
        return 0;
 }
 
@@ -446,10 +491,7 @@ static struct ast_frame *t38_framehook_write(struct ast_channel *chan,
        } else if (f->frametype == AST_FRAME_MODEM) {
                struct ast_sip_session_media *session_media;
 
-               /* Avoid deadlock between chan and the session->media container lock */
-               ast_channel_unlock(chan);
-               session_media = ao2_find(session->media, "image", OBJ_SEARCH_KEY);
-               ast_channel_lock(chan);
+               session_media = session_media_from_datastore_addref(session);
                if (session_media && session_media->udptl) {
                        ast_udptl_write(session_media->udptl, f);
                }
@@ -466,10 +508,7 @@ static struct ast_frame *t38_framehook_read(struct ast_channel *chan,
        if (ast_channel_fdno(session->channel) == 5) {
                struct ast_sip_session_media *session_media;
 
-               /* Avoid deadlock between chan and the session->media container lock */
-               ast_channel_unlock(chan);
-               session_media = ao2_find(session->media, "image", OBJ_SEARCH_KEY);
-               ast_channel_lock(chan);
+               session_media = session_media_from_datastore_addref(session);
                if (session_media && session_media->udptl) {
                        f = ast_udptl_read(session_media->udptl);
                }