res_pjsip_sdp_rtp: Fix intermittent call drop on reINVITE sdp change

A reINVITE that changed the offered codec set (e.g. ulaw+alaw -> alaw
only) could lead to call teardown when a bridge write still using the
old format occurred during the re-negotiation.

This change avoids the termination by making the following changes.

First, Asterisk now holds the lock across both the tx payload update
and the channel format update so they commit atomically.

Second, when processing the sdp on the new incoming offer, merge the
new offer with the old until the negotiation is complete.

Also adds unit tests for the sdp merge

Fixes: #2059

diff --git a/include/asterisk/rtp_engine.h b/include/asterisk/rtp_engine.h
index eab55ec33ae8f04396e795d258595491565d4e38..38af5acb8ec668776b3abec1af2f747bb2c76569 100644 (file)
--- a/include/asterisk/rtp_engine.h
+++ b/include/asterisk/rtp_engine.h
@@ -1593,6 +1593,32 @@ void ast_rtp_codecs_payloads_clear(struct ast_rtp_codecs *codecs, struct ast_rtp
  */
 void ast_rtp_codecs_payloads_copy(struct ast_rtp_codecs *src, struct ast_rtp_codecs *dest, struct ast_rtp_instance *instance);
 
+/*!
+ * \brief Merge payload information from one RTP instance into another
+ * \since 23.5.0
+ * \since 22.11.0
+ * \since 20.21.0
+ *
+ * \param src The source codecs structure
+ * \param dest The destination codecs structure that the values from src will be merged into
+ * \param instance Optionally the instance that the dst codecs structure belongs to
+ *
+ * Example usage:
+ *
+ * \code
+ * ast_rtp_codecs_payloads_merge(&codecs0, &codecs1, NULL);
+ * \endcode
+ *
+ * This copies codecs0 payload mappings into codecs1 without clearing codecs1 first: payloads present in
+ * codecs0 overwrite matching payload-number slots in codecs1, while payloads absent from codecs0 remain
+ * unchanged.
+ *
+ * This updates the codecs1 framing, but does not replace codecs1 preferred format or
+ * preferred DTMF metadata.
+ *
+ */
+void ast_rtp_codecs_payloads_merge(struct ast_rtp_codecs *src, struct ast_rtp_codecs *dest, struct ast_rtp_instance *instance);
+
 /*!
  * \brief Crossover copy the tx payload mapping of src to the rx payload mapping of dest.
  * \since 14.0.0

diff --git a/main/rtp_engine.c b/main/rtp_engine.c
index 07b560184f25bbf95b51d7708c217413324dc147..13ba5b44b364ff5c0761fd006e215ac9870cc815 100644 (file)
--- a/main/rtp_engine.c
+++ b/main/rtp_engine.c
@@ -1335,6 +1335,34 @@ void ast_rtp_codecs_payloads_copy(struct ast_rtp_codecs *src, struct ast_rtp_cod
        ast_rwlock_unlock(&dest->codecs_lock);
 }
 
+void ast_rtp_codecs_payloads_merge(struct ast_rtp_codecs *src, struct ast_rtp_codecs *dest, struct ast_rtp_instance *instance)
+{
+       ast_rwlock_wrlock(&dest->codecs_lock);
+
+       /* Deadlock avoidance because of held write lock. */
+       while (ast_rwlock_tryrdlock(&src->codecs_lock)) {
+               ast_rwlock_unlock(&dest->codecs_lock);
+               sched_yield();
+               ast_rwlock_wrlock(&dest->codecs_lock);
+       }
+
+       /*
+        * Unlike ast_rtp_codecs_payloads_copy(), do not clear destination tx mappings first.
+        * For each tx payload index present in src, copy it into dest (replacing dest at that
+        * same index). Any destination tx mappings at indexes not present in src are retained.
+        *
+        * This also refreshes rx mappings from src and updates dest framing, but intentionally
+        * preserves dest preferred format and preferred DTMF settings.
+        */
+
+       rtp_codecs_payloads_copy_rx(src, dest, instance);
+       rtp_codecs_payloads_copy_tx(src, dest, instance);
+       dest->framing = src->framing;
+
+       ast_rwlock_unlock(&src->codecs_lock);
+       ast_rwlock_unlock(&dest->codecs_lock);
+}
+
 void ast_rtp_codecs_payloads_xover(struct ast_rtp_codecs *src, struct ast_rtp_codecs *dest, struct ast_rtp_instance *instance)
 {
        int idx;

diff --git a/res/res_pjsip_sdp_rtp.c b/res/res_pjsip_sdp_rtp.c
index 635b0827d288823536ae4b156d6129fe5c4788ac..1b0b820e2f7d1cb6268051a8d698edb2d1fc8d3d 100644 (file)
--- a/res/res_pjsip_sdp_rtp.c
+++ b/res/res_pjsip_sdp_rtp.c
@@ -509,7 +509,16 @@ static struct ast_format_cap *set_incoming_call_offer_cap(
         */
        ast_rtp_codecs_payloads_xover(&codecs, &codecs, NULL);
 
-       ast_rtp_codecs_payloads_copy(&codecs,
+       /*
+        * This only merges the newly offered payloads into the live RTP instance
+        * rather than doing a destructive replace, so any format the channel is
+        * still actively sending in (e.g. while a reINVITE is in flight) remains
+        * usable right up until set_caps() commits the channel to the new
+        * negotiated format. A destructive replace here, before the channel's
+        * own read/write format has caught up, is what let a concurrent bridge
+        * write briefly land on a payload mapping that no longer had its format.
+        */
+       ast_rtp_codecs_payloads_merge(&codecs,
                ast_rtp_instance_get_codecs(session_media->rtp), session_media->rtp);
 
        ast_rtp_codecs_payloads_destroy(&codecs);
@@ -581,13 +590,17 @@ static int set_caps(struct ast_sip_session *session,
                 */
                ast_rtp_codecs_payloads_xover(&codecs, &codecs, NULL);
        }
+
+       if (session->channel) {
+               ast_channel_lock(session->channel);
+       }
+
        ast_rtp_codecs_payloads_copy(&codecs, ast_rtp_instance_get_codecs(session_media->rtp),
                session_media->rtp);
 
        apply_cap_to_bundled(session_media, session_media_transport, asterisk_stream, joint);
 
        if (session->channel && ast_sip_session_is_pending_stream_default(session, asterisk_stream)) {
-               ast_channel_lock(session->channel);
                ast_format_cap_remove_by_type(caps, AST_MEDIA_TYPE_UNKNOWN);
                ast_format_cap_append_from_cap(caps, ast_channel_nativeformats(session->channel),
                        AST_MEDIA_TYPE_UNKNOWN);
@@ -641,7 +654,9 @@ static int set_caps(struct ast_sip_session *session,
                if (ast_channel_is_bridged(session->channel)) {
                        ast_channel_set_unbridged_nolock(session->channel, 1);
                }
+       }
 
+       if (session->channel) {
                ast_channel_unlock(session->channel);
        }
 

diff --git a/tests/test_res_rtp.c b/tests/test_res_rtp.c
index 2ecf3833636d05d29e0e1228d7be12b8eb7c265b..46dad0932ead43ccda9d06372df492fcf40bd53a 100644 (file)
--- a/tests/test_res_rtp.c
+++ b/tests/test_res_rtp.c
@@ -690,8 +690,159 @@ AST_TEST_DEFINE(mes)
        return AST_TEST_PASS;
 }
 
+AST_TEST_DEFINE(payload_merge_abandoned_offer)
+{
+       struct ast_rtp_codecs offered = AST_RTP_CODECS_NULL_INIT;
+       struct ast_rtp_codecs active = AST_RTP_CODECS_NULL_INIT;
+       struct ast_rtp_payload_type *payload;
+       enum ast_test_result_state result = AST_TEST_PASS;
+       int offered_initialized = 0;
+       int active_initialized = 0;
+
+       switch (cmd) {
+       case TEST_INIT:
+               info->name = "payload_merge_abandoned_offer";
+               info->category = "/res/res_rtp/";
+               info->summary = "Verify mappings retained after an uncommitted payload merge";
+               info->description =
+                       "Characterizes the state left behind when an SDP offer's payloads "
+                       "are merged into an active RTP mapping but no later copy commits "
+                       "or rolls back that offer.";
+               return AST_TEST_NOT_RUN;
+       case TEST_EXECUTE:
+               break;
+       }
+
+       if (ast_rtp_codecs_payloads_initialize(&offered)) {
+               ast_test_status_update(test, "Unable to initialize RTP codecs\n");
+               result = AST_TEST_FAIL;
+               goto cleanup;
+       }
+       offered_initialized = 1;
+       if (ast_rtp_codecs_payloads_initialize(&active)) {
+               ast_test_status_update(test, "Unable to initialize RTP codecs\n");
+               result = AST_TEST_FAIL;
+               goto cleanup;
+       }
+       active_initialized = 1;
+
+       /* Use dynamic PTs so lookup cannot fall back to the global static table. */
+       if (ast_rtp_codecs_payloads_set_rtpmap_type(
+                       &active, NULL, 96, "audio", "PCMU", 0)
+               || ast_rtp_codecs_payloads_set_rtpmap_type(
+                       &offered, NULL, 97, "audio", "PCMA", 0)) {
+               ast_test_status_update(test, "Unable to create dynamic RTP mappings\n");
+               result = AST_TEST_FAIL;
+               goto cleanup;
+       }
+       ast_rtp_codecs_payloads_xover(&active, &active, NULL);
+       ast_rtp_codecs_payloads_xover(&offered, &offered, NULL);
+
+       ast_rtp_codecs_payloads_merge(&offered, &active, NULL);
+
+       payload = ast_rtp_codecs_get_payload(&active, 96);
+       if (!payload || !payload->asterisk_format
+               || ast_format_cmp(payload->format, ast_format_ulaw) != AST_FORMAT_CMP_EQUAL) {
+               ast_test_status_update(test, "Active PCMU payload 96 was not retained by merge\n");
+               result = AST_TEST_FAIL;
+       }
+       ao2_cleanup(payload);
+
+       payload = ast_rtp_codecs_get_payload(&active, 97);
+       if (!payload || !payload->asterisk_format
+               || ast_format_cmp(payload->format, ast_format_alaw) != AST_FORMAT_CMP_EQUAL) {
+               ast_test_status_update(test, "Offered PCMA payload 97 was not retained after merge\n");
+               result = AST_TEST_FAIL;
+       }
+       ao2_cleanup(payload);
+
+cleanup:
+       if (active_initialized) {
+               ast_rtp_codecs_payloads_destroy(&active);
+       }
+       if (offered_initialized) {
+               ast_rtp_codecs_payloads_destroy(&offered);
+       }
+       return result;
+}
+
+AST_TEST_DEFINE(payload_merge_preserves_preferences)
+{
+       struct ast_rtp_codecs offered = AST_RTP_CODECS_NULL_INIT;
+       struct ast_rtp_codecs active = AST_RTP_CODECS_NULL_INIT;
+       RAII_VAR(struct ast_format *, preferred, NULL, ao2_cleanup);
+       enum ast_test_result_state result = AST_TEST_PASS;
+       int offered_initialized = 0;
+       int active_initialized = 0;
+
+       switch (cmd) {
+       case TEST_INIT:
+               info->name = "payload_merge_preserves_preferences";
+               info->category = "/res/res_rtp/";
+               info->summary = "Verify a payload merge preserves active preferences";
+               info->description =
+                       "Verifies that merging an SDP offer's provisional payload mappings "
+                       "updates framing needed to generate the SDP answer without changing "
+                       "the active preferred format or preferred DTMF metadata before the "
+                       "offer is committed.";
+               return AST_TEST_NOT_RUN;
+       case TEST_EXECUTE:
+               break;
+       }
+
+       if (ast_rtp_codecs_payloads_initialize(&offered)) {
+               ast_test_status_update(test, "Unable to initialize RTP codecs\n");
+               result = AST_TEST_FAIL;
+               goto cleanup;
+       }
+       offered_initialized = 1;
+       if (ast_rtp_codecs_payloads_initialize(&active)) {
+               ast_test_status_update(test, "Unable to initialize RTP codecs\n");
+               result = AST_TEST_FAIL;
+               goto cleanup;
+       }
+       active_initialized = 1;
+
+       ast_rtp_codecs_set_preferred_format(&active, ast_format_ulaw);
+       ast_rtp_codecs_set_preferred_dtmf_format(&active, 101, 8000);
+       ast_rtp_codecs_set_framing(&active, 20);
+       ast_rtp_codecs_set_preferred_format(&offered, ast_format_alaw);
+       ast_rtp_codecs_set_preferred_dtmf_format(&offered, 110, 16000);
+       ast_rtp_codecs_set_framing(&offered, 30);
+
+       ast_rtp_codecs_payloads_merge(&offered, &active, NULL);
+
+       preferred = ast_rtp_codecs_get_preferred_format(&active);
+       if (!preferred
+               || ast_format_cmp(preferred, ast_format_ulaw) != AST_FORMAT_CMP_EQUAL) {
+               ast_test_status_update(test, "Payload merge changed the active preferred format\n");
+               result = AST_TEST_FAIL;
+       }
+       if (ast_rtp_codecs_get_preferred_dtmf_format_pt(&active) != 101
+               || ast_rtp_codecs_get_preferred_dtmf_format_rate(&active) != 8000) {
+               ast_test_status_update(test,
+                       "Payload merge changed the active preferred DTMF metadata\n");
+               result = AST_TEST_FAIL;
+       }
+       if (ast_rtp_codecs_get_framing(&active) != 30) {
+               ast_test_status_update(test, "Payload merge did not update offered framing\n");
+               result = AST_TEST_FAIL;
+       }
+
+cleanup:
+       if (active_initialized) {
+               ast_rtp_codecs_payloads_destroy(&active);
+       }
+       if (offered_initialized) {
+               ast_rtp_codecs_payloads_destroy(&offered);
+       }
+       return result;
+}
+
 static int unload_module(void)
 {
+       AST_TEST_UNREGISTER(payload_merge_preserves_preferences);
+       AST_TEST_UNREGISTER(payload_merge_abandoned_offer);
        AST_TEST_UNREGISTER(mes);
        AST_TEST_UNREGISTER(nack_no_packet_loss);
        AST_TEST_UNREGISTER(nack_nominal);
@@ -713,6 +864,8 @@ static int load_module(void)
        AST_TEST_REGISTER(sr_rr_nominal);
        AST_TEST_REGISTER(fir_nominal);
        AST_TEST_REGISTER(mes);
+       AST_TEST_REGISTER(payload_merge_abandoned_offer);
+       AST_TEST_REGISTER(payload_merge_preserves_preferences);
        return AST_MODULE_LOAD_SUCCESS;
 }