[Asterisk-code-review] AST-2020-001 - res_pjsip: Return dialog locked and referenced (asterisk[13])
Kevin Harwell
asteriskteam at digium.com
Thu Nov 5 11:02:06 CST 2020
Kevin Harwell has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/15162 )
Change subject: AST-2020-001 - res_pjsip: Return dialog locked and referenced
......................................................................
AST-2020-001 - res_pjsip: Return dialog locked and referenced
pjproject returns the dialog locked and with a reference. However,
in Asterisk the method that handles this decrements the reference
and removes the lock prior to returning. This makes it possible,
under some circumstances, for another thread to free said dialog
before the thread that created it attempts to use it again. Of
course when the thread that created it tries to use a freed dialog
a crash can occur.
This patch makes it so Asterisk now returns the newly created
dialog both locked, and with an added reference. This allows the
caller to de-reference, and unlock the dialog when it is safe to
do so.
In the case of a new SIP Invite the lock, and reference are now
held for the entirety of the new invite handling process.
Otherwise it's possible for the dialog, or its dependent objects,
like the transaction, to disappear. For example if there is a TCP
transport error.
ASTERISK-29057 #close
Change-Id: I5ef645a47829596f402cf383dc02c629c618969e
---
M include/asterisk/res_pjsip.h
M res/res_pjsip.c
M res/res_pjsip_pubsub.c
M res/res_pjsip_session.c
4 files changed, 226 insertions(+), 26 deletions(-)
Approvals:
George Joseph: Looks good to me, approved
Kevin Harwell: Approved for Submit
Friendly Automation: Verified
diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h
index f48ea3e..9b6b98d 100644
--- a/include/asterisk/res_pjsip.h
+++ b/include/asterisk/res_pjsip.h
@@ -1958,6 +1958,11 @@
/*!
* \brief General purpose method for creating a UAS dialog with an endpoint
*
+ * \deprecated This function is unsafe (due to the returned object not being locked nor
+ * having its reference incremented) and should no longer be used. Instead
+ * use ast_sip_create_dialog_uas_locked so a properly locked and referenced
+ * object is returned.
+ *
* \param endpoint A pointer to the endpoint
* \param rdata The request that is starting the dialog
* \param[out] status On failure, the reason for failure in creating the dialog
@@ -1965,6 +1970,44 @@
pjsip_dialog *ast_sip_create_dialog_uas(const struct ast_sip_endpoint *endpoint, pjsip_rx_data *rdata, pj_status_t *status);
/*!
+ * \brief General purpose method for creating a UAS dialog with an endpoint
+ *
+ * This function creates and returns a locked, and referenced counted pjsip
+ * dialog object. The caller is thus responsible for freeing the allocated
+ * memory, decrementing the reference, and releasing the lock when done with
+ * the returned object.
+ *
+ * \note The safest way to unlock the object, and decrement its reference is by
+ * calling pjsip_dlg_dec_lock. Alternatively, pjsip_dlg_dec_session can be
+ * used to decrement the reference only.
+ *
+ * The dialog is returned locked and with a reference in order to ensure that the
+ * dialog object, and any of its associated objects (e.g. transaction) are not
+ * untimely destroyed. For instance, that could happen when a transport error
+ * occurs.
+ *
+ * As long as the caller maintains a reference to the dialog there should be no
+ * worry that it might unknowningly be destroyed. However, once the caller unlocks
+ * the dialog there is a danger that some of the dialog's internal objects could
+ * be lost and/or compromised. For example, when the aforementioned transport error
+ * occurs the dialog's associated transaction gets destroyed (see pjsip_dlg_on_tsx_state
+ * in sip_dialog.c, and mod_inv_on_tsx_state in sip_inv.c).
+ *
+ * In this case and before using the dialog again the caller should re-lock the
+ * dialog, check to make sure the dialog is still established, and the transaction
+ * still exists and has not been destroyed.
+ *
+ * \param endpoint A pointer to the endpoint
+ * \param rdata The request that is starting the dialog
+ * \param[out] status On failure, the reason for failure in creating the dialog
+ *
+ * \retval A locked, and reference counted pjsip_dialog object.
+ * \retval NULL on failure
+ */
+pjsip_dialog *ast_sip_create_dialog_uas_locked(const struct ast_sip_endpoint *endpoint,
+ pjsip_rx_data *rdata, pj_status_t *status);
+
+/*!
* \brief General purpose method for creating an rdata structure using specific information
* \since 13.15.0
*
diff --git a/res/res_pjsip.c b/res/res_pjsip.c
index 93ac3ee..5137298 100644
--- a/res/res_pjsip.c
+++ b/res/res_pjsip.c
@@ -3463,7 +3463,11 @@
return 0;
}
-pjsip_dialog *ast_sip_create_dialog_uas(const struct ast_sip_endpoint *endpoint, pjsip_rx_data *rdata, pj_status_t *status)
+typedef pj_status_t (*create_dlg_uac)(pjsip_user_agent *ua, pjsip_rx_data *rdata,
+ const pj_str_t *contact, pjsip_dialog **p_dlg);
+
+static pjsip_dialog *create_dialog_uas(const struct ast_sip_endpoint *endpoint,
+ pjsip_rx_data *rdata, pj_status_t *status, create_dlg_uac create_fun)
{
pjsip_dialog *dlg;
pj_str_t contact;
@@ -3498,11 +3502,7 @@
(type != PJSIP_TRANSPORT_UDP && type != PJSIP_TRANSPORT_UDP6) ? ";transport=" : "",
(type != PJSIP_TRANSPORT_UDP && type != PJSIP_TRANSPORT_UDP6) ? pjsip_transport_get_type_name(type) : "");
-#ifdef HAVE_PJSIP_DLG_CREATE_UAS_AND_INC_LOCK
- *status = pjsip_dlg_create_uas_and_inc_lock(pjsip_ua_instance(), rdata, &contact, &dlg);
-#else
- *status = pjsip_dlg_create_uas(pjsip_ua_instance(), rdata, &contact, &dlg);
-#endif
+ *status = create_fun(pjsip_ua_instance(), rdata, &contact, &dlg);
if (*status != PJ_SUCCESS) {
char err[PJ_ERR_MSG_SIZE];
@@ -3515,13 +3515,48 @@
dlg->sess_count++;
pjsip_dlg_set_transport(dlg, &selector);
dlg->sess_count--;
-#ifdef HAVE_PJSIP_DLG_CREATE_UAS_AND_INC_LOCK
- pjsip_dlg_dec_lock(dlg);
-#endif
return dlg;
}
+pjsip_dialog *ast_sip_create_dialog_uas(const struct ast_sip_endpoint *endpoint, pjsip_rx_data *rdata, pj_status_t *status)
+{
+#ifdef HAVE_PJSIP_DLG_CREATE_UAS_AND_INC_LOCK
+ pjsip_dialog *dlg;
+
+ dlg = create_dialog_uas(endpoint, rdata, status, pjsip_dlg_create_uas_and_inc_lock);
+ if (dlg) {
+ pjsip_dlg_dec_lock(dlg);
+ }
+
+ return dlg;
+#else
+ return create_dialog_uas(endpoint, rdata, status, pjsip_dlg_create_uas);
+#endif
+}
+
+pjsip_dialog *ast_sip_create_dialog_uas_locked(const struct ast_sip_endpoint *endpoint,
+ pjsip_rx_data *rdata, pj_status_t *status)
+{
+#ifdef HAVE_PJSIP_DLG_CREATE_UAS_AND_INC_LOCK
+ return create_dialog_uas(endpoint, rdata, status, pjsip_dlg_create_uas_and_inc_lock);
+#else
+ /*
+ * This is put here in order to be compatible with older versions of pjproject.
+ * Best we can do in this case is immediately lock after getting the dialog.
+ * However, that does leave a "gap" between creating and locking.
+ */
+ pjsip_dialog *dlg;
+
+ dlg = create_dialog_uas(endpoint, rdata, status, pjsip_dlg_create_uas);
+ if (dlg) {
+ pjsip_dlg_inc_lock(dlg);
+ }
+
+ return dlg;
+#endif
+}
+
int ast_sip_create_rdata_with_contact(pjsip_rx_data *rdata, char *packet, const char *src_name, int src_port,
char *transport_type, const char *local_name, int local_port, const char *contact)
{
diff --git a/res/res_pjsip_pubsub.c b/res/res_pjsip_pubsub.c
index 49d4aeb..5545a27 100644
--- a/res/res_pjsip_pubsub.c
+++ b/res/res_pjsip_pubsub.c
@@ -1500,7 +1500,7 @@
}
sub_tree->role = AST_SIP_NOTIFIER;
- dlg = ast_sip_create_dialog_uas(endpoint, rdata, dlg_status);
+ dlg = ast_sip_create_dialog_uas_locked(endpoint, rdata, dlg_status);
if (!dlg) {
if (*dlg_status != PJ_EEXISTS) {
ast_log(LOG_WARNING, "Unable to create dialog for SIP subscription\n");
@@ -1521,8 +1521,16 @@
}
pjsip_evsub_create_uas(dlg, &pubsub_cb, rdata, 0, &sub_tree->evsub);
+
subscription_setup_dialog(sub_tree, dlg);
+ /*
+ * The evsub and subscription setup both add dialog refs, so the dialog ref that
+ * was added when the dialog was created (see ast_sip_create_dialog_uas_lock) can
+ * now be removed. The lock should no longer be needed so can be removed too.
+ */
+ pjsip_dlg_dec_lock(dlg);
+
#ifdef HAVE_PJSIP_EVSUB_GRP_LOCK
pjsip_evsub_add_ref(sub_tree->evsub);
#endif
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index bb2fcea..09693a0 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -2095,6 +2095,75 @@
return SIP_GET_DEST_EXTEN_NOT_FOUND;
}
+/*
+ * /internal
+ * /brief Process initial answer for an incoming invite
+ *
+ * This function should only be called during the setup, and handling of a
+ * new incoming invite. Most, if not all of the time, this will be called
+ * when an error occurs and we need to respond as such.
+ *
+ * When a SIP session termination code is given for the answer it's assumed
+ * this call then will be the final bit of processing before ending session
+ * setup. As such, we've been holding a lock, and a reference on the invite
+ * session's dialog. So before returning this function removes that reference,
+ * and unlocks the dialog.
+ *
+ * \param inv_session The session on which to answer
+ * \param rdata The original request
+ * \param answer_code The answer's numeric code
+ * \param terminate_code The termination code if the answer fails
+ * \param notify Whether or not to call on_state_changed
+ *
+ * \retval 0 if invite successfully answered, -1 if an error occurred
+ */
+static int new_invite_initial_answer(pjsip_inv_session *inv_session, pjsip_rx_data *rdata,
+ int answer_code, int terminate_code, pj_bool_t notify)
+{
+ pjsip_tx_data *tdata = NULL;
+ int res = 0;
+
+ if (inv_session->state != PJSIP_INV_STATE_DISCONNECTED) {
+ if (pjsip_inv_initial_answer(
+ inv_session, rdata, answer_code, NULL, NULL, &tdata) != PJ_SUCCESS) {
+
+ pjsip_inv_terminate(inv_session, terminate_code ? terminate_code : answer_code, notify);
+ res = -1;
+ } else {
+ pjsip_inv_send_msg(inv_session, tdata);
+ }
+ }
+
+ if (answer_code >= 300) {
+ /*
+ * A session is ending. The dialog has a reference that needs to be
+ * removed and holds a lock that needs to be unlocked before returning.
+ */
+ pjsip_dlg_dec_lock(inv_session->dlg);
+ }
+
+ return res;
+}
+
+/*
+ * /internal
+ * /brief Create and initialize a pjsip invite session
+
+ * pjsip_inv_session adds, and maintains a reference to the dialog upon a successful
+ * invite session creation until the session is destroyed. However, we'll wait to
+ * remove the reference that was added for the dialog when it gets created since we're
+ * not ready to unlock the dialog in this function.
+ *
+ * So, if this function successfully returns that means it returns with its newly
+ * created, and associated dialog locked and with two references (i.e. dialog's
+ * reference count should be 2).
+ *
+ * \param endpoint A pointer to the endpoint
+ * \param rdata The request that is starting the dialog
+ *
+ * \retval A pjsip invite session object
+ * \retval NULL on error
+ */
static pjsip_inv_session *pre_session_setup(pjsip_rx_data *rdata, const struct ast_sip_endpoint *endpoint)
{
pjsip_tx_data *tdata;
@@ -2113,15 +2182,28 @@
}
return NULL;
}
- dlg = ast_sip_create_dialog_uas(endpoint, rdata, &dlg_status);
+
+ dlg = ast_sip_create_dialog_uas_locked(endpoint, rdata, &dlg_status);
if (!dlg) {
if (dlg_status != PJ_EEXISTS) {
pjsip_endpt_respond_stateless(ast_sip_get_pjsip_endpoint(), rdata, 500, NULL, NULL, NULL);
}
return NULL;
}
+
+ /*
+ * The returned dialog holds a lock and has a reference added. Any paths where the
+ * dialog invite session is not returned must unlock the dialog and remove its reference.
+ */
+
if (pjsip_inv_create_uas(dlg, rdata, NULL, options, &inv_session) != PJ_SUCCESS) {
pjsip_endpt_respond_stateless(ast_sip_get_pjsip_endpoint(), rdata, 500, NULL, NULL, NULL);
+ /*
+ * The acquired dialog holds a lock, and a reference. Since the dialog is not
+ * going to be returned here it must first be unlocked and de-referenced. This
+ * must be done prior to calling dialog termination.
+ */
+ pjsip_dlg_dec_lock(dlg);
pjsip_dlg_terminate(dlg);
return NULL;
}
@@ -2130,12 +2212,13 @@
inv_session->sdp_neg_flags = PJMEDIA_SDP_NEG_ALLOW_MEDIA_CHANGE;
#endif
if (pjsip_dlg_add_usage(dlg, &session_module, NULL) != PJ_SUCCESS) {
- if (pjsip_inv_initial_answer(inv_session, rdata, 500, NULL, NULL, &tdata) != PJ_SUCCESS) {
- pjsip_inv_terminate(inv_session, 500, PJ_FALSE);
- }
- pjsip_inv_send_msg(inv_session, tdata);
+ /* Dialog's lock and a reference are removed in new_invite_initial_answer */
+ new_invite_initial_answer(inv_session, rdata, 500, 500, PJ_FALSE);
+ /* Remove 2nd reference added at inv_session creation */
+ pjsip_dlg_dec_session(inv_session->dlg, &session_module);
return NULL;
}
+
return inv_session;
}
@@ -2337,7 +2420,6 @@
{
RAII_VAR(struct ast_sip_endpoint *, endpoint,
ast_pjsip_rdata_get_endpoint(rdata), ao2_cleanup);
- pjsip_tx_data *tdata = NULL;
pjsip_inv_session *inv_session = NULL;
struct ast_sip_session *session;
struct new_invite invite;
@@ -2350,27 +2432,48 @@
return;
}
+ /*
+ * Upon a successful pre_session_setup the associated dialog is returned locked
+ * and with an added reference. Well actually two references. One added when the
+ * dialog itself was created, and another added when the pjsip invite session was
+ * created and the dialog was added to it.
+ *
+ * In order to ensure the dialog's, and any of its internal attributes, lifetimes
+ * we'll hold the lock and maintain the reference throughout the entire new invite
+ * handling process. See ast_sip_create_dialog_uas_locked for more details but,
+ * basically we do this to make sure a transport failure does not destroy the dialog
+ * and/or transaction out from underneath us between pjsip calls. Alternatively, we
+ * could probably release the lock if we needed to, but then we'd have to re-lock and
+ * check the dialog and transaction prior to every pjsip call.
+ *
+ * That means any off nominal/failure paths in this function must remove the associated
+ * dialog reference added at dialog creation, and remove the lock. As well the
+ * referenced pjsip invite session must be "cleaned up", which should also then
+ * remove its reference to the dialog at that time.
+ *
+ * Nominally we'll unlock the dialog, and release the reference when all new invite
+ * process handling has successfully completed.
+ */
+
#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);
- }
+ /* Dialog's lock and a reference are removed in new_invite_initial_answer */
+ if (!new_invite_initial_answer(inv_session, rdata, 500, 500, PJ_FALSE)) {
+ /* Terminate the session if it wasn't done in the answer */
+ pjsip_inv_terminate(inv_session, 500, PJ_FALSE);
}
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) {
+ /* Dialog's lock and reference are removed in new_invite_initial_answer */
+ if (!new_invite_initial_answer(inv_session, rdata, 500, 500, PJ_FALSE)) {
+ /* Terminate the session if it wasn't done in the answer */
pjsip_inv_terminate(inv_session, 500, PJ_FALSE);
- } else {
- pjsip_inv_send_msg(inv_session, tdata);
}
+
#ifdef HAVE_PJSIP_INV_SESSION_REF
pjsip_inv_dec_ref(inv_session);
#endif
@@ -2388,6 +2491,17 @@
invite.rdata = rdata;
new_invite(&invite);
+ /*
+ * The dialog lock and reference added at dialog creation time must be
+ * maintained throughout the new invite process. Since we're pretty much
+ * done at this point with things it's safe to go ahead and remove the lock
+ * and the reference here. See ast_sip_create_dialog_uas_locked for more info.
+ *
+ * Note, any future functionality added that does work using the dialog must
+ * be done before this.
+ */
+ pjsip_dlg_dec_lock(inv_session->dlg);
+
ao2_ref(session, -1);
}
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15162
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I5ef645a47829596f402cf383dc02c629c618969e
Gerrit-Change-Number: 15162
Gerrit-PatchSet: 2
Gerrit-Owner: Friendly Automation
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20201105/c996a4b6/attachment-0001.html>
More information about the asterisk-code-review
mailing list