[Pavel Karneliuk <Pavel_Karneliuk@xxxxxxxx>]

Hi Pascal,

thank you for answer. I saw your commits to follow.c and I hoped for your reply.

450:if( newseq > seq[idx] ) {

I think – Yes. It compares sequence numbers.

459: if ( current->data_len > new_pos ) {
I am sure,  that – No. Because it compares length of data from fragment instead of sequence numbers.

There are some places in check_fragments() and reassemble_tcp() with a “naive” comparison of sequence numbers:
369: if( sequence < seq[src_index] ) {

 

I think, they should be replaced with macros from packet-tcp.h 51-55.  At least to be uniformly.

 

Best Regards,
Pavel Karneliuk
Senior Software Engineer

From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Pascal Quantin
Sent: Friday, March 28, 2014 6:14 PM
To: Developer support list for Wireshark
Subject: Re: [Wireshark-dev] Defect in reassembling TCP stream. Bug and Patch are available on Bugzilla.

 

2014-03-28 16:06 GMT+01:00 Pavel Karneliuk <Pavel_Karneliuk@xxxxxxxx>:

Hello,

At first, thank you all for Wireshark. It is amazing tool!

I found a defect and register Bug 9936 – “epan/follow.c - Incorrect comparing a sequence number of TCP fragment when its value wraps over uint32_t limit”
A capture file and my patch are attached to bug in Bugzilla.

Patch is a one-line fix:

--- a/epan/follow.c

+++ b/epan/follow.c

@@ -441,7 +441,7 @@ check_fragments( int idx, tcp_stream_chunk *sc, guint32 acknowledged ) {

         lowest_seq = current->seq;

       }

-      if( current->seq < seq[idx] ) {

+      if( LT_SEQ(current->seq, seq[idx]) ) {

         guint32 newseq;

         /* this sequence number seems dated, but

            check the end to make sure it has no more

 

It is just a replacement a compare operator to wraps-friendly macro. Similar to code around (with GT_SEQ usage).
What do you think?

 

Hi Pavel,

while we are at it, shouldn't the comparison done at lines 450 and 459 be wrapped in a GT_SEQ macro also?

Regards,
Pascal.