[Martin Mathieson <martin.r.mathieson@xxxxxxxxxxxxxx>]

Hi,

I'm now needing to analyse TCP conversations carried over LTE MAC/RLC/PDCP/IP.  So one frame in a log or capture can hold many segments of the same TCP conversation.

The current implementation for TCP analysis is that there is a tcp_analysis struct for each conversation.

Within that struct there is a map (emem_tree_t) from frame number -> tcp_acked

The tcp_acked struct holds the result of the analysis done on a segment during the first pass through the segments of the TCP conversation.

When a frame is re-dissected, we look up the appropriate tcp_acked struct for the segment and apply its findings to the segment currently being dissected.  The problem for me is that I often have several segments with the same frame number, so they all get tagged with the same (last?) analysis stored in a single tcp_acked struct.

My change was to expand the key now to include frame+sequence-number+ack-number (where the sequence-number and ack-number are the raw, rather than relative, values), which works well for me.

Is there a more appropriate key for looking up the segment?  I did think about adding an index for the segment within the frame, but that would be messy, and if you had to segments with the same seq+ack, I think the same analysis would always apply.

I know that the TCP analysis functionality gets used a lot, so I'm very wary of breaking it.  At the very least this will slow it down a little and use more memory.  But I surely can't be the only person who has this problem...

Thanks,

Martin

Index: packet-tcp.c
===================================================================
--- packet-tcp.c	(revision 41749)
+++ packet-tcp.c	(working copy)
@@ -94,6 +94,7 @@
 static int hf_tcp_checksum_good = -1;
 static int hf_tcp_len = -1;
 static int hf_tcp_urgent_pointer = -1;
+static int hf_tcp_analysis = -1;
 static int hf_tcp_analysis_flags = -1;
 static int hf_tcp_analysis_bytes_in_flight = -1;
 static int hf_tcp_analysis_acks_frame = -1;
@@ -753,15 +754,28 @@
 /* when this function returns, it will (if createflag) populate the ta pointer.
  */
 static void
-tcp_analyze_get_acked_struct(guint32 frame, gboolean createflag, struct tcp_analysis *tcpd)
+tcp_analyze_get_acked_struct(guint32 frame, guint32 seq, guint32 ack, gboolean createflag, struct tcp_analysis *tcpd)
 {
-    if (!tcpd)
+    emem_tree_key_t keys[2];
+    guint32         values[3];
+
+    if (!tcpd) {
         return;
+    }
 
-    tcpd->ta=se_tree_lookup32(tcpd->acked_table, frame);
+    /* Get key ready */
+    keys[0].length = 3;
+    keys[0].key = values;
+    values[0] = frame;
+    values[1] = seq;
+    values[2] = ack;
+    keys[1].length = 0;
+    keys[1].key = NULL;
+
+    tcpd->ta = se_tree_lookup32_array(tcpd->acked_table, keys);
     if((!tcpd->ta) && createflag){
-        tcpd->ta=se_alloc0(sizeof(struct tcp_acked));
-        se_tree_insert32(tcpd->acked_table, frame, (void *)tcpd->ta);
+        tcpd->ta = se_alloc0(sizeof(struct tcp_acked));
+        se_tree_insert32_array(tcpd->acked_table, keys, (void *)tcpd->ta);
     }
 }
 
@@ -829,7 +843,7 @@
     &&  seq==tcpd->fwd->nextseq
     &&  tcpd->rev->window==0 ){
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_ZERO_WINDOW_PROBE;
         goto finished_fwd;
@@ -843,7 +857,7 @@
     if( window==0
     && (flags&(TH_RST|TH_FIN|TH_SYN))==0 ){
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_ZERO_WINDOW;
     }
@@ -861,7 +875,7 @@
     &&  GT_SEQ(seq, tcpd->fwd->nextseq)
     &&  (flags&(TH_RST))==0 ){
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_LOST_PACKET;
 
@@ -880,7 +894,7 @@
     &&  seq==(tcpd->fwd->nextseq-1)
     &&  (flags&(TH_SYN|TH_FIN|TH_RST))==0 ){
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_KEEP_ALIVE;
     }
@@ -896,7 +910,7 @@
     &&  ack==tcpd->fwd->lastack
     &&  (flags&(TH_SYN|TH_FIN|TH_RST))==0 ){
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_WINDOW_UPDATE;
     }
@@ -915,7 +929,7 @@
     &&  (seq+seglen)==(tcpd->rev->lastack+(tcpd->rev->window<<(tcpd->rev->win_scale==-2?0:tcpd->rev->win_scale)))
     &&  (flags&(TH_SYN|TH_FIN|TH_RST))==0 ){
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_WINDOW_FULL;
     }
