Wireshark · Ethereal-dev: Re: [Ethereal-dev] Re: [Patch] Correction of VoIP Calls with ordered taps - H323 SIP MGCP

[Alejandro Vaquero <alejandrovaquero@xxxxxxxxx>]

Hi Lars,
   Attached is a patch to fix the "key2" bug and also added more 
comments on that part of the code.
   This is an improvement used to dissect Dynamic payload types used in 
conversations, and also used to display the codec list summary in the 
"Voip Calls Graph".
    Hope the comments explain what I tried to do.

Regards
Alejandro

Lars Roland wrote:

> Alejandro Vaquero schrieb:
>
> > Hi All,
> >    Find attached a patch to fix the H323 and SIP (not changes needed 
> > for MGCP) voip_calls for the new tap ordering.
> >     The patch also improve the dynamic SDP payload type dissection 
> > adding individual hash tables per SDP Media Description to the 
> > conversation.
>
>
> I've checked in the fix for the voip call analysis.
> I haven't checked in the changes for the SDP dissector, because they 
> contain at least one serious bug. In line 1059 of packet-sdp.c (with 
> your your patch applied) key2 is used uninitialized. Besides this bug 
> I have some doubts about the rest, too.
> IMO the block from line 1052 to 1068 is very ugly and I'm not sure if 
> it works correctly. Some comments could help here.
> I have some additional questions about this patch.
> Is it a bugfix or just an improvement? and if not, will this patch 
> replace a huge hash table (>1000 entries) with some smaller tables?
>
> Regards,
> Lars
>
>
>
>
> _______________________________________________
> Ethereal-dev mailing list
> Ethereal-dev@xxxxxxxxxxxx
> http://www.ethereal.com/mailman/listinfo/ethereal-dev
Index: packet-sdp.c
===================================================================
--- packet-sdp.c	(revision 14045)
+++ packet-sdp.c	(working copy)
@@ -166,14 +166,18 @@
 #define SDP_MAX_RTP_PAYLOAD_TYPES 20
 
 typedef struct {
+	gint32 pt[SDP_MAX_RTP_PAYLOAD_TYPES];
+	gint8 pt_count;
+	GHashTable *rtp_dyn_payload;
+} transport_media_pt_t;
+
+typedef struct {
 	char *connection_address;
 	char *connection_type;
 	char *media_port[SDP_MAX_RTP_CHANNELS];
 	char *media_proto[SDP_MAX_RTP_CHANNELS];
-	gint32 media_pt[SDP_MAX_RTP_PAYLOAD_TYPES];
+	transport_media_pt_t media[SDP_MAX_RTP_CHANNELS];
 	gint8 media_count;
-	gint8 media_pt_count;
-	GHashTable *rtp_dyn_payload;
 } transport_info_t;
 
 /* static functions */
@@ -222,7 +226,7 @@
 	gboolean 	is_ipv4_addr=FALSE;
 	gboolean	is_ipv6_addr=FALSE;
     guint32 	ipaddr[4];
-	gint		n;
+	gint		n,i;
 
     /* Initialise packet info for passing to tap */
     sdp_pi = g_malloc(sizeof(sdp_packet_info));
@@ -235,16 +239,11 @@
 	{
 	    transport_info.media_port[n]=NULL;
 	    transport_info.media_proto[n]=NULL;
+		transport_info.media[n].pt_count = 0;
+		transport_info.media[n].rtp_dyn_payload = g_hash_table_new( g_int_hash, g_int_equal);
 	}
-	for (n=0; n < SDP_MAX_RTP_PAYLOAD_TYPES; n++)
-	{
-	    transport_info.media_pt[n]=-1;
-	}
 	transport_info.media_count = 0;
-	transport_info.media_pt_count = 0;
 
