Re: Return value of a new-style dissector

[Peter Wu <peter () lekensteyn nl>]

On Wednesday 25 June 2014 12:56:21 Evan Huus wrote:
> On Wed, Jun 25, 2014 at 12:54 PM, Evan Huus <eapache () gmail com> wrote:
>
> > On Wed, Jun 25, 2014 at 12:32 PM, Peter Wu <peter () lekensteyn nl> 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 () alum mit edu>
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

___________________________________________________________________________
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