Wireshark · Wireshark-dev: Re: [Wireshark-dev] Return value of a new-style dissector

[Peter Wu <peter@xxxxxxxxxxxxx>]

On Wednesday 25 June 2014 12:56:21 Evan Huus wrote:
> On Wed, Jun 25, 2014 at 12:54 PM, Evan Huus <eapache@xxxxxxxxx> wrote:
> 
> > On Wed, Jun 25, 2014 at 12:32 PM, Peter Wu <peter@xxxxxxxxxxxxx> wrote:
> >
> >> Hi,
> >>
> >> Since Pascal's change ("TCP: do desegmentation sanity checks for all sub
> >> dissectors types"), the whois dissector was starting to throw:
> >>
> >> Dissector bug, protocol TCP, in packet 6:
> >> epan/dissectors/packet-tcp.c:3953:
> >> failed assertion "save_desegment_offset == pinfo->desegment_offset &&
> >> save_desegment_len == pinfo->desegment_len"
> >>
> >> That came from this part:
> >>
> >>     pinfo->desegment_len = DESEGMENT_UNTIL_FIN;
> >>     pinfo->desegment_offset = 0;
> >>     return (0);
> >>
> >> It is likely supposed to mean "well, this packet is mine, please give all
> >> data
> >> until the connection is closed". The `return 0` is clearly wrong here. But
> >> what is the correct value?
> >>
> >> From epan/packet.h:
> >> /*
> >>  * Dissector that returns:
> >>  *
> >>  *      The amount of data in the protocol's PDU, if it was able to
> >>  *      dissect all the data;
> >>
> >
> > I think the above line is misleading, it should say "The amount of data in
> > the protocol's PDU, if the tvbuff contains a PDU for that protocol". So
> > tvb_captured_length is the right return value for whois, because it is the
> > amount of data (so far) claimed by the whois dissector.

Some dissectors (HTTP iirc) do repeated dissections, so this is more accurate:
"The amount of data in tvbuff that could form one or more PDUs"

> >>  *      0, if the tvbuff doesn't contain a PDU for that protocol;
> >>  *
> >>  *      The negative of the amount of additional data needed, if
> >>  *      we need more data (e.g., from subsequent TCP segments) to
> >>  *      dissect the entire PDU.
> >>
> >
> > If the tcp dissector respects this (what happens if return is negative
> > *and* pinfo->desegment_len is set???) then it should maybe do this instead
> > of setting desegment_len at all...
> >
> 
> Does this mean we can get rid of the pinfo->desegment stuff once all
> dissectors are new-style?

Yes. Return -1 if you need *at least* 1 byte (like DESEGMENT_ONE_MORE_SEGMENT,
but smarter since it can then count before calling the dissector). Return
DESEGMENT_UNTIL_FIN if you want to have everything until the last byte.
Note: it should then not return tvb_captured_length(tvb).

> >  */
> >> typedef int (*new_dissector_t)(tvbuff_t *, packet_info *, proto_tree *,
> >> void *);
> >>
> >> Almost all callers of call_dissector... ignore its return value, those who
> >> care seem only to be interested in a boolean (zero vs non-zero). The
> >> dissectors
> >> which use DESEGMENT_... do arbitrary things:
> >>
> >>  - Return 0, this is just a plain error. See whois, finger, snmp,
> >> alljoyn, ms-mms.
> >>  - Return tvb_length(tvb). See nbns
> >>  - Return -pinfo->desegment_len. See ldp.
> >>  - Return -1. See sigcomp, rtsp
> >>  - Return -2. See fcip.
> >>  - Return offset. See ssh, ssl
> >>  - Return -DESEGMENT_ONE_MORE_SEGMENT (!!). jxta
> >>  - Return the actual bytes that is wanted (offset_next - offset_before).
> >> See xot.
> >>
> >> (Found with `view -p $(grep -rnl DESEGMENT_ONE_MORE_SEGMENT)`)
> >>
> >> This suggests that something is wrong in the API definition. As it stands
> >> now,
> >> it really needs a boolean. Can someone clarify this? Is this a bug, an
> >> incomplete migration from the old-style dissector or a documentation
> >> issue?
> >> Did I misunderstood something?
> >>
> >
> > I'm not sure, I didn't write the new API. I think incomplete migration?

Ok, forget about the boolean. It would be really nice if desegment_len could
be dropped by fixing the return value.

Guy Harris introduced the new_dissector_t type according to git, the intent
was good, but the implementation is incomplete.

commit 193b8c9bfbd15afde08076ee1dc09abf914b9abe
Author: Guy Harris <guy@xxxxxxxxxxxx>
Date:   Tue Feb 26 11:55:39 2002 +0000

    Allow dissectors to be registered as "old-style" or "new-style"
    dissectors.  "Old-style" dissectors return nothing.  "New-style"
    dissectors return one of:
    
        a positive integer, giving the number of bytes worth of data in
        the tvbuff that it considered to be part of the PDU in the
        tvbuff;
    
        zero, if it didn't consider the data in the tvbuff to be a PDU
        for its protocol;
    
        a negative integer, giving the number of additional bytes worth
        of data in needs to get the complete PDU (for use with
        fragmentation/segmentation when the length of the PDU isn't
        known to the protocol atop the one the dissector is dissecting).
    
    Have "call_dissector()" return the return value of new-style dissectors,
    and the length of the tvbuff handed to it for old-style dissectors.
    
    Have "dissector_try_port()" return FALSE if the subdissector is a
    new-style dissector and returned 0.
    
    Make the EAP dissector a new-style dissector, and have a "EAP fragment"
    dissector that is also a new-style dissector and handles fragmentation
    of EAP messages (as happens above, for example, RADIUS).  Also, clean up
    some signed vs. unsigned comparison problems.
    
    Reassemble EAP-Message AVPs in RADIUS.
    
    svn path=/trunk/; revision=4811


Kind regards,
Peter