Wireshark · Wireshark-dev: Re: [Wireshark-dev] Simplifying (and fixing) tvbuff [Long]

[Bill Meier <wmeier@xxxxxxxxxxx>]

On 12/12/2011 4:55 PM, Bill Meier wrote:
> I've attached new versions of tvbuff-int.h and tvbuff.c with revised
> code for managing REAL_DATA and SUBSET tvb's. The changes are fairly
> simple. Wireshark seems to work AOK with the new versions (although I
> certainly haven't yet done extensive testing).

Attached are:

1.  a diff for tvbuff.h, tvbuff-int.h and tvbuff.h for my changes.

2. a diff file for other changes:
- replaced tvb_free with tvb_free-chain in epan/libwireshark.def
- replaced tvb_free() by tvb_free_chain() in:
  packet-cip.c
  packet-giop.c
  packet-http.c
  packet-kerberos.c
  epan/wslua/wslua_tvb.c
  plugins/asn1/packet-asn1.c

Index: epan/tvbuff.h
===================================================================
--- epan/tvbuff.h	(revision 40114)
+++ epan/tvbuff.h	(working copy)
@@ -86,47 +86,18 @@
  * require further initialization via the appropriate functions */
 extern tvbuff_t* tvb_new(tvbuff_type);
 
-/** Extracs from bit offset number of bits and 
+/** Extracts from bit offset number of bits and
  * Returns a pointer to a newly initialized tvbuff. with the bits
  * octet aligned.
  */
 extern tvbuff_t* tvb_new_octet_aligned(tvbuff_t *tvb, guint32 bit_offset, gint32 no_of_bits);
 
 
-/** Marks a tvbuff for freeing. The guint8* data of a TVBUFF_REAL_DATA
- * is *never* freed by the tvbuff routines. The tvbuff itself is actually freed
- * once its usage count drops to 0.
- *
- * Usage counts increment for any time the tvbuff is
- * used as a member of another tvbuff, i.e., as the backing buffer for
- * a TVBUFF_SUBSET or as a member of a TVBUFF_COMPOSITE.
- *
- * Although you may call tvb_free(), the tvbuff may still be in use
- * by other tvbuff's (TVBUFF_SUBSET or TVBUFF_COMPOSITE), so it is not
- * safe, unless you know otherwise, to free your guint8* data. If you
- * cannot be sure that your TVBUFF_REAL_DATA is not in use by another
- * tvbuff, register a callback with tvb_set_free_cb(); when your tvbuff
- * is _really_ freed, then your callback will be called, and at that time
- * you can free your original data.
- *
- * The caller can artificially increment/decrement the usage count
- * with tvbuff_increment_usage_count()/tvbuff_decrement_usage_count().
- */
-extern void tvb_free(tvbuff_t*);
-
-/** Free the tvbuff_t and all tvbuff's created from it. */
+/** Free the tvbuff_t and all tvbuff's created from it.
+ * Note: the tvbuff arg must be the _head_ of a chain (possibly with no children).
+ * Callbacks to free data will be invoked as each tvb in the chain is freed. */
 extern void tvb_free_chain(tvbuff_t*);
 
-/** Both return the new usage count, after the increment or decrement */
-extern guint tvb_increment_usage_count(tvbuff_t*, const guint count);
-
-/** If a decrement causes the usage count to drop to 0, a the tvbuff
- * is immediately freed. Be sure you know exactly what you're doing
- * if you decide to use this function, as another tvbuff could
- * still have a pointer to the just-freed tvbuff, causing corrupted data
- * or a segfault in the future */
-extern guint tvb_decrement_usage_count(tvbuff_t*, const guint count);
-
 /** Set a callback function to call when a tvbuff is actually freed
  * (once the usage count drops to 0). One argument is passed to
  * that callback --- a void* that points to the real data.
@@ -152,7 +123,10 @@
 extern void tvb_set_real_data(tvbuff_t*, const guint8* data, const guint length,
     const gint reported_length);
 
-/** Combination of tvb_new() and tvb_set_real_data(). Can throw ReportedBoundsError. */
+/** Combination of tvb_new() and tvb_set_real_data(). Can throw ReportedBoundsError.
+ * Normally, a callback to free the data should be registered using tvb_set_free_cb();
+ * when this tvbuff is freed, then your callback will be called, and at that time
+ * you can free your original data. */
 extern tvbuff_t* tvb_new_real_data(const guint8* data, const guint length,
     const gint reported_length);
 
Index: epan/tvbuff-int.h
===================================================================
--- epan/tvbuff-int.h	(revision 40114)
+++ epan/tvbuff-int.h	(working copy)
@@ -49,17 +49,15 @@
 } tvb_comp_t;
 
 struct tvbuff {
+	/* Doubly linked list pointers */
+	tvbuff_t                *next;
+	tvbuff_t                *previous;
+
 	/* Record-keeping */
 	tvbuff_type		type;
 	gboolean		initialized;
-	guint			usage_count;
 	struct tvbuff		*ds_tvb;  /**< data source top-level tvbuff */
 
-	/** The tvbuffs in which this tvbuff is a member
-	 * (that is, a backing tvbuff for a TVBUFF_SUBSET
-	 * or a member for a TVB_COMPOSITE) */
-	GSList			*used_in;
-
 	/** TVBUFF_SUBSET and TVBUFF_COMPOSITE keep track
 	 * of the other tvbuff's they use */
 	union {
Index: epan/tvbuff.c
===================================================================
--- epan/tvbuff.c	(revision 40114)
+++ epan/tvbuff.c	(working copy)
@@ -53,6 +53,19 @@
 #include "charsets.h"
 #include "proto.h"	/* XXX - only used for DISSECTOR_ASSERT, probably a new header file? */
 
+
+// ToDo: Determine how best to make stats visible when testing
+int tvbuff_real_allocated_count	         = 0;
+int tvb_new_child_real_data_count        = 0;
+int tvb_new_real_data_count              = 0;
+int tvb_set_child_real_data_tvbuff_count = 0;
+int tvbuff_subset_allocated_count        = 0;
+int tvbuff_composite_allocated_count     = 0;
+int tvbuff_real_freed_count              = 0;
+int tvbuff_subset_freed_count            = 0;
+int tvbuff_composite_freed_count         = 0;
+//
+
 static const guint8*
 ensure_contiguous_no_exception(tvbuff_t *tvb, const gint offset, const gint length,
 		int *exception);
@@ -66,23 +79,25 @@
 	tvb_backing_t	*backing;
 	tvb_comp_t	*composite;
 
+        tvb->previous		= NULL;
+        tvb->next		= NULL;
 	tvb->type		= type;
 	tvb->initialized	= FALSE;
-	tvb->usage_count	= 1;
 	tvb->length		= 0;
 	tvb->reported_length	= 0;
 	tvb->free_cb		= NULL;
 	tvb->real_data		= NULL;
 	tvb->raw_offset		= -1;
-	tvb->used_in		= NULL;
 	tvb->ds_tvb		= NULL;
 
 	switch(type) {
 		case TVBUFF_REAL_DATA:
 			/* Nothing */
+			tvbuff_real_allocated_count += 1;
 			break;
 
 		case TVBUFF_SUBSET:
+			tvbuff_subset_allocated_count += 1;
 			backing = &tvb->tvbuffs.subset;
 			backing->tvb	= NULL;
 			backing->offset	= 0;
@@ -90,6 +105,7 @@
 			break;
 
 		case TVBUFF_COMPOSITE:
+			tvbuff_composite_allocated_count += 1;
 			composite = &tvb->tvbuffs.composite;
 			composite->tvbs			= NULL;
 			composite->start_offsets	= NULL;
@@ -131,7 +147,7 @@
 {
 	tvbuff_t *sub_tvb = NULL;
 	guint32 byte_offset;
-	gint32 datalen, i; 
+	gint32 datalen, i;
 	guint8 left, right, remaining_bits, *buf;
 	const guint8 *data;
 
@@ -177,7 +193,7 @@
 	buf[datalen-1] &= left_aligned_bitmask[remaining_bits];
 
 	sub_tvb = tvb_new_child_real_data(tvb, buf, datalen, datalen);
-	
+
 	return sub_tvb;
 }
 
@@ -191,102 +207,66 @@
 	return tvb;
 }
 
-void
+static void
 tvb_free(tvbuff_t* tvb)
 {
-	tvbuff_t	*member_tvb;
 	tvb_comp_t	*composite;
-	GSList		*slist;
 
-	tvb->usage_count--;
+	DISSECTOR_ASSERT(tvb);
 
-	if (tvb->usage_count == 0) {
-		switch (tvb->type) {
-		case TVBUFF_REAL_DATA:
-			if (tvb->free_cb) {
-				/*
-				 * XXX - do this with a union?
-				 */
-				tvb->free_cb((gpointer)tvb->real_data);
-			}
-			break;
+	switch (tvb->type) {
+	case TVBUFF_REAL_DATA:
+		if (tvb->free_cb) {
+			/*
+			 * XXX - do this with a union?
+			 */
+			tvb->free_cb((gpointer)tvb->real_data);
+		}
+		tvbuff_real_freed_count += 1;
+		break;
 
-		case TVBUFF_SUBSET:
-			/* This will be NULL if tvb_new_subset() fails because
-			 * reported_length < -1 */
-			if (tvb->tvbuffs.subset.tvb) {
-				tvb_decrement_usage_count(tvb->tvbuffs.subset.tvb, 1);
-			}
-			break;
+	case TVBUFF_SUBSET:
+		/* Nothing */
+		tvbuff_subset_freed_count += 1;
+		break;
 
-		case TVBUFF_COMPOSITE:
-			composite = &tvb->tvbuffs.composite;
-			for (slist = composite->tvbs; slist != NULL ; slist = slist->next) {
-				member_tvb = slist->data;
-				tvb_decrement_usage_count(member_tvb, 1);
-			}
+	case TVBUFF_COMPOSITE:
+		composite = &tvb->tvbuffs.composite;
 
-			g_slist_free(composite->tvbs);
+		g_slist_free(composite->tvbs);
 
-			g_free(composite->start_offsets);
-			g_free(composite->end_offsets);
-			if (tvb->real_data) {
-				/*
-				 * XXX - do this with a union?
-				 */
-				g_free((gpointer)tvb->real_data);
-			}
-
-			break;
+		g_free(composite->start_offsets);
+		g_free(composite->end_offsets);
+		if (tvb->real_data) {
+			/*
+			 * XXX - do this with a union?
+			 */
+			g_free((gpointer)tvb->real_data);
 		}
 
-		if (tvb->used_in) {
-			g_slist_free(tvb->used_in);
-		}
+		tvbuff_composite_freed_count += 1;
+		break;
 
-		g_slice_free(tvbuff_t, tvb);
+	default:
+		DISSECTOR_ASSERT_NOT_REACHED();
 	}
-}
 
-guint
-tvb_increment_usage_count(tvbuff_t* tvb, const guint count)
-{
-	tvb->usage_count += count;
-
-	return tvb->usage_count;
+	g_slice_free(tvbuff_t, tvb);
 }
 
-guint
-tvb_decrement_usage_count(tvbuff_t* tvb, const guint count)
-{
-	if (tvb->usage_count <= count) {
-		tvb->usage_count = 1;
-		tvb_free(tvb);
-		return 0;
-	}
-	else {
-		tvb->usage_count -= count;
-		return tvb->usage_count;
-	}
-
-}
-
 void
 tvb_free_chain(tvbuff_t* tvb)
 {
-	GSList		*slist;
-
-	/* Recursively call tvb_free_chain() */
-	for (slist = tvb->used_in; slist != NULL ; slist = slist->next) {
-		tvb_free_chain( (tvbuff_t*)slist->data );
+	tvbuff_t *next;
+	DISSECTOR_ASSERT(tvb);
+	DISSECTOR_ASSERT(tvb->previous==NULL);
+	while (tvb) {
+		next=tvb->next;
+		tvb_free(tvb);
+		tvb  = next;
 	}
-
-	/* Stop the recursion */
-	tvb_free(tvb);
 }
 
-
-
 void
 tvb_set_free_cb(tvbuff_t* tvb, const tvbuff_free_cb_t func)
 {
@@ -296,10 +276,14 @@
 }
 
 static void
-add_to_used_in_list(tvbuff_t *tvb, tvbuff_t *used_in)
+add_to_chain(tvbuff_t *parent, tvbuff_t *child)
 {
-	tvb->used_in = g_slist_prepend(tvb->used_in, used_in);
-	tvb_increment_usage_count(tvb, 1);
+	DISSECTOR_ASSERT(parent && child);
+	child->next	= parent->next;
+	child->previous = parent;
+	if (parent->next)
+		parent->next->previous = child;
+	parent->next    = child;
 }
 
 void
@@ -309,7 +293,8 @@
 	DISSECTOR_ASSERT(parent->initialized);
 	DISSECTOR_ASSERT(child->initialized);
 	DISSECTOR_ASSERT(child->type == TVBUFF_REAL_DATA);
-	add_to_used_in_list(parent, child);
+	add_to_chain(parent, child);
+	tvb_set_child_real_data_tvbuff_count += 1;
 }
 
 static void
@@ -349,7 +334,7 @@
 	 * so its data source tvbuff is itself.
 	 */
 	tvb->ds_tvb = tvb;
-
+	tvb_new_real_data_count += 1;
 	return tvb;
 }
 
@@ -361,6 +346,7 @@
 		tvb_set_child_real_data_tvbuff (parent, tvb);
 	}
 
+	tvb_new_child_real_data_count += 1;
 	return tvb;
 }
 
@@ -516,7 +502,7 @@
 		tvb->reported_length	= reported_length;
 	}
 	tvb->initialized		= TRUE;
