> -----Original Message-----
> From: [email protected] [mailto:wireshark-dev-
> [email protected]] On Behalf Of Martin Kaiser
> Sent: Thursday, October 18, 2012 5:11 PM
> To: [email protected]
> Subject: Re: [Wireshark-dev] [Wireshark-commits] rev 45566:
> /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-rdp.c
>
> Hi,
>
> Thus wrote Maynard, Chris ([email protected]):
>
> > 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 ...)
--
CONFIDENTIALITY NOTICE: The information contained in this email message is
intended only for use of the intended recipient. If the reader of this message
is not the intended recipient, you are hereby notified that any dissemination,
distribution or copying of this communication is strictly prohibited. If you
have received this communication in error, please immediately delete it from
your system and notify the sender by replying to this email. Thank you.
___________________________________________________________________________
Sent via: Wireshark-dev mailing list <[email protected]>
Archives: http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:[email protected]?subject=unsubscribe
