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