-	add_to_used_in_list(backing, tvb);
+	add_to_chain(backing, tvb);
 
 	/* Optimization. If the backing buffer has a pointer to contiguous, real data,
 	 * then we can point directly to our starting offset in that buffer */
@@ -603,7 +589,6 @@
 	DISSECTOR_ASSERT(tvb->type == TVBUFF_COMPOSITE);
 	composite = &tvb->tvbuffs.composite;
 	composite->tvbs = g_slist_append( composite->tvbs, member );
-	add_to_used_in_list(tvb, member);
 }
 
 void
@@ -615,9 +600,17 @@
 	DISSECTOR_ASSERT(tvb->type == TVBUFF_COMPOSITE);
 	composite = &tvb->tvbuffs.composite;
 	composite->tvbs = g_slist_prepend( composite->tvbs, member );
-	add_to_used_in_list(tvb, member);
 }
 
+// ToDo: Need version which adds composite tvb to chain.
+//       To keep things simple, member tvb's all need
+//        to be part of one chain and the composite tvb must
+//        be attached to that tvb.
+//        How might this be enforced ??
+//       Chain in at the finalize so that all members
+//        preceed the composite tvb in the chain.
+//        tvb_child_finalize_composite() ?
+//       Or: just chain to the first member tvb at finalization ?
 tvbuff_t*
 tvb_new_composite(void)
 {

Index: epan/libwireshark.def
===================================================================
--- epan/libwireshark.def	(revision 40114)
+++ epan/libwireshark.def	(working copy)
@@ -1094,7 +1094,7 @@
 tvb_find_tvb
 tvb_format_text
 tvb_format_text_wsp
-tvb_free
+tvb_free_chain
 tvb_get_bits_buf
 tvb_get_bits
 tvb_get_bits8
Index: epan/dissectors/packet-cip.c
===================================================================
--- epan/dissectors/packet-cip.c	(revision 40114)
+++ epan/dissectors/packet-cip.c	(working copy)
@@ -4874,7 +4874,7 @@
 
                preq_info->ciaData = se_alloc(sizeof(cip_simple_request_info_t));
                dissect_epath( tvbIOI, pinfo, pi, 0, preq_info->IOILen*2, TRUE, preq_info->ciaData);
-               tvb_free(tvbIOI);
+               tvb_free_chain(tvbIOI);
             }
          }
       }
