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

From: Bill Meier <wmeier@xxxxxxxxxxx>
Date: Wed, 21 Dec 2011 12:40:58 -0500
On 12/12/2011 4:55 PM, Bill Meier wrote:
Summary
-------

I've recently been digging into the tvbuff code.

After doing so, I've come to the conclusion that the current tvbuff
implementation of managing tvb's with usage_counts and used_in lists:
- doesn't really provide some of what I understand to be
the intended functionality;
- is not necessary given the ways that Wireshark actually uses tvbuffs;
- can be significantly simplified by doing away with usage counts
and used_in lists.

[snip]

Proposal
--------

>
> [snip]

Ok:

I've committed new versions of tvbuff.h, tvbuff-int.h and tvbuff.c which implement the proposal as described previously. (See below for one difference).

The changes simplify the code and should minimize if not eliminate
tvb leakage (other than for tvbs created by dissectors via tvb_new_real_data()).

I've done a fair amount of testing, but, obviously, "the proof of the pudding is in the eating" so we'll see how things go.

ToDo:
 - Add/update info about dissector tvb usage to doc/README.developer.
 - Update epan/tvbtest.c
 - Test composite tvbs (They might now even work).
 - ???

-----------
From the commit log:

A simplified version of tvbuffs:
- Essentially no changes from current dissector de facto tvbuff usage;
- Do away with 'usage_counts' and with 'used_in' GSLists;
- Manage tvb chains via a simple doubly linked list.
- API changes:
  a. tvb_increment_usage_count() and tvb_decrement_usage_count() no
     longer exist;
  b. tvb_free_chain() can only be called for the 'top-level' (initial)
     tvb of a chain) or for a tvb not in a chain.
  c. tvb_free() now just calls tvb_free_chain() [should have no impact
     on existing  dissectors].

--------
Note: The only difference from the original proposal is that tvb_free() is still callable from a dissector; tvb_free() now just calls tvb_free_chain().

This was done so that existing dissectors calling tvb_free() need not be changed.

(I'd argue that dissectors actually should have been calling tvb_free_chain() for tvbs created using tvb_new_real_data() since that handles the case wherein any subsets, etc had been created on the tvb).

In any case, I believe having tvb_free() just calling tvb_free_chain() should not impact dissectors which use tvb_free().


Bill