Wireshark-dev: Re: [Wireshark-dev] Simplifying (and fixing) tvbuff [Long]
From: Bill Meier <wmeier@xxxxxxxxxxx>
Date: Mon, 12 Dec 2011 17:23:55 -0500
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;
- References:
- [Wireshark-dev] Simplifying (and fixing) tvbuff [Long]
- From: Bill Meier
- [Wireshark-dev] Simplifying (and fixing) tvbuff [Long]
- Prev by Date: [Wireshark-dev] Simplifying (and fixing) tvbuff [Long]
- Next by Date: Re: [Wireshark-dev] Simplifying (and fixing) tvbuff [Long]
- Previous by thread: [Wireshark-dev] Simplifying (and fixing) tvbuff [Long]
- Next by thread: Re: [Wireshark-dev] Simplifying (and fixing) tvbuff [Long]
- Index(es):