From 737dabb0437bd5a0a1f4c07189ca59f1c83afb55 Mon Sep 17 00:00:00 2001 From: novakji Date: Sun, 13 Nov 2016 20:20:17 +0100 Subject: SDP: Payload type name for dynamic payload is wrong for reverse RTP channels Bug: 13132 Change-ID: I61a2575f9d8da958ae2fb01c71f3c71c9643ddea Reviewed-on: https://code.wireshark.org/review/18804 Reviewed-by: Pascal Quantin Petri-Dish: Pascal Quantin Reviewed-by: Michael Mann Tested-by: Michael Mann --- epan/dissectors/packet-sdp.c | 52 +++++++++++--------------------------------- 1 file changed, 13 insertions(+), 39 deletions(-) (limited to 'epan') diff --git a/epan/dissectors/packet-sdp.c b/epan/dissectors/packet-sdp.c index befdb9fd55..9bd39943ed 100644 --- a/epan/dissectors/packet-sdp.c +++ b/epan/dissectors/packet-sdp.c @@ -259,7 +259,7 @@ typedef struct { const guint8 *media_type[SDP_MAX_RTP_CHANNELS]; const guint8 *media_port[SDP_MAX_RTP_CHANNELS]; const guint8 *media_proto[SDP_MAX_RTP_CHANNELS]; - guint8 media_count; + gint8 media_count; /* MSRP transport info (as set while parsing path attribute) */ gboolean msrp_transport_address_set; @@ -330,7 +330,7 @@ static void sdp_dump_transport_info(const transport_info_t* info) { } DENDENT(); count = (int)info->media_count; - DPRINT2(("media_count=%d", count)); + DPRINT2(("media_count=%d", count+1)); DPRINT2(("rtp channels:")); DINDENT(); for (i=0; i <= count; i++) { @@ -370,9 +370,9 @@ static void sdp_dump_disposable_media_info(const disposable_media_info_t* info) DPRINT2(("connection_type=%s", info->connection_type ? (const char *)info->connection_type : "NULL")); count = (int)info->media_count; - DPRINT2(("media_count=%d",count)); + DPRINT2(("media_count=%d",count+1)); DINDENT(); - for (i=0; i < count; i++) { + for (i=0; i <= count; i++) { DPRINT2(("media #%d:",i)); DINDENT(); DPRINT2(("media_type=%s", info->media_type[i] ? (const char *)info->media_type[i] : "NULL")); @@ -1796,7 +1796,7 @@ convert_disposable_media(transport_info_t* transport_info, disposable_media_info gint8 n, transport_index; guint proto_bitmask; - for (n = 0; (n < media_info->media_count) && (n+start_transport_info_count < SDP_MAX_RTP_CHANNELS); n++) + for (n = 0; (n <= media_info->media_count) && (n+start_transport_info_count < SDP_MAX_RTP_CHANNELS); n++) { transport_index = n+start_transport_info_count; if (media_info->media_port[n] != NULL) { @@ -1940,6 +1940,7 @@ setup_sdp_transport(tvbuff_t *tvb, packet_info *pinfo, enum sdp_exchange_type ex } memset(&media_info, 0, sizeof(media_info)); + media_info.media_count=-1; if (request_frame != 0) transport_info = (transport_info_t*)wmem_tree_lookup32( sdp_transport_reqs, request_frame ); @@ -1976,8 +1977,8 @@ setup_sdp_transport(tvbuff_t *tvb, packet_info *pinfo, enum sdp_exchange_type ex (transport_info->sdp_status == SDP_EXCHANGE_ANSWER_ACCEPT)) return; - if (transport_info->media_count > 0) - start_transport_info_count = transport_info->media_count; + if (transport_info->media_count >= 0) + start_transport_info_count = transport_info->media_count+1; DPRINT(("start_transport_info_count=%d", start_transport_info_count)); @@ -2026,10 +2027,10 @@ setup_sdp_transport(tvbuff_t *tvb, packet_info *pinfo, enum sdp_exchange_type ex hf = hf_media; /* Increase the count of media channels, but don't walk off the end of the arrays. */ - if (((transport_info->media_count < 0) && (in_media_description == FALSE)) || (transport_info->media_count < (SDP_MAX_RTP_CHANNELS-1))) + if (transport_info->media_count < (SDP_MAX_RTP_CHANNELS-1)) transport_info->media_count++; - if (in_media_description && (media_info.media_count < (SDP_MAX_RTP_CHANNELS-1))) + if (media_info.media_count < (SDP_MAX_RTP_CHANNELS-1)) media_info.media_count++; in_media_description = TRUE; @@ -2062,26 +2063,6 @@ setup_sdp_transport(tvbuff_t *tvb, packet_info *pinfo, enum sdp_exchange_type ex offset = next_offset; } - if (in_media_description) { - /* Increase the count of media channels, but don't walk off the end of the arrays. */ - /* XXX: I don't know why this was done here - I'm keeping it here in case - * removing it causes problems, but it's wrong. transport_info->media_count - * is already incremented in the while() loop above. Incrementing it - * again here will cause bugs. The name of this is misleading, because - * 'transport_info->media_count' is actually an index, not count. - * In other words, it's a 0-based number, of the current rtp channel. - * So debug printing shows bogus rtp channels get created and then later - * removed because luckily it knows they were bogus. But it will cause bugs - * because if we're not delaying, then for the SDP_EXCHANGE_ANSWER_ACCEPT - * run through this function, it will add new RTP channels at a +1 index, - * which will likely cause problems. - */ - if (transport_info->media_count < (SDP_MAX_RTP_CHANNELS-1)) - transport_info->media_count++; - if (media_info.media_count < (SDP_MAX_RTP_CHANNELS-1)) - media_info.media_count++; - } - #ifdef DEBUG_CONVERSATION sdp_dump_disposable_media_info(&media_info); #endif @@ -2326,6 +2307,7 @@ dissect_sdp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void* data _U_) } memset(&media_info, 0, sizeof(media_info)); + media_info.media_count=-1; /* * As RFC 2327 says, "SDP is purely a format for session @@ -2427,7 +2409,7 @@ dissect_sdp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void* data _U_) if (local_transport_info.media_count < (SDP_MAX_RTP_CHANNELS-1)) local_transport_info.media_count++; - if (in_media_description && (media_info.media_count < (SDP_MAX_RTP_CHANNELS-1))) + if (media_info.media_count < (SDP_MAX_RTP_CHANNELS-1)) media_info.media_count++; in_media_description = TRUE; @@ -2466,14 +2448,6 @@ dissect_sdp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void* data _U_) offset = next_offset; } - if (in_media_description) { - /* Increase the count of media channels, but don't walk off the end of the arrays. */ - if (local_transport_info.media_count < (SDP_MAX_RTP_CHANNELS-1)) - local_transport_info.media_count++; - if (media_info.media_count < (SDP_MAX_RTP_CHANNELS-1)) - media_info.media_count++; - } - /* Take all of the collected strings and convert them into something permanent * for the life of the capture */ @@ -2493,7 +2467,7 @@ dissect_sdp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree, void* data _U_) } #endif - for (n = 0; n < local_transport_info.media_count; n++) + for (n = 0; n <= local_transport_info.media_count; n++) { /* Add (s)rtp and (s)rtcp conversation, if available (overrides t38 if conversation already set) */ /* XXX - This is a placeholder for higher layer protocols that haven't implemented the proper -- cgit v1.2.3