-	transport_info.rtp_dyn_payload = g_hash_table_new( g_int_hash, g_int_equal);
-
 	/*
 	 * As RFC 2327 says, "SDP is purely a format for session
 	 * description - it does not incorporate a transport protocol,
@@ -420,12 +419,13 @@
 				}
 		    }
 	    }
+		set_rtp = FALSE;
 	    /* Add rtp and rtcp conversation, if available (overrides t38 if conversation already set) */
 	    if((!pinfo->fd->flags.visited) && port!=0 && is_rtp && (is_ipv4_addr || is_ipv6_addr)){
 		    src_addr.data=(char *)&ipaddr;
 		    if(rtp_handle){
 				rtp_add_address(pinfo, &src_addr, port, 0,
-	                "SDP", pinfo->fd->num, transport_info.rtp_dyn_payload);
+	                "SDP", pinfo->fd->num, transport_info.media[n].rtp_dyn_payload);
 				set_rtp = TRUE;
 		    }
 		    if(rtcp_handle){
@@ -442,8 +442,32 @@
                                 t38_add_address(pinfo, &src_addr, port, 0, "SDP", pinfo->fd->num);
                     }
 	    }
+
+		/* Create the summary str for the Voip Call analysis */
+		for (i = 0; i < transport_info.media[n].pt_count; i++)
+		{
+			/* if the payload type is dynamic (96 to 127), check the hash table to add the desc in the SDP summary */
+			if ( (transport_info.media[n].pt[i] >=96) && (transport_info.media[n].pt[i] <=127) ) {
+				gchar *str_dyn_pt = g_hash_table_lookup(transport_info.media[n].rtp_dyn_payload, &transport_info.media[n].pt[i]);
+				if (str_dyn_pt)
+					g_snprintf(sdp_pi->summary_str, 50, "%s %s", sdp_pi->summary_str, str_dyn_pt);
+				else
+					g_snprintf(sdp_pi->summary_str, 50, "%s %d", sdp_pi->summary_str, transport_info.media[n].pt[i]);
+			} else 
+				g_snprintf(sdp_pi->summary_str, 50, "%s %s", sdp_pi->summary_str, val_to_str(transport_info.media[n].pt[i], rtp_payload_type_short_vals, "%u"));
+		}
+
+		/* Free the hash table if we did't assigned it to a conv use it */
+		if (set_rtp == FALSE) 
+			rtp_free_hash_dyn_payload(transport_info.media[n].rtp_dyn_payload);
 	}
 
+	/* Free the remainded hash tables not used */
+	for (n = transport_info.media_count; n < SDP_MAX_RTP_CHANNELS; n++)
+	{
+		rtp_free_hash_dyn_payload(transport_info.media[n].rtp_dyn_payload);
+	}
+
 	/* Free up 'connection info' strings */
 	if(transport_info.connection_address) {
 		g_free(transport_info.connection_address);
@@ -452,31 +476,12 @@
 		g_free(transport_info.connection_type);
 	}
 
-
 	datalen = tvb_length_remaining(tvb, offset);
 	if (datalen > 0) {
 		proto_tree_add_text(sdp_tree, tvb, offset, datalen,
 		    "Data (%d bytes)", datalen);
 	}
 
-	/* Create the summary str for the Voip Call analysis */
-	for (n = 0; n < transport_info.media_pt_count; n++)
-	{
-		/* if the payload type is dynamic (96 to 127), check the hash table */
-		if ( (transport_info.media_pt[n] >=96) && (transport_info.media_pt[n] <=127) ) {
-			gchar *str_dyn_pt = g_hash_table_lookup(transport_info.rtp_dyn_payload, &transport_info.media_pt[n]);
-			if (str_dyn_pt)
-				g_snprintf(sdp_pi->summary_str, 50, "%s %s", sdp_pi->summary_str, str_dyn_pt);
-			else
-				g_snprintf(sdp_pi->summary_str, 50, "%s %d", sdp_pi->summary_str, transport_info.media_pt[n]);
-		} else 
-			g_snprintf(sdp_pi->summary_str, 50, "%s %s", sdp_pi->summary_str, val_to_str(transport_info.media_pt[n], rtp_payload_type_short_vals, "%u"));
-	}
-
-	/* Free the hash table if we don't use it */
-    if (set_rtp == FALSE) 
-		rtp_free_hash_dyn_payload(transport_info.rtp_dyn_payload);
-
     /* Report this packet to the tap */
     tap_queue_packet(sdp_tap, pinfo, sdp_pi);
 }
