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;
 
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev@wireshark.org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Reply via email to