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
