Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 45566: /trunk/epan/dissectors/ /trun
Maynard, Chris wrote:
-----Original Message-----
From: wireshark-dev-bounces@xxxxxxxxxxxxx [mailto:wireshark-dev-
bounces@xxxxxxxxxxxxx] On Behalf Of Martin Kaiser
Sent: Thursday, October 18, 2012 5:11 PM
To: wireshark-dev@xxxxxxxxxxxxx
Subject: Re: [Wireshark-dev] [Wireshark-commits] rev 45566:
/trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-rdp.c
Hi,
Thus wrote Maynard, Chris (Christopher.Maynard@xxxxxxxxx):
Recently, I found and fixed some of these problems, but obviously I
didn't catch them all. Are there any more thoughts about changing
tvb_length_remaining() and tvb_reported_length_remaining() to return
0
instead of -1?
it looks like there's quite a few places where people used an unsigned
return value (I just fixed a few obvious cases).
I guess we should do something about this in the tvb part rather than
in the dissectors.
What's the difference between return value 0 and -1 now? Both are
essentially saying there's no data left, -1 is an error case and 0
isn't? Is that significant to the caller, what can he do other than
stop reading?
Martin
That's my understanding from the code; 0 means no more data and -1 means an invalid offset. But I was wondering the same thing myself as to whether or not that mattered to the caller. Those functions have been around for awhile, and I don't know the history of why the -1 was chosen. Maybe it does matter; maybe not?
There are of course potential adverse ramifications of changing the return value from -1 to 0 though. For example, a quick grep shows ~100 cases of things like the following (which is of course only a subset of all the problems):
offset += tvb_length_remaining(...);
I haven't really looked at these occurrences in any detail, but I'm willing to bet that many of those assignments are incorrect since they're not considering either -1 or 0 as a possible return value. And if 0 is returned instead of -1, then consider what will happen if this assignment is being made within a loop construct - you could end up getting stuck in an infinite loop (depending on loop conditions of course), since the offset will, at some point, no longer be increasing, whereas currently the offset would either be:
1) decrementing by 1 if the offset is a signed integer, which will almost certainly produce weird/strange/incorrect results and could also potentially lead to an infinite loop in its own right, or
2) increasing by a very large value, which will probably cause a mal-formed packet condition the next time data is attempted to be grabbed from the tvb.
So, changing the -1 to 0 will correct *some* problems, but definitely not all of them, and for those that it doesn't fix, I guess there's no getting around having to manually fix each one. Still, I didn't know if fixing *some* of them was enough motivation to make the change anyway?
(And then in addition to that, a great many of these very likely should be changed from tvb_length_remaining() to tvb_reported_length_remaining(), but that's another problem ...)
Should tvb_length_remaining() not be (mostly) globally replaced with
tvb_ensure_length_remaining() (which throws an exception when offset is
out of bounds)?
Maybe:
1) rename tvb_length_remaining() to tvb_length_remaining_no_exception()
- for those users who really want to get (and can handle) the -1
2) rename tvb_ensure_length_remaining() to tvb_length_remaining()
