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

[Evan Huus <eapache@xxxxxxxxx>]

Re-replying to -dev (oops)

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.

 

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

 

 */
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?

 

Kind regards,
Peter