| Bug ID |
9112
|
| Summary |
Incorrect "bytes missing in capture file" in "check_fragments" due to an unsigned int wraparound?
|
| Classification |
Unclassified
|
| Product |
Wireshark
|
| Version |
SVN
|
| Hardware |
All
|
| OS |
All
|
| Status |
UNCONFIRMED
|
| Severity |
Major
|
| Priority |
Low
|
| Component |
Dissection engine (libwireshark)
|
| Assignee |
[email protected]
|
| Reporter |
[email protected]
|
| Attachment #11518 Flags |
review_for_checkin?
|
Created attachment 11518 [details]
patch to avoid wraparound in 'unsigned int minus unsigned int' in
check_fragments
Build Information:
SVN 51384, but it does apply to all versions, I think.
--
I dont understand the intended behaviour of this line in epan/follow.c:
if( (glong)(acknowledged - lowest_seq) > 0 )
i.e.:
in this check for missing frames, if acknowledged < lowest_seq
we have modular arithmetic [wraparound] on these unsigned ints, giving a value
> 0,
so I think that in this case its possible that wireshark thinks that there are
missing frames and does return error on this frame, actually losing it, when
instead its here.
Moreover, the wraparound doesnt happen with 32bit compile, having the
subtraction a negative value.
[maybe because of this? http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55771]
['printf("%li\n", (long)((unsigned int)(1) - (unsigned int)(2)));' gives '-1'
with -m32, and '4294967295' with -m64,
and 'if( (long)((unsigned int)(1) - (unsigned int)(2)) > 0)' is false with
-m32, true with -m64]
Anyway, I've tested the patch with some capture files [that I cant share
because they contains sensitive data],
both on wireshark and tshark, and it works:
without the patch, the "follow tcp stream" feature reports '[NNN bytes missing
in capture file]' and there is actually
a missing chunk; with the patch applied, all the data are correctly parsed and
reassembled.
Just to add confusion, my capture files where this happens have HTTP server
gzipped responses,
so, in wireshark, I've corrected the bug 9044 ['"Follow TCP Stream" show only
first HTTP req+res ']
by quickly&dirtily changing the 2 'if (gunzip)' in ui/gtk/follow_tcp.c with
'if(2 == 3)',
just to have all the stream.
Btw, about this bug [9044] I absolutely agree with Sake, the 'follow TCP
stream' option
should at least give the user the choice of not decompressing the stream.
A final note: in epan/follow.c there are also 2 other places where this
happens:
'if(data_length > 0 && ((glong)(sequence - seq[src_index]) > 0) ) {'
and
'if( (glong)(lowest_seq - current->seq) > 0 ) {'
I dont know if the intended behaviour was to take into account the wraparound
or not..
You are receiving this mail because:
- You are watching all bug changes.
