Wireshark · Ethereal-dev: Re: [Ethereal-dev] [patch] conversations demarked by setup frame number

[Jon Ringle <ml-ethereal@xxxxxxxxxx>]

On Tuesday 18 January 2005 10:27 am, Jon Ringle wrote:
> On Tuesday 18 January 2005 02:01 am, Jon Ringle wrote:
> > 1) Added a setup_frame parameter to conversation_t
> > 2) Used the conversation_t next to maintain a list of conversations with
> > the same src/dest tuple but different setup_frame number.
> > 3) Changed the signature of find_conversation() and conversation_new() to
> > pass in the frame number.
> > 4) Adjusted packet-sdp to select RTP conversation if both m=audio and
> > m=image are present, and T.38 conversation if only m=image is present. I
> > expect that RTP/T.38 dissecting to be better, but I don't have a way to
> > generate T.38 packets.
>
> One additional comment I'd like to make is that I don't believe that the
> functional behaviour has changed for any protocol with this patch except
> for SDP setting up multiple RTP/T.38 conversations based on the setup_frame
> number. The reason is that find_conversation() will return a conversation
> if one exists for the src/dest tuple. All the dissectors that use
> find_conversation() before invoking conversation_new() only test for a
> preexisting conversation. A dissector needs to add additional logic after
> the call to find_conversation() to decide whether conversation_new() needs
> to be called anyway even if there is a preexisting conversation. For
> instance in add_rtp_conversation() there is a test to see if the
> setup_frame_number matches the setup_frame number returned from
> find_conversation():

Now that 0.10.9 is released, I was wondering if the patch at:
http://www.ethereal.com/lists/ethereal-dev/200501/msg00408.html
could be applied, if it looks ok.

Attached is an abbreviated version of this patch that has the functional 
changes for review. The full patch in my previous location is mostly adding 
the extra parameter to all the calls to find_conversation() and 
conversation_new().

Jon

Index: epan/conversation.c
===================================================================
--- epan/conversation.c	(revision 13086)
+++ epan/conversation.c	(working copy)
@@ -130,7 +130,8 @@
           * conversation with new 2nd address and 2nd port.
           */
          new_conversation_from_template =
-            conversation_new(&conversation->key_ptr->addr1, addr2,
+            conversation_new(conversation->setup_frame,
+                             &conversation->key_ptr->addr1, addr2,
                              conversation->key_ptr->ptype, conversation->key_ptr->port1,
                              port2, options);
       }
@@ -141,7 +142,8 @@
           * only. Create a new conversation with new 2nd port.
           */
          new_conversation_from_template =
-            conversation_new(&conversation->key_ptr->addr1, &conversation->key_ptr->addr2,
+            conversation_new(conversation->setup_frame,
+                             &conversation->key_ptr->addr1, &conversation->key_ptr->addr2,
                              conversation->key_ptr->ptype, conversation->key_ptr->port1,
                              port2, options);
       }
@@ -152,7 +154,8 @@
           * 2. Create a new conversation with new 2nd address.
           */
          new_conversation_from_template =
-            conversation_new(&conversation->key_ptr->addr1, addr2,
+            conversation_new(conversation->setup_frame,
+                             &conversation->key_ptr->addr1, addr2,
                              conversation->key_ptr->ptype, conversation->key_ptr->port1,
                              conversation->key_ptr->port2, options);
       }
@@ -528,16 +531,42 @@
  * when searching for this conversation.
  */
 conversation_t *
