Wireshark · Wireshark-dev: Re: [Wireshark-dev] reassemble.c changes for SCTP reassembly

[Robin <wireshark@xxxxxxxxxx>]

Hello again,
I just wanted to notice that I have found a better solution without 
modifying reassemble.c.

Regards,
Robin


Robin schrieb:
> Hey everyone,
> I'm currently working on the SCTP fragmented message reassembly and so I 
> require a few changes to the functions in reassemble.c to avoid multiple 
> functions doing the same thing with one line of code difference. SCTP 
> fragments are sequence number based so the changes only affect the 
> add_fragment_seq* and their following functions.
>
> In the following I'll brief on my intended changes so that you can 
> notice me if anything would affect any other protocol.
>
>
> A new function
>
> ----------------------
>
> fragment_data *
> fragment_add_seq_bsn(tvbuff_t *tvb, int offset, packet_info *pinfo, 
> guint32 id, GHashTable *fragment_table, GHashTable *reassembled_table, 
> guint32 frag_number, guint32 frag_data_len, gboolean more_frags, guint 
> start_bsn)
>
> ----------------------
>
> with the new parameter start_bsn to set the first bsn arbitrarily is 
> added. This value is passed to fragment_add_seq_check_work which also 
> got the new parameter. Every other function calling 
> fragment_add_seq_check_work uses 0 for the new parameter to act like 
> before. When fragment_add_seq_check_work calls fragment_add_seq_work it 
> passes the parameter again. In fragment_add_seq_work there is a check if 
> all fragments have been collected:
>
> ----------------------
>
> /* check if we have received the entire fragment
> * this is easy since the list is sorted and the head is faked.
> */
> max = start_bsn;
> for(fd_i=fd_head->next;fd_i;fd_i=fd_i->next) {
> 	if ( fd_i->offset==max ){
> 		max++;
> 	}
> }
> /* max will now be datalen+1 if all fragments have been seen */
>
> ----------------------
>
> The variable max is set to start_bsn instead of 0 as before. This should 
> be all what it needs.
>
> The next change is a bit more critical. Also in fragment_add_seq_work, 
> the fd_head->datalen value will be set different in this part:
>
> ----------------------
>
> if (!more_frags) {
> 	/*
> 	* This is the tail fragment in the sequence.
> 	*/
> 	if (fd_head->datalen) {
> 		/* ok we have already seen other tails for this packet
> 		* it might be a duplicate.
> 		*/
> 		if (fd_head->datalen != fd->offset ){
> 			/* Oops, this tail indicates a different packet
> 			* len than the previous ones. Somethings wrong
> 			*/
> 			fd->flags      |= FD_MULTIPLETAILS;
> 			fd_head->flags |= FD_MULTIPLETAILS;
> 		}
> 	} else {
> 		/* this was the first tail fragment, now we know the
> 		* sequence number of that fragment (which is NOT
> 		* the length of the packet!)
> 		*/
> 		max = fd->offset;
>
> 		/* is any higher offset already there?
> 		* this is important for SCTP to reassemble
> 		* packets not arriving in order
> 		*/
> 		for (fd_i=fd_head->next;fd_i;fd_i=fd_i->next) {
> 			if (fd_i->offset > max) max = fd_i->offset;
> 		}
> 			
> 		fd_head->datalen = max;
> 	}
> }
>
> ----------------------
>
> Formerly fd_head->datalen was just set to fd->offset, assuming the last 
> fragment appearing is the last in the sequence. SCTP fragments are not 
> inevitably in order, so if the last part of the message does not arrive 
> as the last part of the sequence the highest offset is already there. 
> fd_head->datalen has to be set to the highest available value to pass 
> the test if all fragments are already there, that is the code I posted 
> in the first part of this mail.
>
> There are lots of tests if the new fragment is correct, in order, the 
> last, whatever and in every case it is assumed that the last fragment 
> has to carry the highest sequence number unless the FD_DEFRAGMENTED bit 
> has been set and overlapping maybe found. My additions only set the 
> datalen value to the last offset if it's really the highest, but 
> regarding to the assumptions made elsewhere in other protocols that's 
> always the case.
>
> Please let me know if any of my changes would affect any other protocol 
> so I can use another parameter or anything else to keep the behavior of 
> other reassemblies.
>
> Regards,
> Robin
> _______________________________________________
> Wireshark-dev mailing list
> Wireshark-dev@xxxxxxxxxxxxx
> http://www.wireshark.org/mailman/listinfo/wireshark-dev