Re-replying to -dev (oops) On Wed, Jun 25, 2014 at 12:54 PM, Evan Huus <[email protected]> wrote:
> On Wed, Jun 25, 2014 at 12:32 PM, Peter Wu <[email protected]> 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 >> >> >
___________________________________________________________________________ Sent via: Wireshark-dev mailing list <[email protected]> Archives: http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:[email protected]?subject=unsubscribe
