Re: [Wireshark-dev] Defect in reassembling TCP stream. Bug and Patch are available on Bugzilla.
Hi Pavel, Can you use the revised workflow and submit the change to Gerrit. Any discussion on the proposed change will take place in the Gerrit Review process. Workflow info can be found here: http://wiki.wireshark.org/Development/Workflow On 28 March 2014 15:06, Pavel Karneliuk pavel_karnel...@epam.com wrote: Hello, At first, thank you all for Wireshark. It is amazing tool! I found a defect and register Bug 9936https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=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? Best Regards, *Pavel Karneliuk* Senior Software Engineer EPAM Systems Minsk office, Belarus ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org ?subject=unsubscribe ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Defect in reassembling TCP stream. Bug and Patch are available on Bugzilla.
Hello, At first, thank you all for Wireshark. It is amazing tool! I found a defect and register Bug 9936https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=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? Best Regards, Pavel Karneliuk Senior Software Engineer EPAM Systems Minsk office, Belarus ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] Defect in reassembling TCP stream. Bug and Patch are available on Bugzilla.
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-boun...@wireshark.org [mailto:wireshark-dev-boun...@wireshark.org] 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_karnel...@epam.commailto:pavel_karnel...@epam.com: Hello, At first, thank you all for Wireshark. It is amazing tool! I found a defect and register Bug 9936https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=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. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
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_karnel...@epam.com: Hello, At first, thank you all for Wireshark. It is amazing tool! I found a defect and register Bug 9936https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=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. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] Defect in reassembling TCP stream. Bug and Patch are available on Bugzilla.
Yes, I am going to gerrit. From: wireshark-dev-boun...@wireshark.org [mailto:wireshark-dev-boun...@wireshark.org] On Behalf Of Pascal Quantin Sent: Friday, March 28, 2014 6:57 PM To: Developer support list for Wireshark Subject: Re: [Wireshark-dev] Defect in reassembling TCP stream. Bug and Patch are available on Bugzilla. Le 28 mars 2014 16:52, Pavel Karneliuk pavel_karnel...@epam.commailto:pavel_karnel...@epam.com a écrit : 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. Doh that's what happens when you reply without looking carefully at the code ;) 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. As Graham suggested, it would be great if you could submit a patch on gerrit against master branch. Would it be feasible? Regards, Pascal. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] Defect in reassembling TCP stream. Bug and Patch are available on Bugzilla.
Le 28 mars 2014 16:52, Pavel Karneliuk pavel_karnel...@epam.com a écrit : 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. Doh that's what happens when you reply without looking carefully at the code ;) 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. As Graham suggested, it would be great if you could submit a patch on gerrit against master branch. Would it be feasible? Regards, Pascal. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe