[Asterisk-code-review] res pjsip session: Change some asserts to warning/debug mes... (asterisk[15.0])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/6549 )

Change subject: res_pjsip_session:  Change some asserts to warning/debug messages
......................................................................

res_pjsip_session:  Change some asserts to warning/debug messages

There was an issue reported where an SDP received on a 183 Session
Progress message caused a crash because the pending streams had
already been processed when the OK was received.  In that case the
pending topology was legitimately NULL.  There was an assert for an
incorrect number of streams in the topology but not one for
topology being NULL.  In any case, if you're not in dev-mode the
asserts don't do anything and since the scenario is legit, the
asserts weren't appropriate anyway.

* Changed several asserts to warning or debug messages and return
codes as appropriate.

ASTERISK-27264
Reported by: Daniel Heckl

Change-Id: I58daaa9d2938fa980857ab3ec41925ab5ff9c848
---
M res/res_pjsip_session.c
1 file changed, 29 insertions(+), 12 deletions(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve; Approved for Submit
  Kevin Harwell: Looks good to me, approved



diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index b6fa8bb..de1a10d 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -277,7 +277,11 @@
 {
 	int index;
 
-	ast_assert(session->pending_media_state->topology != NULL);
+	if (!session->pending_media_state->topology) {
+		ast_log(LOG_WARNING, "Pending topology was NULL for channel '%s'\n",
+			session->channel ? ast_channel_name(session->channel) : "unknown");
+		return 0;
+	}
 
 	if (ast_stream_get_state(stream) == AST_STREAM_STATE_REMOVED) {
 		return 0;
@@ -766,6 +770,30 @@
 	int i;
 	struct ast_stream_topology *topology;
 
+	/* This situation can legitimately happen when an SDP is received in a
+	 * 183 Session Progress message.  In that case, everything's been done
+	 * by the time this function is called and there are no more pending
+	 * streams.
+	 */
+	if (!session->pending_media_state->topology) {
+		ast_debug(1, "Pending topology was NULL for channel '%s'\n",
+			session->channel ? ast_channel_name(session->channel) : "unknown");
+		return 0;
+	}
+
+	/* If we're handling negotiated streams, then we should already have set
+	 * up session media instances (and Asterisk streams) that correspond to
+	 * the local SDP, and there should be the same number of session medias
+	 * and streams as there are local SDP streams
+	 */
+	if (ast_stream_topology_get_count(session->pending_media_state->topology) != local->media_count
+		|| AST_VECTOR_SIZE(&session->pending_media_state->sessions) != local->media_count) {
+		ast_log(LOG_WARNING, "Local SDP for channel '%s' contains %d media streams while we expected it to contain %u\n",
+			session->channel ? ast_channel_name(session->channel) : "unknown",
+			ast_stream_topology_get_count(session->pending_media_state->topology), local->media_count);
+		return -1;
+	}
+
 	for (i = 0; i < local->media_count; ++i) {
 		struct ast_sip_session_media *session_media;
 		struct ast_stream *stream;
@@ -773,14 +801,6 @@
 		if (!remote->media[i]) {
 			continue;
 		}
-
-		/* If we're handling negotiated streams, then we should already have set
-		 * up session media instances (and Asterisk streams) that correspond to
-		 * the local SDP, and there should be the same number of session medias
-		 * and streams as there are local SDP streams
-		 */
-		ast_assert(i < AST_VECTOR_SIZE(&session->pending_media_state->sessions));
-		ast_assert(i < ast_stream_topology_get_count(session->pending_media_state->topology));
 
 		session_media = AST_VECTOR_GET(&session->pending_media_state->sessions, i);
 		stream = ast_stream_topology_get_stream(session->pending_media_state->topology, i);
@@ -815,9 +835,6 @@
 		if (!remote->media[i]) {
 			continue;
 		}
-
-		ast_assert(i < AST_VECTOR_SIZE(&session->pending_media_state->sessions));
-		ast_assert(i < ast_stream_topology_get_count(session->pending_media_state->topology));
 
 		session_media = AST_VECTOR_GET(&session->pending_media_state->sessions, i);
 		stream = ast_stream_topology_get_stream(session->pending_media_state->topology, i);

-- 
To view, visit https://gerrit.asterisk.org/6549
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15.0
Gerrit-MessageType: merged
Gerrit-Change-Id: I58daaa9d2938fa980857ab3ec41925ab5ff9c848
Gerrit-Change-Number: 6549
Gerrit-PatchSet: 3
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170922/e5dd5310/attachment.html>