@@ -934,7 +948,7 @@
     && (tcpd->rev->lastsegmentflags&TCP_A_KEEP_ALIVE)
     &&  (flags&(TH_SYN|TH_FIN|TH_RST))==0 ){
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_KEEP_ALIVE_ACK;
         goto finished_fwd;
@@ -955,7 +969,7 @@
     && (tcpd->rev->lastsegmentflags&TCP_A_ZERO_WINDOW_PROBE)
     &&  (flags&(TH_SYN|TH_FIN|TH_RST))==0 ){
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_ZERO_WINDOW_PROBE_ACK;
         goto finished_fwd;
@@ -974,7 +988,7 @@
     &&  (flags&(TH_SYN|TH_FIN|TH_RST))==0 ){
         tcpd->fwd->dupacknum++;
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_DUPLICATE_ACK;
         tcpd->ta->dupack_num=tcpd->fwd->dupacknum;
@@ -1004,7 +1018,7 @@
     &&  (flags&(TH_ACK))!=0 ){
 /*QQQ tested*/
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_ACK_LOST_PACKET;
         /* update 'max seq to be acked' in the other direction so we don't get
@@ -1044,7 +1058,7 @@
         &&  tcpd->rev->lastack==seq
         &&  t<20000000 ){
             if(!tcpd->ta){
-                tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+                tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
             }
             tcpd->ta->flags|=TCP_A_FAST_RETRANSMISSION;
             goto finished_checking_retransmission_type;
@@ -1060,7 +1074,7 @@
         if( t<3000000
         && tcpd->fwd->nextseq != seq + seglen ){
             if(!tcpd->ta){
-                tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+                tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
             }
             tcpd->ta->flags|=TCP_A_OUT_OF_ORDER;
             goto finished_checking_retransmission_type;
@@ -1068,7 +1082,7 @@
 
         /* Then it has to be a generic retransmission */
         if(!tcpd->ta){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
         }
         tcpd->ta->flags|=TCP_A_RETRANSMISSION;
         nstime_delta(&tcpd->ta->rto_ts, &pinfo->fd->abs_ts, &tcpd->fwd->nextseqtime);
@@ -1145,7 +1159,7 @@
 
         /* If this ack matches the segment, process accordingly */
         if(ack==ual->nextseq){
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
             tcpd->ta->frame_acked=ual->frame;
             nstime_delta(&tcpd->ta->ts, &pinfo->fd->abs_ts, &ual->ts);
         }
@@ -1206,7 +1220,7 @@
 
         if (in_flight>0 && in_flight<2000000000) {
             if(!tcpd->ta){
-                tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+                tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, TRUE, tcpd);
             }
             tcpd->ta->bytes_in_flight = in_flight;
         }
@@ -1536,7 +1550,8 @@
 }
 
 static void
-tcp_print_sequence_number_analysis(packet_info *pinfo, tvbuff_t *tvb, proto_tree *parent_tree, struct tcp_analysis *tcpd)
+tcp_print_sequence_number_analysis(packet_info *pinfo, tvbuff_t *tvb, proto_tree *parent_tree,
+                          struct tcp_analysis *tcpd, guint32 seq, guint32 ack)
 {
     struct tcp_acked *ta = NULL;
     proto_item *item;
@@ -1547,14 +1562,14 @@
         return;
     }
     if(!tcpd->ta){
-        tcp_analyze_get_acked_struct(pinfo->fd->num, FALSE, tcpd);
+        tcp_analyze_get_acked_struct(pinfo->fd->num, seq, ack, FALSE, tcpd);
     }
     ta=tcpd->ta;
     if(!ta){
         return;
     }
 
-    item=proto_tree_add_text(parent_tree, tvb, 0, 0, "SEQ/ACK analysis");
+    item=proto_tree_add_item(parent_tree, hf_tcp_analysis, tvb, 0, 0, ENC_NA);
     PROTO_ITEM_SET_GENERATED(item);
     tree=proto_item_add_subtree(item, ett_tcp_analysis);
 
@@ -4096,7 +4111,7 @@
             tcpd=get_tcp_conversation_data(conv,pinfo);
         }
         if(!tcpd->ta)
-            tcp_analyze_get_acked_struct(pinfo->fd->num, TRUE, tcpd);
+            tcp_analyze_get_acked_struct(pinfo->fd->num, tcph->th_seq, tcph->th_ack, TRUE, tcpd);
         tcpd->ta->flags|=TCP_A_REUSED_PORTS;
     }
 
@@ -4587,7 +4602,14 @@
 
     /* handle TCP seq# analysis, print any extra SEQ/ACK data for this segment*/
     if(tcp_analyze_seq){
-        tcp_print_sequence_number_analysis(pinfo, tvb, tcp_tree, tcpd);
+        guint32 use_seq = tcph->th_seq;
+        guint32 use_ack = tcph->th_ack;
+        /* May need to recover absolute values here... */
+        if (tcp_relative_seq) {
+            use_seq += tcpd->fwd->base_seq;
+            use_ack += tcpd->rev->base_seq;
+        }
+        tcp_print_sequence_number_analysis(pinfo, tvb, tcp_tree, tcpd, use_seq, use_ack);
     }
 
     /* handle conversation timestamps */
@@ -4813,6 +4835,10 @@
         { "Bad Checksum",       "tcp.checksum_bad", FT_BOOLEAN, BASE_NONE, NULL, 0x0,
             "True: checksum doesn't match packet content; False: matches content or not checked", HFILL }},
 
+        { &hf_tcp_analysis,
+        { "SEQ/ACK analysis",   "tcp.analysis", FT_NONE, BASE_NONE, NULL, 0x0,
+            "This frame has some of the TCP analysis shown", HFILL }},
+
         { &hf_tcp_analysis_flags,
         { "TCP Analysis Flags",     "tcp.analysis.flags", FT_NONE, BASE_NONE, NULL, 0x0,
             "This frame has some of the TCP analysis flags set", HFILL }},