AST-2019-004 - res_pjsip_t38.c: Add NULL checks before using session media

After receiving a 200 OK with a declined stream in response to a T.38
initiated re-invite Asterisk would crash when attempting to dereference
a NULL session media object.

This patch checks to make sure the session media object is not NULL before
attempting to use it.

ASTERISK-28495
patches:
  ast-2019-004.patch submitted by Alexei Gradinari (license 5691)

Change-Id: I168f45f4da29cfe739acf87e597baa2aae7aa572

channels/chan_sip.c

@@ -10921,7 +10921,13 @@ static int process_sdp(struct sip_pvt *p, struct sip_request *req, int t38action
 			    ast_rtp_lookup_mime_multiple2(s3, NULL, newnoncodeccapability, 0, 0));
 	}
 
-	if (portno != -1 || vportno != -1 || tportno != -1) {
+	/* When UDPTL is negotiated it is expected that there are no compatible codecs as audio or
+	 * video is not being transported, thus we continue in this function further up if that is
+	 * the case. If we receive an SDP answer containing both a UDPTL stream and another media
+	 * stream however we need to check again to ensure that there is at least one joint codec
+	 * instead of assuming there is one.
+	 */
+	if ((portno != -1 || vportno != -1 || tportno != -1) && ast_format_cap_count(newjointcapability)) {
 		/* We are now ready to change the sip session and RTP structures with the offered codecs, since
 		   they are acceptable */
 		unsigned int framing;

main/stasis_cache.c

@@ -825,16 +825,29 @@ static void caching_topic_exec(void *data, struct stasis_subscription *sub,
 
 	msg_type = stasis_message_type(message);
 
-	/*
-	 * app_voicemail used to rely on the cache containing every topic subscribe and
-	 * unsubscribe in order to determine if anyone was currently subscribed to a
-	 * particular mailbox.  This caused the cache to grow unabated for the life of
-	 * the asterisk instance.  Since it no longer needs the cache of these message
-	 * types, and no other function needs them, we no longer cache them.
-	 */
 	if (stasis_subscription_change_type() == msg_type) {
-		ao2_cleanup(caching_topic_needs_unref);
-		return;
+		struct stasis_subscription_change *change = stasis_message_data(message);
+
+		/*
+		 * If this change type is an unsubscribe, we need to find the original
+		 * subscribe and remove it from the cache otherwise the cache will
+		 * continue to grow unabated.
+		 */
+		if (strcmp(change->description, "Unsubscribe") == 0) {
+			struct stasis_cache_entry *sub;
+
+			ao2_wrlock(caching_topic->cache->entries);
+			sub = cache_find(caching_topic->cache->entries, stasis_subscription_change_type(), change->uniqueid);
+			if (sub) {
+				cache_remove(caching_topic->cache->entries, sub, stasis_message_eid(message));
+				ao2_cleanup(sub);
+			}
+			ao2_unlock(caching_topic->cache->entries);
+			ao2_cleanup(caching_topic_needs_unref);
+			return;
+		}
+		msg_put = message;
+		msg = message;
 	} else if (stasis_cache_clear_type() == msg_type) {
 		/* Cache clear event. */
 		msg_put = NULL;

res/res_pjsip_messaging.c

@@ -91,10 +91,13 @@ static enum pjsip_status_code check_content_type_in_dialog(const pjsip_rx_data *
 	static const pj_str_t text = { "text", 4};
 	static const pj_str_t application = { "application", 11};
 
+	if (!(rdata->msg_info.msg->body && rdata->msg_info.msg->body->len > 0)) {
+		return res;
+	}
+
 	/* We'll accept any text/ or application/ content type */
-	if (rdata->msg_info.msg->body && rdata->msg_info.msg->body->len
-		&& (pj_stricmp(&rdata->msg_info.msg->body->content_type.type, &text) == 0
-			|| pj_stricmp(&rdata->msg_info.msg->body->content_type.type, &application) == 0)) {
+	if (pj_stricmp(&rdata->msg_info.msg->body->content_type.type, &text) == 0
+			|| pj_stricmp(&rdata->msg_info.msg->body->content_type.type, &application) == 0) {
 		res = PJSIP_SC_OK;
 	} else if (rdata->msg_info.ctype
 		&& (pj_stricmp(&rdata->msg_info.ctype->media.type, &text) == 0

res/res_pjsip_sdp_rtp.c

@@ -1941,7 +1941,7 @@ static int apply_negotiated_sdp_stream(struct ast_sip_session *session,
 	}
 
 	if (set_caps(session, session_media, session_media_transport, remote_stream, 0, asterisk_stream)) {
-		return 1;
+		return -1;
 	}
 
 	/* Set the channel uniqueid on the RTP instance now that it is becoming active */

res/res_pjsip_t38.c

@@ -203,7 +203,6 @@ static int t38_automatic_reject(void *obj)
 {
 	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);
-	struct ast_sip_session_media *session_media;
 
 	if (!datastore) {
 		return 0;
@@ -212,8 +211,7 @@ static int t38_automatic_reject(void *obj)
 	ast_debug(2, "Automatically rejecting T.38 request on channel '%s'\n",
 		session->channel ? ast_channel_name(session->channel) : "<gone>");
 
-	session_media = session->pending_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
-	t38_change_state(session, session_media, datastore->data, T38_REJECTED);
+	t38_change_state(session, NULL, datastore->data, T38_REJECTED);
 	ast_sip_session_resume_reinvite(session);
 
 	return 0;
@@ -322,28 +320,37 @@ static int t38_reinvite_response_cb(struct ast_sip_session *session, pjsip_rx_da
 		int index;
 
 		session_media = session->active_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
-		t38_change_state(session, session_media, state, T38_ENABLED);
+		if (!session_media) {
+			ast_log(LOG_WARNING, "Received %d response to T.38 re-invite on '%s' but no active session media\n",
+					status.code, session->channel ? ast_channel_name(session->channel) : "unknown channel");
+		} else {
+			t38_change_state(session, session_media, state, T38_ENABLED);
 
-		/* Stop all the streams in the stored away active state, they'll go back to being active once
-		 * we reinvite back.
-		 */
-		for (index = 0; index < AST_VECTOR_SIZE(&state->media_state->sessions); ++index) {
-			struct ast_sip_session_media *session_media = AST_VECTOR_GET(&state->media_state->sessions, index);
+			/* Stop all the streams in the stored away active state, they'll go back to being active once
+			 * we reinvite back.
+			 */
+			for (index = 0; index < AST_VECTOR_SIZE(&state->media_state->sessions); ++index) {
+				struct ast_sip_session_media *session_media = AST_VECTOR_GET(&state->media_state->sessions, index);
 
-			if (session_media && session_media->handler && session_media->handler->stream_stop) {
-				session_media->handler->stream_stop(session_media);
+				if (session_media && session_media->handler && session_media->handler->stream_stop) {
+					session_media->handler->stream_stop(session_media);
+				}
 			}
+
+			return 0;
 		}
 	} else {
 		session_media = session->pending_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
-		t38_change_state(session, session_media, state, T38_REJECTED);
-
-		/* Abort this attempt at switching to T.38 by resetting the pending state and freeing our stored away active state */
-		ast_sip_session_media_state_free(state->media_state);
-		state->media_state = NULL;
-		ast_sip_session_media_state_reset(session->pending_media_state);
 	}
 
+	/* If no session_media then response contained a declined stream, so disable */
+	t38_change_state(session, NULL, state, session_media ? T38_REJECTED : T38_DISABLED);
+
+	/* Abort this attempt at switching to T.38 by resetting the pending state and freeing our stored away active state */
+	ast_sip_session_media_state_free(state->media_state);
+	state->media_state = NULL;
+	ast_sip_session_media_state_reset(session->pending_media_state);
+
 	return 0;
 }
 
@@ -426,12 +433,10 @@ static int t38_interpret_parameters(void *obj)
 		/* Negotiation can not take place without a valid max_ifp value. */
 		if (!parameters->max_ifp) {
 			if (data->session->t38state == T38_PEER_REINVITE) {
-				session_media = data->session->pending_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
-				t38_change_state(data->session, session_media, state, T38_REJECTED);
+				t38_change_state(data->session, NULL, state, T38_REJECTED);
 				ast_sip_session_resume_reinvite(data->session);
 			} else if (data->session->t38state == T38_ENABLED) {
-				session_media = data->session->active_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
-				t38_change_state(data->session, session_media, state, T38_DISABLED);
+				t38_change_state(data->session, NULL, state, T38_DISABLED);
 				ast_sip_session_refresh(data->session, NULL, NULL, NULL,
 					AST_SIP_SESSION_REFRESH_METHOD_INVITE, 1, state->media_state);
 				state->media_state = NULL;
@@ -454,6 +459,11 @@ static int t38_interpret_parameters(void *obj)
 			state->our_parms.version = MIN(state->our_parms.version, state->their_parms.version);
 			state->our_parms.rate_management = state->their_parms.rate_management;
 			session_media = data->session->pending_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
+			if (!session_media) {
+				ast_log(LOG_ERROR, "Failed to negotiate parameters for reinvite on channel '%s' (No pending session media).\n",
+					data->session->channel ? ast_channel_name(data->session->channel) : "unknown channel");
+				break;
+			}
 			ast_udptl_set_local_max_ifp(session_media->udptl, state->our_parms.max_ifp);
 			t38_change_state(data->session, session_media, state, T38_ENABLED);
 			ast_sip_session_resume_reinvite(data->session);
@@ -468,8 +478,13 @@ static int t38_interpret_parameters(void *obj)
 			}
 			state->our_parms = *parameters;
 			session_media = media_state->default_session[AST_MEDIA_TYPE_IMAGE];
+			if (!session_media) {
+				ast_log(LOG_ERROR, "Failed to negotiate parameters on channel '%s' (No default session media).\n",
+					data->session->channel ? ast_channel_name(data->session->channel) : "unknown channel");
+				break;
+			}
 			ast_udptl_set_local_max_ifp(session_media->udptl, state->our_parms.max_ifp);
-			t38_change_state(data->session, session_media, state, T38_LOCAL_REINVITE);
+			t38_change_state(data->session, NULL, state, T38_LOCAL_REINVITE);
 			ast_sip_session_refresh(data->session, NULL, t38_reinvite_sdp_cb, t38_reinvite_response_cb,
 				AST_SIP_SESSION_REFRESH_METHOD_INVITE, 1, media_state);
 		}
@@ -478,12 +493,10 @@ static int t38_interpret_parameters(void *obj)
 	case AST_T38_REFUSED:
 	case AST_T38_REQUEST_TERMINATE:         /* Shutdown T38 */
 		if (data->session->t38state == T38_PEER_REINVITE) {
-			session_media = data->session->pending_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
-			t38_change_state(data->session, session_media, state, T38_REJECTED);
+			t38_change_state(data->session, NULL, state, T38_REJECTED);
 			ast_sip_session_resume_reinvite(data->session);
 		} else if (data->session->t38state == T38_ENABLED) {
-			session_media = data->session->active_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
-			t38_change_state(data->session, session_media, state, T38_DISABLED);
+			t38_change_state(data->session, NULL, state, T38_DISABLED);
 			ast_sip_session_refresh(data->session, NULL, NULL, NULL, AST_SIP_SESSION_REFRESH_METHOD_INVITE, 1, state->media_state);
 			state->media_state = NULL;
 		}
@@ -493,6 +506,11 @@ static int t38_interpret_parameters(void *obj)
 
 		if (data->session->t38state == T38_PEER_REINVITE) {
 			session_media = data->session->pending_media_state->default_session[AST_MEDIA_TYPE_IMAGE];
+			if (!session_media) {
+				ast_log(LOG_ERROR, "Failed to request parameters for reinvite on channel '%s' (No pending session media).\n",
+					data->session->channel ? ast_channel_name(data->session->channel) : "unknown channel");
+				break;
+			}
 			parameters.max_ifp = ast_udptl_get_far_max_ifp(session_media->udptl);
 			parameters.request_response = AST_T38_REQUEST_NEGOTIATE;
 			ast_queue_control_data(data->session->channel, AST_CONTROL_T38_PARAMETERS, &parameters, sizeof(parameters));
@@ -788,7 +806,7 @@ static int negotiate_incoming_sdp_stream(struct ast_sip_session *session,
 
 	if ((session->t38state == T38_REJECTED) || (session->t38state == T38_DISABLED)) {
 		ast_debug(3, "Declining; T.38 state is rejected or declined\n");
-		t38_change_state(session, session_media, state, T38_DISABLED);
+		t38_change_state(session, NULL, state, T38_DISABLED);
 		return 0;
 	}
 

tests/CI/gates.jenkinsfile

@@ -67,7 +67,7 @@ pipeline {
 
 					stage ("Checkout") {
 						sh "sudo chown -R jenkins:users ."  
-						env.GERRIT_PROJECT_URL = env.GERRIT_CHANGE_URL.replaceAll(/\/[0-9]+$/, "/${env.GERRIT_PROJECT}")
+						env.GERRIT_PROJECT_URL = env.GIT_URL.replaceAll(/[^\/]+$/, env.GERRIT_PROJECT)
 					
 						/*
 						 * Jenkins has already automatically checked out the base branch
@@ -90,10 +90,10 @@ pipeline {
 							checkout scm: [$class: 'GitSCM',
 								branches: [[name: env.GERRIT_BRANCH ]],
 								extensions: [
-									[$class: 'ScmName', name: 'gerrit-public'],
+									[$class: 'ScmName', name: env.GERRIT_NAME],
 									[$class: 'CleanBeforeCheckout'],
 									[$class: 'PreBuildMerge', options: [
-										mergeRemote: 'gerrit-public',
+										mergeRemote: env.GERRIT_NAME,
 										fastForwardMode: 'NO_FF',
 										mergeStrategy: 'RECURSIVE',
 										mergeTarget: env.GERRIT_BRANCH]],

tests/CI/unittests.jenkinsfile

@@ -34,7 +34,7 @@ pipeline {
 			triggerOnEvents: [
 				commentAddedContains('^recheck$'),
 				patchsetCreated(excludeDrafts: false,
-								excludeNoCodeChange: true,
+								excludeNoCodeChange: false,
 								excludeTrivialRebase: false),
 				draftPublished()
 			],
@@ -68,7 +68,7 @@ pipeline {
 
 					stage ("Checkout") {
 						sh "sudo chown -R jenkins:users ."
-						env.GERRIT_PROJECT_URL = env.GERRIT_CHANGE_URL.replaceAll(/\/[0-9]+$/, "/${env.GERRIT_PROJECT}")
+						env.GERRIT_PROJECT_URL = env.GIT_URL.replaceAll(/[^\/]+$/, env.GERRIT_PROJECT)
 
 						/*
 						 * Jenkins has already automatically checked out the base branch
@@ -91,10 +91,10 @@ pipeline {
 							checkout scm: [$class: 'GitSCM',
 								branches: [[name: env.GERRIT_BRANCH ]],
 								extensions: [
-									[$class: 'ScmName', name: 'gerrit-public'],
+									[$class: 'ScmName', name: env.GERRIT_NAME],
 									[$class: 'CleanBeforeCheckout'],
 									[$class: 'PreBuildMerge', options: [
-										mergeRemote: 'gerrit-public',
+										mergeRemote: env.GERRIT_NAME,
 										fastForwardMode: 'NO_FF',
 										mergeStrategy: 'RECURSIVE',
 										mergeTarget: env.GERRIT_BRANCH]],