[Asterisk-code-review] pjsip: Match lifetime of INVITE session to our session. (asterisk[16])
Joshua Colp
asteriskteam at digium.com
Fri Oct 30 09:10:28 CDT 2020
Joshua Colp has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/15122 )
Change subject: pjsip: Match lifetime of INVITE session to our session.
......................................................................
pjsip: Match lifetime of INVITE session to our session.
In some circumstances it was possible for an INVITE
session to be destroyed while we were still using it.
This occurred due to the reference on the INVITE session
being released internally as a result of its state
changing to DISCONNECTED.
This change adds a reference to the INVITE session
which is released when our own session is destroyed,
ensuring that the INVITE session remains valid for
the lifetime of our session.
ASTERISK-29022
Change-Id: I300c6d9005ff0e6efbe1132daefc7e47ca6228c9
---
M channels/chan_pjsip.c
M res/res_pjsip_session.c
2 files changed, 30 insertions(+), 150 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/22/15122/1
diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index 1c10fbf..649dd36 100644
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -673,9 +673,6 @@
ast_log(LOG_ERROR, "Session already DISCONNECTED [reason=%d (%s)]\n",
session->inv_session->cause,
pjsip_get_status_text(session->inv_session->cause)->ptr);
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
return 0;
}
@@ -692,10 +689,6 @@
ast_sip_session_send_response(session, packet);
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
-
if (status != PJ_SUCCESS) {
char err[PJ_ERR_MSG_SIZE];
@@ -725,14 +718,6 @@
ast_setstate(ast, AST_STATE_UP);
session = ao2_bump(channel->session);
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- if (pjsip_inv_add_ref(session->inv_session) != PJ_SUCCESS) {
- ast_log(LOG_ERROR, "Can't increase the session reference counter\n");
- ao2_ref(session, -1);
- return -1;
- }
-#endif
-
/* the answer task needs to be pushed synchronously otherwise a race condition
can occur between this thread and bridging (specifically when native bridging
attempts to do direct media) */
@@ -742,9 +727,6 @@
if (res == -1) {
ast_log(LOG_ERROR,"Cannot answer '%s': Unable to push answer task to the threadpool.\n",
ast_channel_name(session->channel));
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
}
ao2_ref(session, -1);
ast_channel_lock(ast);
@@ -1343,9 +1325,6 @@
ast_sip_session_send_response(session, packet);
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
ao2_ref(ind_data, -1);
return 0;
@@ -1377,31 +1356,20 @@
ast_log(LOG_ERROR, "Session already DISCONNECTED [reason=%d (%s)]\n",
session->inv_session->cause,
pjsip_get_status_text(session->inv_session->cause)->ptr);
- goto failure;
+ return -1;
}
if (ast_sip_create_request("INFO", session->inv_session->dlg, session->endpoint, NULL, NULL, &tdata)) {
ast_log(LOG_ERROR, "Could not create text video update INFO request\n");
- goto failure;
+ return -1;
}
if (ast_sip_add_body(tdata, &body)) {
ast_log(LOG_ERROR, "Could not add body to text video update INFO request\n");
- goto failure;
+ return -1;
}
ast_sip_session_send_request(session, tdata);
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
-
return 0;
-
-failure:
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
- return -1;
-
}
/*!
@@ -1449,9 +1417,6 @@
ast_log(LOG_ERROR, "Session already DISCONNECTED [reason=%d (%s)]\n",
session->inv_session->cause,
pjsip_get_status_text(session->inv_session->cause)->ptr);
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
ao2_ref(session, -1);
return -1;
}
@@ -1492,10 +1457,6 @@
}
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
-
ao2_ref(session, -1);
return 0;
}
@@ -1726,18 +1687,9 @@
res = ast_rtp_instance_write(media->rtp, &fr);
} else {
ao2_ref(channel->session, +1);
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- if (pjsip_inv_add_ref(channel->session->inv_session) != PJ_SUCCESS) {
- ast_log(LOG_ERROR, "Can't increase the session reference counter\n");
+ if (ast_sip_push_task(channel->session->serializer, transmit_info_with_vidupdate, channel->session)) {
ao2_cleanup(channel->session);
- } else {
-#endif
- if (ast_sip_push_task(channel->session->serializer, transmit_info_with_vidupdate, channel->session)) {
- ao2_cleanup(channel->session);
- }
-#ifdef HAVE_PJSIP_INV_SESSION_REF
}
-#endif
}
ast_test_suite_event_notify("AST_CONTROL_VIDUPDATE", "Result: Success");
} else {
@@ -1751,17 +1703,7 @@
break;
case AST_CONTROL_CONNECTED_LINE:
ao2_ref(channel->session, +1);
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- if (pjsip_inv_add_ref(channel->session->inv_session) != PJ_SUCCESS) {
- ao2_cleanup(channel->session);
- SCOPE_EXIT_LOG_RTN_VALUE(-1, LOG_ERROR, "%s: Couldn't increase the session reference counter\n",
- ast_channel_name(ast));
- }
-#endif
if (ast_sip_push_task(channel->session->serializer, update_connected_line_information, channel->session)) {
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(channel->session->inv_session);
-#endif
ao2_cleanup(channel->session);
}
break;
@@ -1868,18 +1810,10 @@
if (!ind_data) {
SCOPE_EXIT_LOG_RTN_VALUE(-1, LOG_ERROR, "%s: Couldn't alloc indicate data\n", ast_channel_name(ast));
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- if (pjsip_inv_add_ref(ind_data->session->inv_session) != PJ_SUCCESS) {
- ao2_cleanup(ind_data);
- SCOPE_EXIT_LOG_RTN_VALUE(-1, LOG_ERROR, "%s: Couldn't increase the session reference counter\n", ast_channel_name(ast));
- }
-#endif
+
if (ast_sip_push_task(channel->session->serializer, indicate, ind_data)) {
ast_log(LOG_ERROR, "%s: Cannot send response code %d to endpoint %s. Could not queue task properly\n",
ast_channel_name(ast), response_code, ast_sorcery_object_get_id(channel->session->endpoint));
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(ind_data->session->inv_session);
-#endif
ao2_cleanup(ind_data);
res = -1;
}
@@ -2150,10 +2084,6 @@
}
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(trnf_data->session->inv_session);
-#endif
-
ao2_ref(trnf_data, -1);
ao2_cleanup(endpoint);
ao2_cleanup(contact);
@@ -2170,19 +2100,8 @@
return -1;
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- if (pjsip_inv_add_ref(trnf_data->session->inv_session) != PJ_SUCCESS) {
- ast_log(LOG_ERROR, "Can't increase the session reference counter\n");
- ao2_cleanup(trnf_data);
- return -1;
- }
-#endif
-
if (ast_sip_push_task(channel->session->serializer, transfer, trnf_data)) {
ast_log(LOG_WARNING, "Error requesting transfer\n");
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(trnf_data->session->inv_session);
-#endif
ao2_cleanup(trnf_data);
return -1;
}
@@ -2277,12 +2196,12 @@
ast_log(LOG_ERROR, "Session already DISCONNECTED [reason=%d (%s)]\n",
session->inv_session->cause,
pjsip_get_status_text(session->inv_session->cause)->ptr);
- goto failure;
+ return -1;
}
if (!(body_text = ast_str_create(32))) {
ast_log(LOG_ERROR, "Could not allocate buffer for INFO DTMF.\n");
- goto failure;
+ return -1;
}
ast_str_set(&body_text, 0, "Signal=%c\r\nDuration=%u\r\n", dtmf_data->digit, dtmf_data->duration);
@@ -2290,27 +2209,16 @@
if (ast_sip_create_request("INFO", session->inv_session->dlg, session->endpoint, NULL, NULL, &tdata)) {
ast_log(LOG_ERROR, "Could not create DTMF INFO request\n");
- goto failure;
+ return -1;
}
if (ast_sip_add_body(tdata, &body)) {
ast_log(LOG_ERROR, "Could not add body to DTMF INFO request\n");
pjsip_tx_data_dec_ref(tdata);
- goto failure;
+ return -1;
}
ast_sip_session_send_request(session, tdata);
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
-
return 0;
-
-failure:
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(session->inv_session);
-#endif
- return -1;
-
}
/*! \brief Function called by core to stop a DTMF digit */
@@ -2351,19 +2259,8 @@
return -1;
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- if (pjsip_inv_add_ref(dtmf_data->session->inv_session) != PJ_SUCCESS) {
- ast_log(LOG_ERROR, "Can't increase the session reference counter\n");
- ao2_cleanup(dtmf_data);
- return -1;
- }
-#endif
-
if (ast_sip_push_task(channel->session->serializer, transmit_info_dtmf, dtmf_data)) {
ast_log(LOG_WARNING, "Error sending DTMF via INFO.\n");
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(dtmf_data->session->inv_session);
-#endif
ao2_cleanup(dtmf_data);
return -1;
}
@@ -2858,10 +2755,6 @@
ast_sip_send_request(tdata, data->session->inv_session->dlg, data->session->endpoint, NULL, NULL);
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(data->session->inv_session);
-#endif
-
ao2_cleanup(data);
return 0;
@@ -2883,18 +2776,7 @@
return -1;
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- if (pjsip_inv_add_ref(data->session->inv_session) != PJ_SUCCESS) {
- ast_log(LOG_ERROR, "Can't increase the session reference counter\n");
- ao2_ref(data, -1);
- return -1;
- }
-#endif
-
if (ast_sip_push_task(channel->session->serializer, sendtext, data)) {
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(data->session->inv_session);
-#endif
ao2_ref(data, -1);
return -1;
}
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index e8c8fdf..f9342aa 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -2981,6 +2981,9 @@
if (session->inv_session) {
pjsip_dlg_dec_session(session->inv_session->dlg, &session_module);
+#ifdef HAVE_PJSIP_INV_SESSION_REF
+ pjsip_inv_dec_ref(session->inv_session);
+#endif
}
ast_test_suite_event_notify("SESSION_DESTROYED", "Endpoint: %s", endpoint_name);
@@ -3088,6 +3091,24 @@
}
ast_sip_dialog_set_serializer(inv_session->dlg, session->serializer);
ast_sip_dialog_set_endpoint(inv_session->dlg, endpoint);
+
+ /* When a PJSIP INVITE session is created it is created with a reference
+ * count of 1, with that reference being managed by the underlying state
+ * of the INVITE session itself. When the INVITE session transitions to
+ * a DISCONNECTED state that reference is released. This means we can not
+ * rely on that reference to ensure the INVITE session remains for the
+ * lifetime of our session. To ensure it does we add our own reference
+ * and release it when our own session goes away, ensuring that the INVITE
+ * session remains for the lifetime of session.
+ */
+
+#ifdef HAVE_PJSIP_INV_SESSION_REF
+ if (pjsip_inv_add_ref(inv_session) != PJ_SUCCESS) {
+ ast_log(LOG_ERROR, "Can't increase the session reference counter\n");
+ return NULL;
+ }
+#endif
+
pjsip_dlg_inc_session(inv_session->dlg, &session_module);
inv_session->mod_data[session_module.id] = ao2_bump(session);
session->contact = ao2_bump(contact);
@@ -3873,9 +3894,6 @@
ast_sip_session_get_name(invite->session),
invite->session->inv_session->cause,
pjsip_get_status_text(invite->session->inv_session->cause)->ptr);
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(invite->session->inv_session);
-#endif
SCOPE_EXIT_RTN_VALUE(-1);
}
@@ -3993,9 +4011,6 @@
handle_incoming_request(invite->session, invite->rdata);
end:
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(invite->session->inv_session);
-#endif
SCOPE_EXIT_RTN_VALUE(0, "%s\n", ast_sip_session_get_name(invite->session));
}
@@ -4016,20 +4031,6 @@
return;
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- if (pjsip_inv_add_ref(inv_session) != PJ_SUCCESS) {
- ast_log(LOG_ERROR, "Can't increase the session reference counter\n");
- if (inv_session->state != PJSIP_INV_STATE_DISCONNECTED) {
- if (pjsip_inv_initial_answer(inv_session, rdata, 500, NULL, NULL, &tdata) == PJ_SUCCESS) {
- pjsip_inv_terminate(inv_session, 500, PJ_FALSE);
- } else {
- pjsip_inv_send_msg(inv_session, tdata);
- }
- }
- return;
- }
-#endif
-
session = ast_sip_session_alloc(endpoint, NULL, inv_session, rdata);
if (!session) {
if (pjsip_inv_initial_answer(inv_session, rdata, 500, NULL, NULL, &tdata) == PJ_SUCCESS) {
@@ -4037,9 +4038,6 @@
} else {
pjsip_inv_send_msg(inv_session, tdata);
}
-#ifdef HAVE_PJSIP_INV_SESSION_REF
- pjsip_inv_dec_ref(inv_session);
-#endif
return;
}
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15122
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I300c6d9005ff0e6efbe1132daefc7e47ca6228c9
Gerrit-Change-Number: 15122
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20201030/33cc5fd9/attachment-0001.html>
More information about the asterisk-code-review
mailing list