Wireshark-dev: Re: [Wireshark-dev] TCP reassembly and Return value of a new-style dissector
-----Original Message-----
From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Peter Wu
Sent: den 10 december 2014 17:17
To: wireshark-dev@xxxxxxxxxxxxx
Subject: Re: [Wireshark-dev] TCP reassembly and Return value of a new-style dissector
On Tuesday 09 December 2014 21:01:37 Anders Broman wrote:
>> Hi,
>> I have recently come across some problems with reassembly of SIP
>> messages over TCP. One problem seems to be related to when a segment
>> contains one full PDU and a segment of the next (following) PDU (in
>> this case the first SIP line of the following PDU) is not complete.
>
>(added punctuation for easier reading)
>
>HTTP does not seem to have problems with it, probably because it tries to consume as much of a PDU as possible, saving incomplete data for later desegmentation. See the attached crafted packet for example >which contains three HTTP responses:
>
>1. PDU 1 contains one full HTTP response and the begin of the next.
>2. PDU 2 contains all the remainder of the second HTTP response, minus
> one last character.
> 3. PDU 3 has the last character of the second request and the begin of
> the third request .
> 4. PDU 4 contains the remainder of the third HTTP request.
>
>'HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 1\r\n\r\n1'
>'HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\n', 'Content-Length: 2\r\n\r\n1', '2'
>'HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nC', 'ontent-Length: 3\r\n\r\n123'
>> I think the ultimate solution would be for the TCP dissector to call
>> the SIP dissector again with the next incomplete PDU after receiving
>> the number of bytes "accepted" by the SIP dissector e.g using the
>> "new-style dissector interface.
>> also see http://seclists.org/wireshark/2014/Jun/289
>It would indeed be nice if the dissector core could be improved to handle this situation. As I mentioned before, the HTTP dissector has no problems with this because it tries to dissect all data until more data is >needed.
>This is from dissect_sip:
>
> remaining_length = tvb_reported_length(tvb);
> len = dissect_sip_common(tvb, 0, remaining_length, pinfo, tree, FALSE, FALSE);
> if (len < 0)
> return 0; /* not SIP */
> else
> return len;
>dissect_http instead always returns tvb_captured_length ("all data in the PDU is part of the HTTP protocol") while returning 0 means "I know for sure that this is not SIP".
>
>dissect_sip_common returns -1 meaning "need more data", but then dissect_sip return 0 meaning "no, not SIP, try a different dissector!".
I'm not sure the TCP dissector cares about the return value I think it only looks at pinfo.
I think part of my problem is that the SIP dissector mixes the heuristics and 'common' functionality. I'm actually looking at SIP over TCP currently, e.g dissect_sip_tcp(). I have also run into a problem where reassembly fails
When there is duplicated packets but works if the dupes are marked as ignored... ( dupes due to the way mirroring is set up).
> As I read the code the first step would be to have
> call_dissector() [OK]
> try_conversation_dissector()
> dissector_try_heuristic()
> dissector_try_uint_new [OK]
>
> Return the number of bytes consumed, 0 or -1(need more data) not sure
> about DESEGMENT_UNTIL_FIN (-2?).
>
> If people agree the biggest change is to change
> dissector_try_heuristic() to return an int.
> What do you think?
I would not particularly mind changing the return value, but what should be the new semantics of the return value? I found that documentation is quite lacking here. When I tried to make the core to actual handle return values[1], many dissectors broke because they were not written with the documented behavior (see also the mail you linked before).
Here is a document I wrote back then to clarify it for myself https://git.lekensteyn.nl/peter/wireshark-notes/tree/doc/dissector.txt
--
Kind regards,
Peter
https://lekensteyn.nl
[1]: https://git.lekensteyn.nl/peter/wireshark/commit/?h=reassembly-fixes&id=aef08cc2434b5ba5aee4422fbcf481004c62583a
