Simplifying (and fixing) tvbuff [Long]

[Bill Meier <wmeier () newsguy com>]

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.


(Comment: I started digging into the tvbuff code while reviewing the 
patch provided with Bug #6573:
  "tvbuff_t values leak if they have a child tvbuff_t (memory leak)".
The patch does, in fact, improve matters significantly but the patched 
tvbuff still has issues).

Below I give a few examples of issues related to the current tvbuff 
implementation and then describe how tvbuff can be simplified.

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).

I think the revised code:
- provides the functionality required by Wireshark;
- is simpler and easier to understand.

Comments are welcome !

Bill


Some current tvbuff issues
--------------------------

Based upon comments in tvbuff.h[1] and in epan.c[2] it seems a design 
goal for the tvbuff code was to allow a dissector to do things like the 
following:

a. Do tvb_subset_new() and then, when done with the subset, tvb_free() ;

b. Increment the usage count of a tvb which the dissector wishes
   to "save for later use".

Neither of the above sequences currently work properly (and thus are not 
currently actually used in Wireshark). (See [3] below for some details).

c. The following sequence will fail:
   x1 = tvb_new_real_data();
   x2 = tvb_new_child_real_data();
   tvb_free(x2);
   tvb_free_chain(x1);

   issue: For REAL_DATA there's no "back-pointer" in x2
          pointing to x1 and thus the tvb_free() can't
          clean the "used_in" list for x1.

d. composite tvbuff's don't currently work (with respect to
   usage-counts, used-in lists and tvb_free()).
   The patch with bug #6573 certainly improves matters, but my brain
   fries when I try to work thru all the scenarios.

I expect there are more issues which I didn't see or have forgotten as I 
worked through the code.


Proposal
--------

Essentially:

1. API:
   Any new tvbs created by a dissector which are based upon or
   chained to a tvb handed to the dissector should not (must not)
   be free'd or "saved for later use" by the dissector.
   (AFAIKT Wireshark dissectors currently work this way so
   no dissector changes are required). (See Note below).

   A dissector can create a new tvb using tvb_new_real_data()
   with data storage allocated by the dissector.
   - Since the tvb is owned and managed by the dissector (and not
     chained to any tvb handed to the dissector), the dissector
     can do whatever is desired with that tvb (e.g., save the tvb for
     later use when dissecting another frame).
   - The dissector must eventually free the tvb
     (and possibly the storage) via a call to tvb_free_chain() for
     that tvb.

   Note: - tvb_free_chain() is the only way to free tvb's.
           [Existing dissector calls to tvb_free() can simply
           be replaced by calls to tvb_free_chain()].

         - tvb_free_chain must be called only for the tvb which
           is the first tvb in the chain.
           [XXX: It may be that it will turn out to be OK to allow
           a dissector to do a tvb_free_chain() for any tvb
           created by that dissector (even if based upon or
           chained to a tvb handed to the dissector).

2. Implementation:
   a. Do away with usage_counts and the used_in lists altogether;
      All the existing tvb_new..() functions work pretty much as is
      except they don't try to maintain usage_counts &
      used_in lists & etc.

   b. tvb's (real, subset & composite) are "chained" using a simple
      doubly-linked list (maintained via new tvbuff_t *pointers in
      the tvb struct ('next' and 'previous').

      (A composite tvb should always be part of a chain so that it
      eventually gets free'd).
      [XXX:
        - This means that all members of a composite must
            be part of the same chain as the composite.
      - One way to add a composite tvb to the chain:
        tvb_child_finalize_composite() or some thing similar.
        TBD].

    c. tvb_free_chain() just goes down the list freeing each tvb in
       turn. The only special actions are to exec the call-back function
       for REAL_DATA tvb's and to clear the COMPOSITE GSLISTs. No
       special action is required for SUBSET tvb's.

      [XXX: If it's possible to guarantee that any
            subset/composite tvb in the list only references tvbs
            earlier in the list then it should be OK to allow
            free_chain() to start at any place in the list].

Notes:

[1]  From tvbuff.h:
  * The caller can artificially increment/decrement the usage count
 * with tvbuff_increment_usage_count()/tvbuff_decrement_usage_count().

[2]  From packet.h
        /* Free all tvb's created from this tvb, unless dissector
         * wanted to store the pointer (in which case, the dissector
         * would have incremented the usage count on that tvbuff_t*) */
        tvb_free_chain(edt->tvb);

[3]
a. tvb_free() of a subset doesn't remove the tvb from the parent 
"used_in" list and thus a subsequent tvb_free_chain() fails.
(The patch for bug #6573 added some code to fix this).

b. In general, the sequence tvb_increment_usage_count(tvb)/tvb_free(tvb) 
as implemented does work correctly.

However: Note that most likely a tvb handed to dissector is ultimately 
based upon a "top-level" real tvb which is based, AFAIKT, on a data 
buffer filled in by wiretap (which will be overwritten when the next 
frame is read).

So: trying to "keep around" a tvb in such a case won't actually work
    as intended. The tvb needs to be copied if it is to be saved.

Attachment: tvbuff.c
Description:

Attachment: tvbuff-int.h
Description:

___________________________________________________________________________
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-request () wireshark org?subject=unsubscribe