Index: epan/dissectors/packet-giop.c
===================================================================
--- epan/dissectors/packet-giop.c	(revision 40114)
+++ epan/dissectors/packet-giop.c	(working copy)
@@ -1344,7 +1344,7 @@
       stream_is_big_endian = !get_CDR_octet(tvb,&my_offset);
       decode_IOR(tvb, NULL, NULL, &my_offset, 0, stream_is_big_endian);
 
-      tvb_free(tvb);
+      tvb_free_chain(tvb);
 
     }
 
Index: epan/dissectors/packet-http.c
===================================================================
--- epan/dissectors/packet-http.c	(revision 40114)
+++ epan/dissectors/packet-http.c	(working copy)
@@ -1493,7 +1493,7 @@
 			raw_len = tvb_length_remaining(new_tvb, 0);
 			tvb_memcpy(new_tvb, raw_data, 0, raw_len);
 
-			tvb_free(new_tvb);
+			tvb_free_chain(new_tvb);
 		}
 
 		tvb_memcpy(tvb, (guint8 *)(raw_data + raw_len),
Index: epan/dissectors/packet-kerberos.c
===================================================================
--- epan/dissectors/packet-kerberos.c	(revision 40114)
+++ epan/dissectors/packet-kerberos.c	(working copy)
@@ -910,7 +910,7 @@
             offset = get_ber_length(encr_tvb, id_offset, &item_len, &ind);
         }
         CATCH (BoundsError) {
-            tvb_free(encr_tvb);
+            tvb_free_chain(encr_tvb);
             do_continue = TRUE;
         }
         ENDTRY;