@@ -840,7 +845,7 @@
 dissect_sdp_media(tvbuff_t *tvb, proto_item *ti,
 		  transport_info_t *transport_info){
   proto_tree *sdp_media_tree;
-  gint offset, next_offset, tokenlen;
+  gint offset, next_offset, tokenlen, index;
   guint8 *media_format;
 
   offset = 0;
@@ -927,9 +932,10 @@
       media_format = tvb_get_string(tvb, offset, tokenlen);
       proto_tree_add_string(sdp_media_tree, hf_media_format, tvb, offset,
                              tokenlen, val_to_str(atol(media_format), rtp_payload_type_vals, "%u"));
-	  transport_info->media_pt[transport_info->media_pt_count] = atol(media_format);
-	  if (transport_info->media_pt_count < SDP_MAX_RTP_PAYLOAD_TYPES - 1)
-		  transport_info->media_pt_count++;
+	  index = transport_info->media[transport_info->media_count].pt_count;
+	  transport_info->media[transport_info->media_count].pt[index] = atol(media_format);
+	  if (index < (SDP_MAX_RTP_PAYLOAD_TYPES-1))
+		  transport_info->media[transport_info->media_count].pt_count++;
       g_free(media_format);
     } else {
       proto_tree_add_item(sdp_media_tree, hf_media_format, tvb,
@@ -954,7 +960,7 @@
 
 static void dissect_sdp_media_attribute(tvbuff_t *tvb, proto_item * ti, transport_info_t *transport_info){
   proto_tree *sdp_media_attribute_tree;
-  gint offset, next_offset, tokenlen;
+  gint offset, next_offset, tokenlen, n;
   guint8 *field_name;
   guint8 *payload_type;
   guint8 *encoding_name;
@@ -1013,8 +1019,40 @@
 
 	key=g_malloc( sizeof(gint) );
 	*key=atol(payload_type);
-	g_hash_table_insert(transport_info->rtp_dyn_payload, key, encoding_name);
 
+	/* As per RFC2327 it is possible to have multiple Media Descriptions ("m=") For example:
+
+			a=rtpmap:101 G726-32/8000
+			m=audio 49170 RTP/AVP 0 97
+			a=rtpmap:97 telephone-event/8000
+			m=audio 49172 RTP/AVP 97 101
+			a=rtpmap:97 G726-24/8000
+
+	The Media attributes ("a="s) after the "m=" only apply for that "m=". 
+	If there is an "a=" before the first "m=", that attribute apply for all the session	(all the "m="s).
+	*/
+
+	/* so, if this "a=" appear before any "m=", we add it to all the dynamic hash tables */ 
+	if (transport_info->media_count == 0) {
+		for (n=0; n < SDP_MAX_RTP_CHANNELS; n++) {
+			if (n==0)
+				g_hash_table_insert(transport_info->media[n].rtp_dyn_payload, key, encoding_name);
+			else {	/* we create a new key and encoding_name to assign to the other hash tables */
+				gint *key2;
+				key2=g_malloc( sizeof(gint) );
+				*key2=atol(payload_type);
+				g_hash_table_insert(transport_info->media[n].rtp_dyn_payload, key2, g_strdup(encoding_name));
+			}
+		}
+
+	/* if the "a=" is after an "m=", only apply to this "m=" */
+	} else 
+		/* in case there is an overflow in SDP_MAX_RTP_CHANNELS, we keep always the last "m=" */
+		if (transport_info->media_count == SDP_MAX_RTP_CHANNELS-1)
+			g_hash_table_insert(transport_info->media[ transport_info->media_count ].rtp_dyn_payload, key, encoding_name);		
+		else
+			g_hash_table_insert(transport_info->media[ transport_info->media_count-1 ].rtp_dyn_payload, key, encoding_name);
+
     g_free(payload_type);
   } else g_free(field_name);
 }