ANNOUNCEMENT: Live Wireshark University & Allegro Packets online APAC Wireshark Training Session
April 17th, 2024 | 14:30-16:00 SGT (UTC+8) | Online

Wireshark-dev: [Wireshark-dev] reassembly again

From: Martin Kaiser <lists@xxxxxxxxx>
Date: Wed, 10 Aug 2011 22:57:51 +0200
Dear all,

earlier today, I revoked my pending DVB-CI patch for spdu reassembly as
I found a problem :-(

I now think I know what happens, I'd appreciate your opinion how to deal
with it properly:

a tdpu looks like

header | body | trailer

Depending on header info, the body may contain a fragment of an spdu.
body_len is (no surprise) the body's length.

I go to the beginning of the body and do the following for each and
every message

        frag_msg = fragment_add_seq_next(tvb, offset, pinfo,
                SEQ_ID_TRANSPORT_LAYER,
                spdu_fragment_table,
                spdu_reassembled_table,
                body_len,
                hdr_tag == T_DATA_MORE ? 1 : 0);

-> I put the current body into the fragementation mechanism.

        payload_tvb = process_reassembled_data(tvb, offset, pinfo,
                "Reassembled SPDU", frag_msg, &tpdu_frag_items,
                NULL, trans_tree);

And see if things can be reassembled.

If multiple fragments were reassembled, things work ok. payload_tvb contains
(body_len #1 + ... + body_len #n) bytes.

The problem occurs when there's no fragments, i.e. the message can be
"reassembled" straight away. In this case, payload_tvb contains 
body | trailer !

To me this is quite surprising, I put in just the body and get body |
trailer in return whereas usually, I get body1|body2|...|bodyN
reassembled without any trailers (as expected).

The code that causes this is in process_reassembled_data(),
epan/reassemble.c

      } else {
         /*
          * No.
          * Return a tvbuff with the payload.
          */
         next_tvb = tvb_new_subset_remaining(tvb, offset);


Should this be changed to something like 
tvb_new_subset(tvb, <original offset>, <original len>, <orignal len>)?

with the values that we supplied to fragment_add_seq_next()?


(Needless to say that if this needs to be changed, I'm happy to look into
it.)


Thanks for your advice,


   Martin