Wireshark · Wireshark-dev: Re: [Wireshark-dev] Use of tcp_dissect_pdus() with a protocol which has a variable length "PDU lengt

[Peter Wu <peter@xxxxxxxxxxxxx>]

Hi,

While looking at improving the Websocket dissector, I found the need to
support variable length fields in tcp_dissect_pdus. Here is Bills
original mail (which had no replies).

On Fri, May 09, 2014 at 11:02:45AM -0400, Bill Meier wrote:
> To: TCP re-assembly experts:
> 
> 
> The MQTT protocol dissected by packet-mqtt.c runs over TCP. The field which
> specifies the MQTT PDU length can be 1 to 4 bytes; the length of a complete
> MQTT PDU can be less than 4 bytes.
> 
> So: trying use tcp_dissect_pdus() won't work since the "fixed length"
> needed to handle the maximum size of the "PDU length" field is larger than
> the possible minimum PDU size.
> 
> One approach is to assume that the complete length field will, with high
> probability, always be in 1 TCP segment and thus available even if
> specifying a 'fixed length' which includes only a 1 byte PDU length field.
> (This is certainly not guaranteed).
> 
> Or, obviously, the dissector could do reassembly as described in
> README.dissector section 2.7.2 "Modifying the pinfo struct".
> 
> However, digging a little deeper, I note that tcp_dissect_pdus() is doing
> something related to "want_pdu_tracking" (which I've never delved into and
> which is not mentioned in README.dissector).
> 
> So it occurred to me that using a modified tcp_dissect_pdus() which allows
> the get_pdu_len function to return DESEGMENT_ONE_MORE_SEGMENT might be a
> workable approach.
> 
> So: I added the following to tcp_dissect_pdus() and modified
> the packet-mqtt.c get_pdu_len() function appropriately.
> 
> (added code in tcp_dissect_pdus):
> 
>          plen = (*get_pdu_len)(pinfo, tvb, offset);
> +
> +        /* Is "more data" being requested before the PDU length can be
>             determined ?
> +         *  If so, request same.
> +         */
> +        if (plen == DESEGMENT_ONE_MORE_SEGMENT) {
> +            if (!proto_desegment || !pinfo->can_desegment) {
> +                REPORT_DISSECTOR_BUG("DESEGMENT_ONE_MORE_SEGMENT not
> allowed");
> +            }
> +            pinfo->desegment_offset = offset;
> +            pinfo->desegment_len = DESEGMENT_ONE_MORE_SEGMENT;
> +            return;
> +        }
> +
>          if (plen < fixed_len) {
> 
> 
> My questions:
> 
> 1. Is this a reasonable approach (it works AOK in my tests) ?

This approach looks fine to me. I have taken the same approach (but
replacing REPORT_DISSECTOR_BUG by DISSECTOR_ASSERT in
https://code.wireshark.org/review/7279

> 2. If not, and packet-mqtt should do reassembly itself, does the reassembly
> code also need to do the "want_pdu_tracking" stuff ?
> 
> Bill

Looking at the current implementation of tcp_dissect_pdus, it is not
necessary to change want_pdu_tracking as the size of the new PDU is not
yet known.

Since this is an interesting API update, I thought that informing the
list would be a good idea.

Kind regards,
Peter