TFTP: Rewritten object (file) export algorithm

Significantly increased readability of the code, speed of work
and reduced size of the consumed memory.
 - The number of memory allocations has been reduced from N to a few
 - Removed double (redundant) data copying

Change-Id: I05aed194932ed3305eefb6e2e0f847e57851c41c
Reviewed-on: https://code.wireshark.org/review/36026
Reviewed-by: Martin Mathieson <martin.r.mathieson@googlemail.com>

1 files changed, 47 insertions, 118 deletions

diff --git a/epan/dissectors/packet-tftp.c b/epan/dissectors/packet-tftp.c
index 022e9b09d5..99d3422c25 100644
--- a/epan/dissectors/packet-tftp.c
+++ b/epan/dissectors/packet-tftp.c
@@ -52,11 +52,12 @@ typedef struct _tftp_conv_info_t {
   /* Sequence analysis */
   guint32      next_block_num;
   gboolean     blocks_missing;
+  guint        file_length;
+  gboolean     last_package_available;
 
-  /* When exporting file object, build up list of data blocks here */
+  /* When exporting file object, build data here */
   guint32      next_tap_block_num;
-  GSList       *block_list;
-  guint        file_length;
+  guint8       *payload_data;
 
   /* Assembly of fragments */
   guint32      reassembly_id;
@@ -190,19 +191,11 @@ static int tftp_eo_tap = -1;
 /* Preference setting - defragment fragmented TFTP files */
 static gboolean tftp_defragment = FALSE;
 
-/* A list of block list entries to delete from cleanup callback when window is closed. */
-typedef struct eo_info_dynamic_t {
-    gchar  *filename;
-    GSList *block_list;
-} eo_info_dynamic_t;
-static GSList *s_dynamic_info_list = NULL;
-
 /* Used for TFTP Export Object feature */
 typedef struct _tftp_eo_t {
-	guint32  pkt_num;
-	gchar    *filename;
-	guint32  payload_len;
-	GSList   *block_list;
+  gchar    *filename;
+  guint32  payload_len;
+  guint8   *payload_data;
 } tftp_eo_t;
 
 /* Tap function */
@@ -213,10 +206,6 @@ tftp_eo_packet(void *tapdata, packet_info *pinfo, epan_dissect_t *edt _U_, const
   const tftp_eo_t *eo_info = (const tftp_eo_t *)data;
   export_object_entry_t *entry;
 
-  GSList *block_iterator;
-  guint  payload_data_offset = 0;
-  eo_info_dynamic_t *dynamic_info;
-
   /* These values will be freed when the Export Object window is closed. */
   entry = g_new(export_object_entry_t, 1);
 
@@ -226,63 +215,23 @@ tftp_eo_packet(void *tapdata, packet_info *pinfo, epan_dissect_t *edt _U_, const
   /* Copy filename */
   entry->filename = g_path_get_basename(eo_info->filename);
 
-  /* Iterate over list of blocks and concatenate into contiguous memory */
+  /* Free up unnecessary memory */
+  g_free(eo_info->filename);
+
+  /* Pass out the contigous data and length already accumulated. */
   entry->payload_len = eo_info->payload_len;
-  entry->payload_data = (guint8 *)g_try_malloc((gsize)entry->payload_len);
-  for (block_iterator = eo_info->block_list; block_iterator; block_iterator = block_iterator->next) {
-    GByteArray *block = (GByteArray*)block_iterator->data;
-    memcpy(entry->payload_data + payload_data_offset,
-               block->data,
-               block->len);
-    payload_data_offset += block->len;
-  }
+  entry->payload_data = eo_info->payload_data;
 
   /* These 2 fields not used */
   entry->hostname = NULL;
   entry->content_type = NULL;
 
-  /* Add to list of entries to be cleaned up.  eo_info is only packet scope, so
-     need to make list only of block list now */
-  dynamic_info = g_new(eo_info_dynamic_t, 1);
-  dynamic_info->filename = eo_info->filename;
-  dynamic_info->block_list = eo_info->block_list;
-  s_dynamic_info_list = g_slist_append(s_dynamic_info_list, (eo_info_dynamic_t*)dynamic_info);
-
   /* Pass out entry to the GUI */
   object_list->add_entry(object_list->gui_data, entry);
 
   return TAP_PACKET_REDRAW; /* State changed - window should be redrawn */
 }
 
-/* Clean up the stored parts of a single tapped entry */
-static void cleanup_tftp_eo(eo_info_dynamic_t *dynamic_info)
-{
-  GSList *block_iterator;
-  /* Free the filename */
-  g_free(dynamic_info->filename);
-
-  /* Walk list of block items */
-  for (block_iterator = dynamic_info->block_list; block_iterator; block_iterator = block_iterator->next) {
-    GByteArray *block = (GByteArray*)(block_iterator->data);
-    /* Free block data and the block itself */
-    g_byte_array_free(block, TRUE);
-  }
-}
-
-/* Callback for freeing up data supplied with taps.  The taps themselves only have
-   packet scope, so only store/free dynamic memory pointers */
-static void tftp_eo_cleanup(void)
-{
-  /* Cleanup each entry in the global list */
-  GSList *dynamic_iterator;
-  for (dynamic_iterator = s_dynamic_info_list; dynamic_iterator; dynamic_iterator = dynamic_iterator->next) {
-    eo_info_dynamic_t *dynamic_info = (eo_info_dynamic_t*)dynamic_iterator->data;
-    cleanup_tftp_eo(dynamic_info);
-  }
-  /* List is empty again */
-  s_dynamic_info_list = NULL;
-}
-
 static void
 tftp_dissect_options(tvbuff_t *tvb, packet_info *pinfo, int offset,
                      proto_tree *tree, guint16 opcode, tftp_conv_info_t *tftp_info)
@@ -332,20 +281,6 @@ tftp_dissect_options(tvbuff_t *tvb, packet_info *pinfo, int offset,
   }
 }
 
-static void cleanup_tftp_blocks(tftp_conv_info_t *conv)
-{
-    GSList *block_iterator;
-
-    /* Walk list of block items */
-    for (block_iterator = conv->block_list; block_iterator; block_iterator = block_iterator->next) {
-        GByteArray *block = (GByteArray*)block_iterator->data;
-        /* Free block data and the block itself */
-        g_byte_array_free(block, TRUE);
-    }
-    conv->block_list = NULL;
-    conv->file_length = 0;
-}
-
 static gboolean
 error_is_likely_tsize_probe(guint16 error, const tftp_conv_info_t *tftp_info)
 {
@@ -418,6 +353,7 @@ static void dissect_tftp_message(tftp_conv_info_t *tftp_info,
   fragment_head *tftpfd_head = NULL;
   heur_dtbl_entry_t *hdtbl_entry;
   struct tftpinfo tftpinfo;
+  guint32     payload_data_offset;
 
   col_set_str(pinfo->cinfo, COL_PROTOCOL, "TFTP");
 
@@ -509,6 +445,7 @@ static void dissect_tftp_message(tftp_conv_info_t *tftp_info,
     proto_item_set_len(root_ti, 4);
     blocknum_item = proto_tree_add_item_ret_uint(tftp_tree, hf_tftp_blocknum, tvb, offset, 2,
                                                  ENC_BIG_ENDIAN, &blocknum);
+    offset += 2;
 
     if (!PINFO_FD_VISITED(pinfo)) {
       blocknum = determine_full_blocknum(blocknum, tftp_info);
@@ -522,8 +459,12 @@ static void dissect_tftp_message(tftp_conv_info_t *tftp_info,
                              blocknum);
     proto_item_set_generated(ti);
 
+    bytes = tvb_reported_length_remaining(tvb, offset);
+    is_last_package = (bytes < tftp_info->blocksize);
+
     /* Sequence analysis on blocknums (first pass only) */
     if (!PINFO_FD_VISITED(pinfo)) {
+      tftp_info->last_package_available |= is_last_package;
       if (blocknum > tftp_info->next_block_num) {
         /* There is a gap.  Don't try to recover from this. */
         tftp_info->next_block_num = blocknum + 1;
@@ -533,13 +474,11 @@ static void dissect_tftp_message(tftp_conv_info_t *tftp_info,
       else if (blocknum == tftp_info->next_block_num) {
         /* OK, inc what we expect next */
         tftp_info->next_block_num++;
+        tftp_info->file_length += bytes;
       }
     }
-    offset += 2;
 
     /* Show number of bytes in this block, and whether it is the end of the file */
-    bytes = tvb_reported_length_remaining(tvb, offset);
-    is_last_package = (bytes < tftp_info->blocksize);
     col_append_fstr(pinfo->cinfo, COL_INFO, ", Block: %u%s",
                     blocknum,
                     is_last_package ?" (last)":"" );
@@ -590,64 +529,53 @@ static void dissect_tftp_message(tftp_conv_info_t *tftp_info,
     }
 
     /* If Export Object tap is listening, need to accumulate blocks info list
-       to send to tap. But if already know there are blocks missing, there is no
-       point in trying. */
-    if (have_tap_listener(tftp_eo_tap) && !tftp_info->blocks_missing &&
-        tftp_info->is_simple_file) {
-
-      if (blocknum == 1) {
-        /* Reset data for this conversation, freeing any accumulated blocks! */
-        cleanup_tftp_blocks(tftp_info);
-        tftp_info->next_tap_block_num = 1;
+       to send to tap. But we have a number of conditions for this.
+       */
+    if (have_tap_listener(tftp_eo_tap) &&
+        tftp_info->is_simple_file            /* This is a simple file */
+        && filename != NULL                  /* There is a file name */
+        && !tftp_info->blocks_missing        /* No missing blocks */
+        && tftp_info->last_package_available /* Last package known */
+    ) {
+
+      if (blocknum == 1 && !tftp_info->payload_data) {
+          tftp_info->payload_data = (guint8 *)g_try_malloc((gsize)tftp_info->file_length);
       }
 
-      if (blocknum != tftp_info->next_tap_block_num) {
-        /* Ignore.  Could be missing frames, or just clicking previous frame */
+      if (tftp_info->payload_data == NULL ||
+          (blocknum != tftp_info->next_tap_block_num)) {
+        /* Ignore. Not enough memory or just clicking previous frame */
         break;
       }
+      payload_data_offset =
+          (tftp_info->next_tap_block_num - 1) * tftp_info->blocksize;
 
-      if (bytes > 0) {
-        /* Create a block for this block */
-        GByteArray *block = g_byte_array_sized_new(bytes);
-        block->len = bytes;
-        tvb_memcpy(tvb, block->data, offset, bytes);
-
-        /* Add to the end of the list (does involve traversing whole list..) */
-        tftp_info->block_list = g_slist_append(tftp_info->block_list, block);
-        tftp_info->file_length += bytes;
-
-        /* Look for next blocknum next time */
-        tftp_info->next_tap_block_num++;
-      }
+      /* Copy data to its place in the payload_data */
+      tvb_memcpy(tvb, tftp_info->payload_data + payload_data_offset, offset,
+                 bytes);
+      tftp_info->next_tap_block_num++;
 
       /* Tap export object only when reach end of file */
-      if (bytes < tftp_info->blocksize) {
+      if (is_last_package) {
         tftp_eo_t        *eo_info;
 
-        /* If don't have a filename, won't tap file info */
-        if (filename == NULL) {
-            cleanup_tftp_blocks(tftp_info);
-            break;
-        }
-
         /* Create the eo_info to pass to the listener */
         eo_info = wmem_new(wmem_packet_scope(), tftp_eo_t);
 
         /* Set filename */
         eo_info->filename = g_strdup(filename);
 
-        /* Send block list, which will be combined and freed at tap. */
+        /* Set payload */
         eo_info->payload_len = tftp_info->file_length;
-        eo_info->pkt_num = blocknum;
-        eo_info->block_list = tftp_info->block_list;
+        eo_info->payload_data = tftp_info->payload_data;
 
         /* Send to tap */
         tap_queue_packet(tftp_eo_tap, pinfo, eo_info);
 
-        /* Have sent, so forget list of blocks, and only pay attention if we
+        /* Have sent, so forget payload_data, and only pay attention if we
            get back to the first block again. */
-        tftp_info->block_list = NULL;
         tftp_info->next_tap_block_num = 1;
+        tftp_info->payload_data = NULL;
       }
     }
     break;
@@ -736,11 +664,12 @@ tftp_info_for_conversation(conversation_t *conversation)
     tftp_info->next_block_num = 1;
     tftp_info->blocks_missing = FALSE;
     tftp_info->next_tap_block_num = 1;
-    tftp_info->block_list = NULL;
     tftp_info->file_length = 0;
     tftp_info->reassembly_id = conversation->conv_index;
     tftp_info->last_reassembly_package = G_MAXUINT32;
     tftp_info->is_simple_file = TRUE;
+    tftp_info->payload_data = NULL;
+    tftp_info->last_package_available = FALSE;
     conversation_add_proto_data(conversation, proto_tftp, tftp_info);
   }
   return tftp_info;
@@ -1035,7 +964,7 @@ proto_register_tftp(void)
     "Whether fragmented TFTP files should be reassembled", &tftp_defragment);
 
   /* Register the tap for the "Export Object" function */
-  tftp_eo_tap = register_export_object(proto_tftp, tftp_eo_packet, tftp_eo_cleanup);
+  tftp_eo_tap = register_export_object(proto_tftp, tftp_eo_packet, NULL);
 }
 
 void