-conversation_new(address *addr1, address *addr2, port_type ptype,
+conversation_new(guint32 setup_frame, address *addr1, address *addr2, port_type ptype,
     guint32 port1, guint32 port2, guint options)
 {
 /*
 	g_assert(!(options | CONVERSATION_TEMPLATE) || ((options | (NO_ADDR2 | NO_PORT2 | NO_PORT2_FORCE))) &&
 				"A conversation template may not be constructed without wildcard options");
 */
+	GHashTable* hashtable;
 	conversation_t *conversation;
+	conversation_t *tc;
+	conversation_key existing_key;
 	conversation_key *new_key;
 
+	if (options & NO_ADDR2) {
+		if (options & (NO_PORT2|NO_PORT2_FORCE)) {
+			hashtable = conversation_hashtable_no_addr2_or_port2;
+		} else {
+			hashtable = conversation_hashtable_no_addr2;
+		}
+	} else {
+		if (options & (NO_PORT2|NO_PORT2_FORCE)) {
+			hashtable = conversation_hashtable_no_port2;
+		} else {
+			hashtable = conversation_hashtable_exact;
+		}
+	}
+
+	existing_key.addr1 = *addr1;
+	existing_key.addr2 = *addr2;
+	existing_key.ptype = ptype;
+	existing_key.port1 = port1;
+	existing_key.port2 = port2;
+
+	conversation = g_hash_table_lookup(hashtable, &existing_key);
+	tc = conversation; /* Remember if lookup was successful */
+
 	new_key = g_mem_chunk_alloc(conversation_key_chunk);
 	new_key->next = conversation_keys;
 	conversation_keys = new_key;
@@ -547,8 +576,18 @@
 	new_key->port1 = port1;
 	new_key->port2 = port2;
 
-	conversation = g_mem_chunk_alloc(conversation_chunk);
+	if (conversation) {
+		for (; conversation->next; conversation = conversation->next)
+			;
+		conversation->next = g_mem_chunk_alloc(conversation_chunk);
+		conversation = conversation->next;
+	} else {
+		conversation = g_mem_chunk_alloc(conversation_chunk);
+	}
+
+	conversation->next = NULL;
 	conversation->index = new_index;
+	conversation->setup_frame = setup_frame;
 	conversation->data_list = NULL;
 
 	/* clear dissector handle */
@@ -560,23 +599,11 @@
 
 	new_index++;
 
-	if (options & NO_ADDR2) {
-		if (options & (NO_PORT2|NO_PORT2_FORCE)) {
-			g_hash_table_insert(conversation_hashtable_no_addr2_or_port2,
-			    new_key, conversation);
-		} else {
-			g_hash_table_insert(conversation_hashtable_no_addr2,
-			    new_key, conversation);
-		}
-	} else {
-		if (options & (NO_PORT2|NO_PORT2_FORCE)) {
-			g_hash_table_insert(conversation_hashtable_no_port2,
-			    new_key, conversation);
-		} else {
-			g_hash_table_insert(conversation_hashtable_exact,
-			    new_key, conversation);
-		}
-	}
+	/* only insert a hash table entry if this
+	 * is the first conversation with this key */
+	if (!tc)
+		g_hash_table_insert(hashtable, new_key, conversation);
+
 	return conversation;
 }
 
@@ -653,9 +680,11 @@
  * addr1, port1, addr2, and port2.
  */
 static conversation_t *
-conversation_lookup_hashtable(GHashTable *hashtable, address *addr1, address *addr2,
+conversation_lookup_hashtable(GHashTable *hashtable, guint32 frame_num, address *addr1, address *addr2,
     port_type ptype, guint32 port1, guint32 port2)
 {
+	conversation_t* conversation;
+	conversation_t* match;
 	conversation_key key;
 
 	/*
@@ -667,7 +696,18 @@
 	key.ptype = ptype;
 	key.port1 = port1;
 	key.port2 = port2;
-	return g_hash_table_lookup(hashtable, &key);
+
+	match = g_hash_table_lookup(hashtable, &key);
+
+	if (match) {
+		for (conversation = match->next; conversation; conversation = conversation->next) {
+			if ((conversation->setup_frame < frame_num)
+				&& (conversation->setup_frame > match->setup_frame))
+				match = conversation;
+		}
+	}
+
+	return match;
 }
 
 
@@ -708,7 +748,7 @@
  *	otherwise, we found no matching conversation, and return NULL.
  */
 conversation_t *
-find_conversation(address *addr_a, address *addr_b, port_type ptype,
+find_conversation(guint32 frame_num, address *addr_a, address *addr_b, port_type ptype,
     guint32 port_a, guint32 port_b, guint options)
 {
    conversation_t *conversation;
@@ -724,7 +764,7 @@
        */
       conversation =
          conversation_lookup_hashtable(conversation_hashtable_exact,
-         addr_a, addr_b, ptype,
+         frame_num, addr_a, addr_b, ptype,
          port_a, port_b);
       if ((conversation == NULL) && (addr_a->type == AT_FC)) {
          /* In Fibre channel, OXID & RXID are never swapped as
@@ -732,7 +772,7 @@
           */
          conversation =
             conversation_lookup_hashtable(conversation_hashtable_exact,
-            addr_b, addr_a, ptype,
+            frame_num, addr_b, addr_a, ptype,
             port_a, port_b);
       }
       if (conversation != NULL)
@@ -755,14 +795,14 @@
        */
       conversation =
          conversation_lookup_hashtable(conversation_hashtable_no_addr2,
-         addr_a, addr_b, ptype, port_a, port_b);
+         frame_num, addr_a, addr_b, ptype, port_a, port_b);
       if ((conversation == NULL) && (addr_a->type == AT_FC)) {
          /* In Fibre channel, OXID & RXID are never swapped as
           * TCP/UDP ports are in TCP/IP.
           */
          conversation =
             conversation_lookup_hashtable(conversation_hashtable_no_addr2,
-            addr_b, addr_a, ptype,
+            frame_num, addr_b, addr_a, ptype,
             port_a, port_b);
       }
       if (conversation != NULL) {
@@ -805,7 +845,7 @@
       if (!(options & NO_ADDR_B)) {
          conversation =
             conversation_lookup_hashtable(conversation_hashtable_no_addr2,
-            addr_b, addr_a, ptype, port_b, port_a);
+            frame_num, addr_b, addr_a, ptype, port_b, port_a);
          if (conversation != NULL) {
             /*
              * If this is for a connection-oriented
@@ -847,14 +887,14 @@
        */
       conversation =
          conversation_lookup_hashtable(conversation_hashtable_no_port2,
-         addr_a, addr_b, ptype, port_a, port_b);
+         frame_num, addr_a, addr_b, ptype, port_a, port_b);
       if ((conversation == NULL) && (addr_a->type == AT_FC)) {
          /* In Fibre channel, OXID & RXID are never swapped as
           * TCP/UDP ports are in TCP/IP
           */
          conversation =
             conversation_lookup_hashtable(conversation_hashtable_no_port2,
-            addr_b, addr_a, ptype, port_a, port_b);
+            frame_num, addr_b, addr_a, ptype, port_a, port_b);
       }
       if (conversation != NULL) {
          /*
@@ -896,7 +936,7 @@
       if (!(options & NO_PORT_B)) {
          conversation =
             conversation_lookup_hashtable(conversation_hashtable_no_port2,
-            addr_b, addr_a, ptype, port_b, port_a);
+            frame_num, addr_b, addr_a, ptype, port_b, port_a);
          if (conversation != NULL) {
             /*
              * If this is for a connection-oriented
@@ -933,7 +973,7 @@
     */
    conversation =
       conversation_lookup_hashtable(conversation_hashtable_no_addr2_or_port2,
-      addr_a, addr_b, ptype, port_a, port_b);
+      frame_num, addr_a, addr_b, ptype, port_a, port_b);
    if (conversation != NULL) {
       /*
        * If this is for a connection-oriented protocol:
@@ -978,11 +1018,11 @@
    if (addr_a->type == AT_FC)
       conversation =
       conversation_lookup_hashtable(conversation_hashtable_no_addr2_or_port2,
-      addr_b, addr_a, ptype, port_a, port_b);
+      frame_num, addr_b, addr_a, ptype, port_a, port_b);
    else 
       conversation =
       conversation_lookup_hashtable(conversation_hashtable_no_addr2_or_port2,
-      addr_b, addr_a, ptype, port_b, port_a);
+      frame_num, addr_b, addr_a, ptype, port_b, port_a);
    if (conversation != NULL) {
       /*
        * If this is for a connection-oriented protocol, set the
@@ -1097,7 +1137,7 @@
 {
 	conversation_t *conversation;
 
-	conversation = find_conversation(addr_a, addr_b, ptype, port_a,
+	conversation = find_conversation(pinfo->fd->num, addr_a, addr_b, ptype, port_a,
 	    port_b, 0);
 
 	if (conversation != NULL) {
Index: epan/conversation.h
===================================================================
--- epan/conversation.h	(revision 13086)
+++ epan/conversation.h	(working copy)
@@ -66,6 +66,7 @@
 typedef struct conversation {
 	struct conversation *next;	/* pointer to next conversation on hash chain */
 	guint32	index;			/* unique ID for conversation */
+	guint32 setup_frame;	/* frame number that setup this conversation */
 	GSList *data_list;		/* list of data associated with conversation */
 	dissector_handle_t dissector_handle;
 					/* handle for protocol dissector client associated with conversation */
@@ -75,10 +76,10 @@
 
 extern void conversation_init(void);
 
-extern conversation_t *conversation_new(address *addr1, address *addr2,
+extern conversation_t *conversation_new(guint32 setup_frame, address *addr1, address *addr2,
     port_type ptype, guint32 port1, guint32 port2, guint options);
 
-extern conversation_t *find_conversation(address *addr_a, address *addr_b,
+extern conversation_t *find_conversation(guint32 frame_num, address *addr_a, address *addr_b,
     port_type ptype, guint32 port_a, guint32 port_b, guint options);
 
 extern void conversation_add_proto_data(conversation_t *conv, int proto,
Index: epan/dissectors/packet-t38.c
===================================================================
--- epan/dissectors/packet-t38.c	(revision 13086)
+++ epan/dissectors/packet-t38.c	(working copy)
@@ -199,14 +199,14 @@
          * Check if the ip address and port combination is not
          * already registered as a conversation.
          */
-        p_conv = find_conversation( addr, &null_addr, PT_UDP, port, other_port,
+        p_conv = find_conversation( setup_frame_number, addr, &null_addr, PT_UDP, port, other_port,
                                 NO_ADDR_B | (!other_port ? NO_PORT_B : 0));
 
         /*
          * If not, create a new conversation.
          */
-        if ( ! p_conv ) {
-                p_conv = conversation_new( addr, &null_addr, PT_UDP,
+        if ( !p_conv || p_conv->setup_frame != setup_frame_number) {
+                p_conv = conversation_new( setup_frame_number, addr, &null_addr, PT_UDP,
                                            (guint32)port, (guint32)other_port,
                                                                    NO_ADDR2 | (!other_port ? NO_PORT2 : 0));
         }
@@ -892,7 +892,7 @@
         if (!p_conv_data)
         {
                 /* First time, get info from conversation */
-                p_conv = find_conversation(&pinfo->net_src, &pinfo->net_dst,
+                p_conv = find_conversation(pinfo->fd->num, &pinfo->net_src, &pinfo->net_dst,
                                    pinfo->ptype,
                                    pinfo->srcport, pinfo->destport, NO_ADDR_B);
                 if (p_conv)
Index: epan/dissectors/packet-sdp.c
===================================================================
--- epan/dissectors/packet-sdp.c	(revision 13086)
+++ epan/dissectors/packet-sdp.c	(working copy)
@@ -207,7 +207,8 @@
 
 	guint32 	port=0;
 	gboolean 	is_rtp=FALSE;
-	gboolean        is_t38=FALSE;
+	gboolean 	is_t38=FALSE;
+	gboolean 	set_rtp=FALSE;
 	gboolean 	is_ipv4_addr=FALSE;
 	gboolean	is_ipv6_addr=FALSE;
     guint32 	ipaddr[4];
@@ -399,12 +400,13 @@
 				}
 		    }
 	    }
-	    /* Add rtp and rtcp conversation, if available */
+	    /* 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);
+				set_rtp = TRUE;
 		    }
 		    if(rtcp_handle){
 				port++;
@@ -413,8 +415,8 @@
 		    }
 	    }
 			
-	    /* Add t38 conversation, if available */
-	    if((!pinfo->fd->flags.visited) && port!=0 && is_t38 && is_ipv4_addr){
+	    /* Add t38 conversation, if available and only if no rtp */
+	    if((!pinfo->fd->flags.visited) && port!=0 && !set_rtp && is_t38 && is_ipv4_addr){
                     src_addr.data=(char *)&ipaddr;
                     if(t38_handle){
                                 t38_add_address(pinfo, &src_addr, port, 0, "SDP", pinfo->fd->num);
Index: epan/dissectors/packet-rtp.c
===================================================================
--- epan/dissectors/packet-rtp.c	(revision 13086)
+++ epan/dissectors/packet-rtp.c	(working copy)
@@ -216,14 +216,14 @@
 	 * Check if the ip address and port combination is not
 	 * already registered as a conversation.
 	 */
-	p_conv = find_conversation( addr, &null_addr, PT_UDP, port, other_port,
+	p_conv = find_conversation( setup_frame_number, addr, &null_addr, PT_UDP, port, other_port,
                                 NO_ADDR_B | (!other_port ? NO_PORT_B : 0));
 
 	/*
 	 * If not, create a new conversation.
 	 */
-	if ( ! p_conv ) {
-		p_conv = conversation_new( addr, &null_addr, PT_UDP,
+	if ( !p_conv || p_conv->setup_frame != setup_frame_number) {
+		p_conv = conversation_new( setup_frame_number, addr, &null_addr, PT_UDP,
 		                           (guint32)port, (guint32)other_port,
 								   NO_ADDR2 | (!other_port ? NO_PORT2 : 0));
 	}
@@ -633,7 +633,7 @@
 	if (!p_conv_data)
 	{
 		/* First time, get info from conversation */
-		p_conv = find_conversation(&pinfo->net_dst, &pinfo->net_src,
+		p_conv = find_conversation(pinfo->fd->num, &pinfo->net_dst, &pinfo->net_src,
                                    pinfo->ptype,
                                    pinfo->destport, pinfo->srcport, NO_ADDR_B);
 		if (p_conv)