@@ -919,7 +919,7 @@
 
         data_len = item_len + offset - CONFOUNDER_PLUS_CHECKSUM;
         if ((int) item_len + offset > length) {
-            tvb_free(encr_tvb);
+            tvb_free_chain(encr_tvb);
             continue;
         }
 
@@ -932,7 +932,7 @@
 g_warning("woohoo decrypted keytype:%d in frame:%u\n", keytype, pinfo->fd->num);
             plaintext = g_malloc(data_len);
             tvb_memcpy(encr_tvb, plaintext, CONFOUNDER_PLUS_CHECKSUM, data_len);
-            tvb_free(encr_tvb);
+            tvb_free_chain(encr_tvb);
 
             if (datalen) {
                 *datalen = data_len;
Index: epan/wslua/wslua_tvb.c
===================================================================
--- epan/wslua/wslua_tvb.c	(revision 40114)
+++ epan/wslua/wslua_tvb.c	(working copy)
@@ -318,12 +318,12 @@
  * Tvb & TvbRange
  *
  * a Tvb represents a tvbuff_t in Lua.
- * a TvbRange represents a range in a tvb (tvb,offset,length) it's main purpose is to do bounds checking,
- *            it helps too simplifing argument passing to Tree. In wireshark terms this is worthless nothing
+ * a TvbRange represents a range in a tvb (tvb,offset,length). its main purpose is to do bounds checking;
+ *            it helps too, simplifing argument passing to Tree. In wireshark terms this is worthless nothing
  *            not already done by the TVB itself. In lua's terms is necessary to avoid abusing TRY{}CATCH(){}
  *            via preemptive bounds checking.
  *
- * These lua objects refers to structures in wireshak that are freed independently from Lua's garbage collector.
+ * These lua objects refers to structures in wireshark that are freed independently from Lua's garbage collector.
  * To avoid using a pointer from Lua to Wireshark's that is already freed, we maintain a list of the pointers with
  * a marker that track's it's expiry.
  *
@@ -357,7 +357,7 @@
         tvb->expired = TRUE;
     } else {
         if (tvb->need_free)
-            tvb_free(tvb->ws_tvb);
+            tvb_free_chain(tvb->ws_tvb);
         g_free(tvb);
     }
 }
Index: plugins/asn1/packet-asn1.c
===================================================================
--- plugins/asn1/packet-asn1.c	(revision 40114)
+++ plugins/asn1/packet-asn1.c	(working copy)
@@ -2922,9 +2922,7 @@
 	get_values();
 
 	g_node_destroy(asn1_nodes);	asn1_nodes = 0;
-#ifndef _WIN32		/* tvb_free not yet exported to plugins... */
-	tvb_free(asn1_desc);
-#endif
+	tvb_free_chain(asn1_desc);
 					asn1_desc = 0;
 	g_free(data);			data = 0;