[Asterisk-code-review] res/res pjsip: Fix crash due to misuse of session->media bet... (asterisk[13])
Friendly Automation
asteriskteam at digium.com
Wed Jan 30 07:04:04 CST 2019
Friendly Automation has submitted this change and it was merged. ( https://gerrit.asterisk.org/10601 )
Change subject: res/res_pjsip: Fix crash due to misuse of session->media between threads.
......................................................................
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
---
M res/res_pjsip_t38.c
1 file changed, 51 insertions(+), 12 deletions(-)
Approvals:
George Joseph: Looks good to me, but someone else must approve
Joshua C. Colp: Looks good to me, approved
Friendly Automation: Approved for Submit
diff --git a/res/res_pjsip_t38.c b/res/res_pjsip_t38.c
index cff625f..cd225d4 100644
--- a/res/res_pjsip_t38.c
+++ b/res/res_pjsip_t38.c
@@ -79,6 +79,17 @@
.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 @@
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 @@
{
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 @@
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 @@
{
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 @@
}
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 @@
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 @@
} 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 @@
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);
}
--
To view, visit https://gerrit.asterisk.org/10601
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia92e2389b8d804bf205473e92ec06217e87ce237
Gerrit-Change-Number: 10601
Gerrit-PatchSet: 7
Gerrit-Owner: Paulo Vicentini <paulo.vicentini at gmail.com>
Gerrit-Reviewer: Friendly Automation (1000185)
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua C. Colp <jcolp at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Paulo Vicentini <paulo.vicentini at gmail.com>
Gerrit-Reviewer: Torrey Searle <tsearle at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190130/e2ce2526/attachment-0001.html>
More information about the asterisk-code-review
mailing list