Re: Return value of a new-style dissector

[Peter Wu <peter () lekensteyn nl>]

On Wednesday 25 June 2014 18:32:44 Peter Wu 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;
>  *
>  *    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.
>  */
> 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)`)

For DESEGMENT_UNTIL_FIN:

 - Return 0. See finger, ldss, whois.

desegment_len seems to be invented for use in and by dissectors, the packet API
does not use it.

Proposal: make the API return gboolean rather than an int?
Alternative: use negative values to set DESEGMENT_ONE_MORE_SEGMENT (if
desegment_len is unset). Smells like bad API design though, having two
ways to do the same thing. It may also cause breakage in unobvious ways,
existing dissectors that return the wrong value or previously set
desegment_len values?

So, back to the first proposal, what about changing to return a boolean?

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